Make the NewX() monitoring variables safe to re-register#83
Merged
fearful-symmetry merged 2 commits intoelastic:mainfrom Oct 11, 2022
Merged
Make the NewX() monitoring variables safe to re-register#83fearful-symmetry merged 2 commits intoelastic:mainfrom
fearful-symmetry merged 2 commits intoelastic:mainfrom
Conversation
Collaborator
faec
approved these changes
Oct 10, 2022
Contributor
faec
left a comment
There was a problem hiding this comment.
This all seems reasonable. I think we probably don't want to reset a metric that has already been defined. However, can we still log a warning when things are redefined? It makes sense not to panic, but I think we should still try to remove redefinitions when we discover them, so I'd still rather have them show up in the logs.
Contributor
Author
|
@faec regarding logging, the main use case here is monitoring variables that get hit when elastic-agent reloads a beat component. So my thinking is that we would fill the logs with scary-looking but irrelevant log lines, but it might still be worth doing at a debug level? |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes elastic/beats#33125
Because this problem makes me a tad paranoid, I decided to implement the fix on a more global level. Specifically, this changes the behavior of the
NewX()monitoring functions so they don't panic when a variable of the same name and type is re-registered. This fix is a tad opinionated, and I'd like to hear input from others on a few things:NewX()will create a new monitoring variable with a zero value. Should we zero out the underlying metric, and then return?Checklist
CHANGELOG.md