-
Notifications
You must be signed in to change notification settings - Fork 889
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
Clarify how metric metadata and type suffixes are handled #2440
Conversation
c703a18
to
2fdcdf2
Compare
2fdcdf2
to
78d8b7e
Compare
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.
LGTM, there might be some corner cases such as https://github.com/open-telemetry/opentelemetry-specification/pull/2440/files#r834957165 but I guess it's probably a rare case.
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
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 this need to specify how to handle metrics with the '_created' suffix?
I noticed that when I looked through the spec as well. IIUC, that would give us the start timestamp of the metric, which would be helpful. I still need to think through how to use it (e.g. will we generate an additional metric with a |
This can be followed up in a separate PR if it turns out to be needed. |
Fixes #2437
Changes
Explicitly call out how to handle prometheus metric metadata and OTLP metric metadata. Most of this was implicit, but this makes it more explicit. This PR also clarifies that
_total
suffixes are trimmed from metrics, which is already the case in the collector.It proposes the following changes from the current state:
_info
suffixes from info metrics (after support for info is implemented: [prometheusreceiver] Convert Info and StateSet metrics to non-monotonic sums opentelemetry-collector-contrib#8516)Unit suffixes are required by the OpenMetrics spec
cc @aabmass @legendecas @jmacd @Aneurysm9