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

Metric SDK Accumulator requirements and details #880

Merged
merged 34 commits into from
Sep 29, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 26, 2020

Changes

Adds detail on the SDK Accumulator components and its responsibilities to the OpenTelemetry Metric API.

Fixes #626. This now includes at least a full skeleton of the document.
Fixes #607.

@jmacd jmacd added the spec:metrics Related to the specification/metrics directory label Aug 26, 2020
@jmacd jmacd requested review from a team August 26, 2020 07:56
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jkwatson
Copy link
Contributor

I'm trying to wrap my head around how this all maps to the existing Java implementation. I think that, roughly, what you call an Accumulator is Java's MeterSdk, and what you call a Controller is Java's MetricProducer, but I don't think the mapping is exactly 1:1.

For example, in Java, there is not really a single thing that "Receives metric events from the API, computes one Accumulation per active Instrument and Label Set pair" This is done in the individual Instrument implementation instances, with the assistance of an associated Aggregator.

So, I understand that this is supposed to be documenting a "model" implementation of the SDK, but I'm worried that, when we get ready for GA, we'll have a feature matrix, like we do for tracing, that will basically have to be completely blank (or minus-signs) for Java, since we won't have any of these components.

I'd love to see a document that outlines the functionality that an SDK MUST provide, aside from this model implementation, which reads very prescriptive in its language, despite the insistence that this is only describing one possible implementation.

@jmacd
Copy link
Contributor Author

jmacd commented Aug 26, 2020

@jkwatson your feedback is valuable. Let me know how we can get this merged so that we can keep refining this document (together). Thanks.

specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
@jmacd jmacd changed the title Metric SDK Accumulator details Metric SDK Accumulator requirements and details Aug 27, 2020
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.

Great work on this!

@jmacd
Copy link
Contributor Author

jmacd commented Sep 18, 2020

@open-telemetry/specs-metrics-approvers PTAL. I've reorganized the material in this draft to call out a separate "SDK" and "Accumulator" components to try and address confusion raised by several reviewers. I added a new diagram to clarify data flow through the Accumulator.

@jmacd jmacd assigned bogdandrutu and unassigned carlosalberto Sep 18, 2020
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
jmacd and others added 3 commits September 28, 2020 23:22
Thanks @MrAlias

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@jmacd
Copy link
Contributor Author

jmacd commented Sep 29, 2020

@bogdandrutu I think this is ready.

@bogdandrutu bogdandrutu merged commit ac1b085 into open-telemetry:master Sep 29, 2020
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* Metric SDK Accumulator details

* Add changelog

* Newline

* Apply suggestions from code review

Co-authored-by: Aaron Abbott <aaronabbott@google.com>

* Fix link

* Add detail on MeterImpl wrapper

* Update specification/metrics/sdk.md

Co-authored-by: John Watson <jkwatson@gmail.com>

* Use ```go

* From recent feedback

* Edits

* Typo

* De-lint

* Refinements about Accumulator

* Lint

* Reword what Accumulator implements

* Address question about duplicate registration

* Add Accumulator.Collect() requirements

* Spelling

* Add controller to diagram

* Provider->MeterProvider

* Add image link

* Update image

* Make SDK vs Accumulator more distinct

* Move some Accumulator requirements into SDK requirements

* Spelling

* Lint

* Remove uses of "itself"; edits

* Move the diagram

* Apply suggestions from code review

Thanks @MrAlias

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

* Merge (again)

Co-authored-by: Aaron Abbott <aaronabbott@google.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory
Projects
None yet
10 participants