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

libbeat/monitoring/inputmon: log key, id and input type when registering/deregistering metrics #35647

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Jun 1, 2023

What does this PR do?

This notes under "metric_registry" all inputmon-handled metric registration and deregistration, linking register and deregister operations by use of a unique ID unrelated to the request.

Why is it important?

We have had a number of cases where metric registrations have been duplicated, this addition allows us to identify pairs of registration/deregistration and correlate with prior un-deregistered registrations with the same name. The logging at INFO level speeds recovery of the information with minimal additional log load.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • Marked as backporting for benefits to support.

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@efd6 efd6 added enhancement Team:Security-External Integrations backport-skip Skip notification from the automated backport with mergify 8.9-candidate labels Jun 1, 2023
@efd6 efd6 self-assigned this Jun 1, 2023
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 1, 2023
@efd6 efd6 requested a review from a team June 1, 2023 23:05
…ing/deregistering metrics

This notes under "metric_registry" all inputmon-handled metric
registration and deregistration, linking register and deregister
operations by use of a unique ID unrelated to the request.
@efd6 efd6 force-pushed the s3431-inputmon branch from 57a4ff4 to f380cf3 Compare June 1, 2023 23:06
@elasticmachine
Copy link
Collaborator

💚 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: 2023-06-01T23:06:43.476+0000

  • Duration: 69 min 5 sec

Test stats 🧪

Test Results
Failed 0
Passed 26421
Skipped 1975
Total 28396

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@efd6 efd6 marked this pull request as ready for review June 2, 2023 00:21
@efd6 efd6 requested a review from a team as a code owner June 2, 2023 00:21
@efd6 efd6 requested review from pierrehilbert and faec June 2, 2023 00:21
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@efd6 efd6 added backport-v8.8.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Jun 2, 2023
@kcreddy kcreddy added the libbeat label Jun 2, 2023
@andrewkroh
Copy link
Member

andrewkroh commented Jun 2, 2023

I assume this is related to the panics that this causes. One suggestion is to modify NewInputRegistry to return an error if the ID is already registered. The upstream elastic-agent-libs/monitoring package would need extended to offer a method that does not panic.

What's an example cause of a duplicated registration that this logging would help us trace?

@efd6
Copy link
Contributor Author

efd6 commented Jun 4, 2023

I'm not convinced that relaxing the constraint of registration is a good idea.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

My main driver for making the change upstream is to lower the consequence of these programming mistakes on production systems. I like that the panic alerts us to these issues, but in production I would choose to have the input continue to operate and log and error about metric collection issues. Something along the lines of https://pkg.go.dev/go.uber.org/zap#Logger.DPanic behavior.

But the logging is useful in any case for debugging these problems.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Jun 5, 2023
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@efd6 efd6 merged commit 8abdf32 into elastic:main Jun 5, 2023
mergify bot pushed a commit that referenced this pull request Jun 5, 2023
…ing/deregistering metrics (#35647)

This notes under "metric_registry" all inputmon-handled metric
registration and deregistration, linking register and deregister
operations by use of a unique ID unrelated to the request.

(cherry picked from commit 8abdf32)

# Conflicts:
#	libbeat/monitoring/inputmon/input.go
efd6 added a commit that referenced this pull request Jun 5, 2023
…nput type when registering/deregistering metrics (#35673)

* libbeat/monitoring/inputmon: log key, id and input type when registering/deregistering metrics (#35647)

This notes under "metric_registry" all inputmon-handled metric
registration and deregistration, linking register and deregister
operations by use of a unique ID unrelated to the request.

(cherry picked from commit 8abdf32)

# Conflicts:
#	libbeat/monitoring/inputmon/input.go

* resolve conflicts

---------

Co-authored-by: Dan Kortschak <[email protected]>
Co-authored-by: Dan Kortschak <[email protected]>
@reakaleek reakaleek mentioned this pull request Jul 19, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.9-candidate backport-v8.8.0 Automated backport with mergify enhancement libbeat Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants