-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[processor/spanmetricsprocessor] Add ms unit to latency buckets #13423
Conversation
22093ec
to
c6046c0
Compare
c6046c0
to
4273deb
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. cc @albertteoh as code owner
Should I update the changelog? |
Yes, I believe it should be mentioned in the changelog. please create an |
4273deb
to
dc1cdac
Compare
dc1cdac
to
3548984
Compare
3548984
to
5504735
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.
Forgive my ignorance, but what are the use cases for the optional metric units in the telemetry data itself? My understanding, at least for prometheus, is that units are not stored with the metric data but, instead, are carried in the metric name itself like http_request_duration_seconds
.
Hum, relevant references:
Based on that, I think the metric should be renamed to something like Also, the unit is part of the data model https://opentelemetry.io/docs/reference/specification/metrics/datamodel/#opentelemetry-protocol-data-model. I don't know if many vendors use it, but it would allow auto unit discovery. |
Description: Define the
latency
metric unit as milliseconds forspanmetricsprocessor
.Testing: unit tests
Documentation: the metric was already documented as being in milliseconds.