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.
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
[EXPORTER] Prometheus: Add unit to names, convert to word #2213
[EXPORTER] Prometheus: Add unit to names, convert to word #2213
Changes from 3 commits
06b2288
fb15283
41dda7b
8da0585
c0dea97
ac9a1aa
1102d09
8fb3e8a
f34923c
638c5c9
54a5686
cd310b9
71638d4
53c1fa3
498caad
cc949cc
c80ebb9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code moved inside the loop on data points.
The metric name, type, help, etc should be constant for all data points, so can this be moved outside ?
Even if
type
is needed and not available elsewhere, peek at the first data point before entering the loop on data points ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. As long as it is safe to assume all data points have the same type, peeking at the first seems like the right thing to do. I took that approach in https://github.com/open-telemetry/opentelemetry-cpp/pull/2288/files#diff-e92aebc994eed5c134b09ecde972d9db39a9bfc46edc6d93b0b5a4c98e4ff27fR46.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, moved back to where it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the prometheus C++ library merge metric families? We definitely don't want one metric family for each data point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, creating new family only if it doesn't exist already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've removed B and $ from mappings elsewhere, as B means
bel
in UCUM. See open-telemetry/opentelemetry-java#5719There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
do not execute the find twice, save the result in
pos
and test forpos == std::string::npos
at the start of this method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counters MUST end in _total, so we need to be less tolerant here. We can only skip the _total suffix if the name ends in _total. Since we need unit suffixes to come before type suffixes, one approach would be to trim _total suffixes from the name before appending the unit suffix, and finally append the final _total suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will not add total unless if it has already.