Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

send validator-new.version after updating the validator identity #34521

Closed
wants to merge 1 commit into from

Conversation

gystemd
Copy link

@gystemd gystemd commented Dec 19, 2023

Problem

After updating the validator identity using solana-validator set-identity, the validator-new.version is not sent for the new identity.

Summary of Changes

I simply added a datapoint_info! after the identity of the validator has been correctly updated in admin_rpc_service

Fixes #34389

@mergify mergify bot added community Community contribution need:merge-assist labels Dec 19, 2023
@mergify mergify bot requested a review from a team December 19, 2023 15:48
@gystemd gystemd force-pushed the send-validator-version branch from 591c116 to 044ff77 Compare December 21, 2023 13:56
@@ -693,6 +693,10 @@ impl AdminRpcImpl {
.cluster_info
.set_keypair(Arc::new(identity_keypair));
warn!("Identity set to {}", post_init.cluster_info.id());
solana_metrics::datapoint_info!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that all the data should be passed as it is done during the initialization of the validator, otherwise a query with a condition on the cluster won't return these data at all.

@steviez
Copy link
Contributor

steviez commented Dec 22, 2023

@diman-io - I see that you created the issue #34389 originally. I have a few questions / comments:

  1. Can you elaborate more on the problem you see that currently exists? I saw your comment in the issue (posted below), but can you add why this is problematic ? The node will begin submitting metrics with the new key automatically.

After updating the validator identity using solana-validator set-identity, the validator-new.version is not sent for the new identity.

  1. Regardless of above, we should NOT re-use the existing datapoint name validator-new. This datapoint corresponds to a specific event, the completion of Validator::new(). This is a distinct event and should have a distinct metric.. I would name it something like validator-set_identity.
  2. Potentially related to motivation for this PR, I would probably include the old validator identity in the metric as well. This would provide a way to "link" metrics submitted with old identity and metrics submitted with new identity.

@diman-io
Copy link
Contributor

@steviez

Honestly, I never knew why exactly 'new' was used. I assumed there was some kind of 'legacy' validator datapoint, and then a 'new' one was created :) Thanks for the clarification. If it's for tracking new launches, then yes, we definitely can't do it the way I suggested.

The start of the issue can be considered here https://discord.com/channels/428295358100013066/689412830075551748/1183118976075694100

image

But if the operator uses a primary/secondary update scheme (or always launches with a fake one), then it won't be possible in the metrics to link their real identity with the actual version. And you can't really calculate the number of certain versions in the cluster. So, this requires a solution beyond my level, but then the question is whether it's necessary, or if counting the numbers on a picture was just a one-time thing for a chat in Discord.

Personally, I would be interested in seeing the version/name of the client, but I'm still waiting for it to be broadcasted through gossip someday. That conversation on Discord just showed me that this is somehow already being sent, and I just realized that the version gets lost when the identity changes, so I created an issue for a quick fix.

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 8, 2024
@github-actions github-actions bot closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution need:merge-assist stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validator-new.version does not update when the identity is changed
3 participants