-
Notifications
You must be signed in to change notification settings - Fork 894
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
Metrics SDK: Accumulator and Processor state requirements #1198
Conversation
|
||
The Accumulator interacts with the Processor during the call to its | ||
`Collect()` method, during which it calls `Process()` once per | ||
ExportRecord on the Processor. |
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.
Is there a reason why the Controller doesn't mediate this? Couldn't the controller ask the accumulator for the data, then pass it to the Processor, rather than have the Accumulator know about the processor directly? What do we gain from the extra coupling between Accumulator and Processor?
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.
The Controller does mediate this--it's the exclusive "owner" of the pipeline and mediates everything, I'd say. Why ask the Accumulator to pass data to the Processor? The Processor has lots of flexibility this way, e.g., to simply ignore Accumulations.
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.
My question is...why not have the controller pass this data to the processor, and decouple the accumulator from the processor?
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.
After thinking about this, I guess there's no reason to require the Accumulator to call the Processor directly. The Processor could instead implement an iterator pattern, to let the Processor scan through a collection representing the Accumulator state.
Is that a better design? I'm not sure the code will be cleaner for me in Go--if I'm to avoid allocating an intermediate data structure--but it sounds logically cleaner.
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'll ponder in my nascent java implementation what the appropriate degree of coupling might be. @breedx-splk this is something we should think about.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
@open-telemetry/specs-metrics-approvers Please take a look at this updated spec about Accumulator and Processor state. |
This requirement ensures that export pipelines constructed for | ||
stateless exporters (e.g. Statsd, OTLP with a stateless | ||
ExportKindSelector) are not forced into the use of permanent state in | ||
the Accumulator. This implies that the use of long-term state in a |
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.
It would be great if the details of how the golang implementation manages this were included in the detailed description of the model implementation. [I think this is done via some clever reference counting, IIRC].
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
Introduces basic requirements for the Processor and adds a missing requirement for the Accumulator.
Related issues #731
Relates to open-telemetry/opentelemetry-java#1976 and open-telemetry/opentelemetry-java#1978
Part of #2000.