Skip to content
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

Merged
merged 25 commits into from
May 6, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Apr 18, 2020

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

@jmacd jmacd added the metrics Relates to the Metrics API/SDK label Apr 18, 2020
@c24t c24t mentioned this pull request Apr 20, 2020
Copy link
Contributor

@MrAlias MrAlias left a 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.

text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
@jmacd jmacd changed the title WIP: Explain the Metric API instruments Explain the Metric API instruments Apr 23, 2020
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a 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!

text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a 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.

text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Apr 28, 2020

Thanks @MikeGoldsmith @codeboten. I found other .md files in OTel that use a syntax to avoid repeating the same link a million times, and used that for otep-88 here.

@jmacd
Copy link
Contributor Author

jmacd commented Apr 29, 2020

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.

Copy link
Member

@cijothomas cijothomas left a 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.

Copy link
Contributor

@lzchen lzchen left a 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!

text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
text/0098-metric-instruments-explained.md Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@open-telemetry/specs-approvers this OTEP got very popular, can we review this to make progress?

@jmacd
Copy link
Contributor Author

jmacd commented May 4, 2020

@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

@bogdandrutu
Copy link
Member

Please fix markdown lint errors :)

@jmacd
Copy link
Contributor Author

jmacd commented May 5, 2020

@bogdandrutu Done. Needs one more approver 😆

@jmacd
Copy link
Contributor Author

jmacd commented May 6, 2020

@bogdandrutu Let's merge this.

@bogdandrutu bogdandrutu merged commit 2e37507 into open-telemetry:master May 6, 2020
@bogdandrutu
Copy link
Member

Yes Sir :)

@jmacd jmacd deleted the jmacd/98 branch May 6, 2020 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics Relates to the Metrics API/SDK
Projects
None yet