Skip to content

Make the NewX() monitoring variables safe to re-register#83

Merged
fearful-symmetry merged 2 commits intoelastic:mainfrom
fearful-symmetry:makeMonMetricsSafer
Oct 11, 2022
Merged

Make the NewX() monitoring variables safe to re-register#83
fearful-symmetry merged 2 commits intoelastic:mainfrom
fearful-symmetry:makeMonMetricsSafer

Conversation

@fearful-symmetry
Copy link
Copy Markdown
Contributor

@fearful-symmetry fearful-symmetry commented Oct 5, 2022

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:

  • Instead of panicking when a variable is re-registered, we return the existing variable. My concern is that this is a tad implicit, and code might be assuming that a call to NewX() will create a new monitoring variable with a zero value. Should we zero out the underlying metric, and then return?
  • If someone tries to re-register a variable with options, we just revert to a panic. I figured this was the most understandable behavior, as certain options, such as expvar exporting, can't really be modified. Should we attempt to merge or reset variables where possible?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.md

@fearful-symmetry fearful-symmetry added bug Something isn't working Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.6.0 labels Oct 5, 2022
@fearful-symmetry fearful-symmetry self-assigned this Oct 5, 2022
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner October 5, 2022 22:00
@fearful-symmetry fearful-symmetry requested review from cmacknz and faec and removed request for a team October 5, 2022 22:00
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Oct 5, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-10-05T22:25:15.796+0000

  • Duration: 9 min 26 sec

Test stats 🧪

Test Results
Failed 0
Passed 723
Skipped 5
Total 728

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link
Copy Markdown
Contributor

@faec faec left a comment

Choose a reason for hiding this comment

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

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.

@fearful-symmetry
Copy link
Copy Markdown
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V2: Intermittent panic on output reload

3 participants