Add default_histogram_aggregation configuration option to prometheus exporter #4124
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
carlosalberto
approved these changes
Jul 3, 2024
|
cc @dashpole |
dashpole
reviewed
Jul 8, 2024
| histogram aggregation. The option MAY be named `default_histogram_aggregation`, | ||
| and MUST support the following values: | ||
|
|
||
| * `explicit_bucket_histogram`: Use [Explicit Bucket Histogram Aggregation](../sdk.md#explicit-bucket-histogram-aggregation). |
There was a problem hiding this comment.
This structure makes sense for environment-variable-based config, where we expect string constants, but maybe not so much for SDK exporter config? E.g. Go allows users to provide an AggregationSelector, which I don't think would comply with this.
arminru
added
spec:metrics
jack-berg
added a commit
that referenced
this pull request
Jul 24, 2024
… default aggregation (#4142) Reviewing @dashpole's [comments](#4124 (review)) 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](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricreader) MetricReaders should be constructed with: - "The default output aggregation (optional), a function of instrument kind. If not configured, the [default aggregation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#default-aggregation) SHOULD be used." - "The default output temporality (optional), a function of instrument kind. If not configured, the Cumulative temporality SHOULD be used." And for MetricExporters, we [state that](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricexporter) "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 `aggregation` and `temporality` options 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 `aggregation` and `temporality` options 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 `aggregation` and `temporality` should be supported. This achieves the goals of #4124 in a more generic way. | Exporter | default `aggregation` | `temporality` | |---|---|---| | Prometheus | configurable | always cumulative | | OTLP | configurable | configurable | | Standard Output | configurable | configurable | | InMemory | configurable | configurable |
carlosalberto
pushed a commit
to carlosalberto/opentelemetry-specification
that referenced
this pull request
Oct 31, 2024
… default aggregation (open-telemetry#4142) Reviewing @dashpole's [comments](open-telemetry#4124 (review)) on open-telemetry#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](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricreader) MetricReaders should be constructed with: - "The default output aggregation (optional), a function of instrument kind. If not configured, the [default aggregation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#default-aggregation) SHOULD be used." - "The default output temporality (optional), a function of instrument kind. If not configured, the Cumulative temporality SHOULD be used." And for MetricExporters, we [state that](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricexporter) "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 `aggregation` and `temporality` options 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 `aggregation` and `temporality` options 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 `aggregation` and `temporality` should be supported. This achieves the goals of open-telemetry#4124 in a more generic way. | Exporter | default `aggregation` | `temporality` | |---|---|---| | Prometheus | configurable | always cumulative | | OTLP | configurable | configurable | | Standard Output | configurable | configurable | | InMemory | configurable | configurable |
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.
Similar to how its possible to specify that base2 exponential histogram should be the default histogram aggregation for the otlp metric exporter, it should be possible to do so for the prometheus exporter.
This adds a new optional config option to the prometheus exporter called
default_histogram_aggregation, with the same mechanics as the OTLP-exporter equivalent.The motivation is that there is a PR to extend the file configuration schema with this option, and we have a rule that all portions of the file configuration schema should be explicitly defined or implied by language in the spec, since we don't want to add configuration options which could potentially contradict the spec.
cc @open-telemetry/configuration-maintainers, @open-telemetry/wg-prometheus, @Abhishekkr3003