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: add asynchronous instrument details; add export pipeline terminology details #1159

Closed
wants to merge 3 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Oct 30, 2020

Changes

Fills in two missing parts of the metric SDK specification, with terminology taken from the OTel-Go implementation and requirements gathered from the API specification.

Part of #1158.

@jmacd jmacd added the spec:metrics Related to the specification/metrics directory label Oct 30, 2020
@jmacd jmacd requested review from a team October 30, 2020 07:57
@jkwatson
Copy link
Contributor

jkwatson commented Oct 30, 2020

This all seems reasonable; I don't know how to map it to the Java implementation, unfortunately.

@jmacd
Copy link
Contributor Author

jmacd commented Oct 30, 2020

@carlosalberto has begun auditing and coming up to speed on this implementation, thankfully. 😀

specification/metrics/sdk.md Outdated Show resolved Hide resolved
be cumulative or delta. Note that the term ExportKind is used in the
SDK to refer to this choice, while the same concept is called
AggregationTemporality when stored as a field in the OpenTelemetry
protocol. TODO: rename ExportKind to AggregationTemporality?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of consistency 👍

specification/metrics/sdk.md Outdated Show resolved Hide resolved
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 20, 2020
Comment on lines +294 to +295
unit of data. The Processor checkpoint interval starts and finishes
before and after calling Accumulator.Collect to process Acumulations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unit of data. The Processor checkpoint interval starts and finishes
before and after calling Accumulator.Collect to process Acumulations
unit of data. The Processor checkpoint interval starts before and finishes
after calling Accumulator.Collect to process Acumulations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I'm very interested to see the Processor spec finished. I don't have a good idea of why "start" is necessary on the Processor and how its supposed to react to that call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point. Linking these two conversations together here: #1198 (comment)

@jkwatson you wrote:

why not have the controller pass this data to the processor, and decouple the accumulator from the processor?

See also open-telemetry/opentelemetry-go#1362, where this coupling makes trouble for setting up the SDK (FYI @seanschade). Thinking in terms of the OTel-Go implementation, it would make sense for Collect() from the Accumulator to write into a channel and for the Processor to read from a channel.

I can imagine in other languages it would be more natural to use an iterator pattern to iterate over the results of collection using a ForEach() pattern, the way exporters consume the output of the Processor (although it means executing asynchronous instrument callbacks during the iteration). It won't be easy to do such a refactoring in Go because of how map iteration works--I'll try to make this explanation allow more language-specific approach and encourage decoupling, then file issues to fix the OTel-Go implementation.

@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 28, 2020
@jmacd jmacd removed the Stale label Dec 2, 2020
@jmacd jmacd reopened this Dec 2, 2020
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 31, 2020
@github-actions
Copy link

github-actions bot commented Jan 7, 2021

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 7, 2021
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 Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants