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

metrics-exporter-prometheus: properly sanitize metric names #290

Merged
merged 4 commits into from
Apr 4, 2022

Conversation

gnuvince
Copy link
Contributor

Closes #289

#289 describes a case when a valid metric name is sanitized (i.e., changed) when it need not be. I added a test to exercise this case and modified the sanitize_metrics_name function to pass this test. In terms of approach, I changed from using methods on strings to using a pre-constructed lookup table. The helper functions that were previously used to implement sanitize_metrics_name have been moved in the test module to act as oracles in the property tests.

@tobz
Copy link
Member

tobz commented Mar 18, 2022

@gnuvince Thanks for opening the initial issue and the subsequent PR to fix it.

I think the lookup table approach is a bit too optimized. I know, this might sound silly, but given the pattern of how an exporter is used -- typically on an interval or polled infrequently -- the lookup table-based approach feels like it's trading far too much in terms of obviousness and readability for what is likely a small improvement in overall performance.

I'd like to a see a more iterative, straightforward approach -- replace the initial character, even if it results in a suboptimal clone, then do the remaining steps -- as a first pass of fixing this issue.

@gnuvince
Copy link
Contributor Author

gnuvince commented Mar 18, 2022

How about this version? If performance is not much of an issue, I think this is the clearest I can make it.

Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Looking a lot simpler and straightforward, just one more small tweak.

metrics-exporter-prometheus/src/formatting.rs Outdated Show resolved Hide resolved
gnuvince and others added 2 commits March 19, 2022 13:06
Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
@tobz tobz added C-exporter Component: exporters such as Prometheus, TCP, etc. E-simple Effort: simple. T-bug Type: bug. S-awaiting-release Status: awaiting a release to be considered fixed/implemented. labels Mar 20, 2022
@tobz tobz changed the title Fix #289 metrics-exporter-prometheus: properly sanitize metric names Mar 20, 2022
@tobz tobz merged commit ed7e2e9 into metrics-rs:main Apr 4, 2022
@tobz
Copy link
Member

tobz commented May 30, 2022

Released in metrics-exporter-prometheus@v0.10.0.

@tobz tobz removed the S-awaiting-release Status: awaiting a release to be considered fixed/implemented. label May 30, 2022
@gnuvince gnuvince deleted the fix-289 branch May 31, 2022 14:01
mnpw pushed a commit to mnpw/metrics that referenced this pull request Mar 8, 2024
…rs#290)

Co-authored-by: Toby Lawrence <tobz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-exporter Component: exporters such as Prometheus, TCP, etc. E-simple Effort: simple. T-bug Type: bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prometheus exporter sanitizes valid metric names
2 participants