Clarify reader / exporter relationship for temporality preference and default aggregation #4142
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jack-berg
commented
Jul 11, 2024
reyang
reviewed
Jul 11, 2024
reyang
reviewed
Jul 11, 2024
dashpole
approved these changes
Jul 15, 2024
jack-berg
changed the title
jack-berg
changed the title
…b.com/jack-berg/opentelemetry-specification into clarify-metric-exporter-configuration
jmacd
approved these changes
Jul 15, 2024
carlosalberto
approved these changes
Jul 20, 2024
|
Will merge wednesday 7/24 if there are no additional comments. |
jack-berg
added a commit
that referenced
this pull request
Jul 23, 2024
Splitting off a portion of #4142. This PR makes the host and port configuration options explicit for the prometheus exporter. Currently, they're [implied from the corresponding env vars](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#prometheus-exporter) `OTEL_EXPORTER_PROMETHEUS_HOST` and `OTEL_EXPORTER_PROMETHEUS_PORT`. See this [convo](https://github.com/open-telemetry/opentelemetry-specification/pull/4142/files#r1674675871) for more details. --------- Co-authored-by: Robert Pająk <pellared@hotmail.com> Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
…y-specification into clarify-metric-exporter-configuration
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewing @dashpole's comments on #4124, I realize that a key clarification is missing for the contract between readers and exporters, and for the configuration of built-in exporters.
We state that MetricReaders should be constructed with:
And for MetricExporters, we state that "The aggregation and temporality properties used by the OpenTelemetry Metric SDK are determined when registering Metric Exporters through their associated MetricReader."
But we don't connect the dots that the
aggregationandtemporalityoptions for the MetricReader should be obtained from a MetricExporter. I think this is understood, since the whole reason these are configurable at the MetricReader level is that MetricExporters and their associated backends have different requirements around temporality and default aggregation (especially in the case of base2 exponential histogram).This PR clarifies that the MetricReader
aggregationandtemporalityoptions SHOULD come from the associated MetricExporter, NOT from the caller.Furthermore, because these are now clearly spelled out as preferences of MetricExporter, I've updated all the built-in metric exporter definitions to make expectations clear around what defaults and configuration of
aggregationandtemporalityshould be supported. This achieves the goals of #4124 in a more generic way.aggregationtemporality