-
Notifications
You must be signed in to change notification settings - Fork 163
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
Explain the Metric API instruments #98
Conversation
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.
Good start. I especially appreciate the added "Example uses" in the relevant details sections as well as the propagated table outlining the metrics.
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.
Thank you, the examples as well as explanations go a long way to clarify metric instruments in my mind. Just a few minor comments, overall it looks great!
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.
Left some comments to fix up some links and a couple changes to language to read a little better.
Thanks @MikeGoldsmith @codeboten. I found other |
There is an outstanding discussion here, #98 (comment), but it's not about the matter at hand. If we agree about the names of the instruments and their default aggregation, then I think this PR is ready to merge. The debate above is about how we document the protocol and the internals of the SDK and export pipeline. I do not think the outcome of this debate will have an impact on the specification we write for the API. In any case we shouldn't hold this up over a decision about use of "Temporal quality" in the API documentation. |
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.
Agree the stated purpose of defining standard instrument names and their default aggregations is met!
Thanks.
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.
Really well explained! Good job!
@open-telemetry/specs-approvers this OTEP got very popular, can we review this to make progress? |
@open-telemetry/specs-approvers please approve this. We have reached agreement by all, and the only outstanding discussion point belongs to the SDK: open-telemetry/opentelemetry-proto#147 |
Please fix markdown lint errors :) |
@bogdandrutu Done. Needs one more approver 😆 |
@bogdandrutu Let's merge this. |
Yes Sir :) |
This document replaces OTEPs #93 and #96.
The number of instruments is expanded to 6.
Resolves open-telemetry/opentelemetry-specification#467
Resolves open-telemetry/opentelemetry-specification#465
Resolves open-telemetry/opentelemetry-specification#462