-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Rename metric instruments to match feature-freeze API specification #2202
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2202 +/- ##
=====================================
Coverage 72.9% 72.9%
=====================================
Files 176 176
Lines 12162 12162
=====================================
Hits 8868 8868
Misses 3055 3055
Partials 239 239
|
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.
Looks good overall with two minor nitpicks about the CHANGELOG. Nothing blocking.
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.
Looks good vis. a vis. syntactical changes.
My comments are nearly all regarding the "human-readable" and variable names in tests. For consistency, I would prefer to see all the names of test instruments changed in the same way we've changed the code names, e.g. we shouldn't see
valuerecorder := ...
or
NewInt64Histogram("valuerecorder.int", ...)
in the test code. but rather
histogram := ...
and
NewInt64Histogram("histogram.int", ...)
The same comment applies to the other name changes.
Co-authored-by: Tyler Yahn <[email protected]>
Thanks Co-authored-by: ET <[email protected]>
Thank you @evantorrie ! Co-authored-by: ET <[email protected]>
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.
Renames look good, just one question regarding additive vs adding, but otherwise I agree this change will greatly reduce confusion.
Thank you ❤️ @evantorrie for the fixes. I went through and found a few more and have generally tidied up the PR. |
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
This renames ValueRecorder to Histogram, ValueObserver to GaugeObserver, SumObserver to CounterObserver, and UpDownSumObserver to UpDownCounterObserver.
The API was marked "feature-freeze" here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md
This updatesgetting-started.md
with an up-to-date example of getting started with the metrics API.This is otherwise not a functional change.
Note: The OTel-Go metrics API is still considered experimental and subject to change in the future. We discussed making this change ASAP to avoid confusing new users who review the API specification and will not understand these legacy names.