-
Notifications
You must be signed in to change notification settings - Fork 888
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
Conversation
This all seems reasonable; I don't know how to map it to the Java implementation, unfortunately. |
@carlosalberto has begun auditing and coming up to speed on this implementation, thankfully. 😀 |
91481c8
to
cfb0411
Compare
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? |
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.
I like the idea of consistency 👍
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
unit of data. The Processor checkpoint interval starts and finishes | ||
before and after calling Accumulator.Collect to process Acumulations |
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.
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 |
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.
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.
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.
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.
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
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.