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

Serve metrics from the local registry in the diagnostic service #51031

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

hugoShaka
Copy link
Contributor

In #50913 I added a local registry and assumed the metrics service was serving the metrics. It appears this is handled by default by the diagnostics service. The metrics service is undocumented (in the config reference) and disabled by default.

This PR:

  • makes both the metrics and diagnostics service create the metrics handler the same way
  • ensures metrics registered against the local registry are collected by the diagnostics service

@github-actions github-actions bot requested review from fspmarshall and tcsc January 14, 2025 18:15
@hugoShaka hugoShaka added the no-changelog Indicates that a PR does not require a changelog entry label Jan 14, 2025
@hugoShaka hugoShaka changed the title Use local metrics registry in the diagnostic service Serve metrics from the local registry in the diagnostic service Jan 14, 2025
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Any chance we could catch this through testing?

Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

+1 for adding some test for either the handler or the combined gatherer - or maybe even actually hitting /metrics on a running Teleport?

@hugoShaka
Copy link
Contributor Author

Any chance we could catch this through testing?

Done in 66970e8

or maybe even actually hitting /metrics on a running Teleport?

Yup, that's what the metrics service test was doing, I mostly copied it and made it query the diagnostic service instead. The code is a bit duplicated but I don't think the deduplicating this test code is worth the time and complexity.

@hugoShaka hugoShaka requested a review from codingllama January 15, 2025 15:48
Copy link
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Thanks, Hugo.

@hugoShaka hugoShaka added this pull request to the merge queue Jan 15, 2025
Merged via the queue into master with commit 9286ccd Jan 15, 2025
41 checks passed
@hugoShaka hugoShaka deleted the hugo/diagnostics-service-use-local-metrics-registry branch January 15, 2025 16:55
hugoShaka added a commit that referenced this pull request Jan 17, 2025
* Use local metrics registry in the diagnostic service

* Test metrics are served by the diag service
hugoShaka added a commit that referenced this pull request Jan 17, 2025
* Use local metrics registry in the diagnostic service

* Test metrics are served by the diag service
hugoShaka added a commit that referenced this pull request Jan 17, 2025
* Use local metrics registry in the diagnostic service

* Test metrics are served by the diag service
hugoShaka added a commit that referenced this pull request Jan 17, 2025
* Use local metrics registry in the diagnostic service

* Test metrics are served by the diag service
mvbrock pushed a commit that referenced this pull request Jan 18, 2025
* Use local metrics registry in the diagnostic service

* Test metrics are served by the diag service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants