From 9ac4f2d59981cca8b48d0dfc4f2b654bf879733c Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Mon, 18 Oct 2021 19:10:10 -0700 Subject: [PATCH 1/6] Clarify AggregationTemporality override rules --- specification/metrics/sdk.md | 44 +++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 13f3e622752..bcdc4d8c54d 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -19,6 +19,7 @@ Table of Contents * [Push Metric Exporter](#push-metric-exporter) * [Pull Metric Exporter](#pull-metric-exporter) * [Defaults and configuration](#defaults-and-configuration) +* [Temporality override rules](#temporality-override-rules) * [Numerical limits handling](#numerical-limits-handling) * [Compatibility requirements](#compatibility-requirements) * [Concurrency requirements](#concurrency-requirements) @@ -162,9 +163,12 @@ are the inputs: * The `extra dimensions` which come from Baggage/Context (optional). If not provided, no extra dimension will be used. Please note that this only applies to [synchronous Instruments](./api.md#synchronous-instrument). - * The `aggregation` (optional) to be used. If not provided, a default - aggregation will be applied by the SDK. The default aggregation is a TODO. - * The `exemplar_reservoir` (optional) to use for storing exemplars. + * The `aggregation` (optional) to be used. If not provided, the SDK SHOULD + apply a [default aggregation](#default-aggregation). If the aggregation has + temporality, the SDK SHOULD use the [temporality override + rules](#temporality-override-rules) to determine the aggregation + temporality. + * The `exemplar_reservoir` (optional) to use for storing exemplars. This should be a factory or callback similar to aggregation which allows different reservoirs to be chosen by the aggregation. @@ -575,6 +579,8 @@ authors MAY choose the best idiomatic design for their language: instance is set to use Cumulative, and it has an associated [Push Metric Exporter](#push-metric-exporter) instance which has the temporality set to Delta), would the SDK want to fail fast or use some fallback logic? +* Refer to the [temporality override rules](#temporality-override-rules) for how + to determine the temporality. * Refer to the [supplementary guidelines](./supplementary-guidelines.md#aggregation-temporality), which have more context and suggestions. @@ -668,6 +674,8 @@ language: Exporter](./sdk_exporters/prometheus.md) instance is being used, and the temporality is set to Delta), would the SDK want to fail fast or use some fallback logic? +* Refer to the [temporality override rules](#temporality-override-rules) for how + to determine the temporality. * Refer to the [supplementary guidelines](./supplementary-guidelines.md#aggregation-temporality), which have more context and suggestions. @@ -817,6 +825,36 @@ modeled to interact with other components in the SDK: The SDK MUST provide configuration according to the [SDK environment variables](../sdk-environment-variables.md) specification. +## Temporality override rules + +There are several places where [Aggregation +Temporality](./datamodel.md#temporality) can be configured in the OpenTelemetry +SDK. The SDK MUST use the following order to determine which temporality to be +used: + +* If the temporality is explicitly specified on a [View](#view), use the + specified temporality and goto END. +* If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) only + supports one temporality (either Cumulative or Delta), use the supported + temporality and goto END. +* If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) + supports both Cumulative and Delta: + * If the temporality is explicitly specified on the + [MetricExporter](#metricexporter) or [MetricReader](#metricreader), use the + specified temporality and goto END. + * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) + has a preferred temporality, use the preferred temporality and goto END. + * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) + does not have a preferred temporality, use Cumulative and goto END. +* END. + +If the above process caused conflicts (e.g. a View has an explicitly specified +temporality Delta, but the `MetricExporter` only supports Cumulative), the SDK +SHOULD treat the conflicts as error. It is unspecified _how_ the SDK should +handle these error (e.g. it could fail fast during the SDK configuration time). +Please refer to [Error handling in OpenTelemetry](../error-handling.md) for the +general guidance. + ## Numerical limits handling The SDK MUST handle numerical limits in a graceful way according to [Error From c1f61a57add05c6ce7ee409f6c1b019cfbfb4466 Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Mon, 18 Oct 2021 19:15:58 -0700 Subject: [PATCH 2/6] update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e0720246bc..9c8450fe5bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ release. ([#1935](https://github.com/open-telemetry/opentelemetry-specification/pull/1935)) - Add clarifications on how to handle numerical limits. ([#2007](https://github.com/open-telemetry/opentelemetry-specification/pull/2007)) +- Add clarifications on how to determine aggregation temporality. + ([#2013](https://github.com/open-telemetry/opentelemetry-specification/pull/2013), + [#2032](https://github.com/open-telemetry/opentelemetry-specification/pull/2032)) ### Logs From 601c0372664714b73df53e3dc9b58660060701bb Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Tue, 19 Oct 2021 08:51:50 -0700 Subject: [PATCH 3/6] remove per view setting, adjust wording --- specification/metrics/sdk.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index bcdc4d8c54d..3abf2b26d6c 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -832,20 +832,19 @@ Temporality](./datamodel.md#temporality) can be configured in the OpenTelemetry SDK. The SDK MUST use the following order to determine which temporality to be used: -* If the temporality is explicitly specified on a [View](#view), use the - specified temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) only supports one temporality (either Cumulative or Delta), use the supported temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) supports both Cumulative and Delta: - * If the temporality is explicitly specified on the - [MetricExporter](#metricexporter) or [MetricReader](#metricreader), use the - specified temporality and goto END. + * If the temporality is explicitly specified (e.g. via environment variables) + on the [MetricExporter](#metricexporter) or [MetricReader](#metricreader), + use the specified temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) has a preferred temporality, use the preferred temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) - does not have a preferred temporality, use Cumulative and goto END. + does not have a preferred temporality, use the default temporality based on + the View [aggregation](#aggregation) and goto END. * END. If the above process caused conflicts (e.g. a View has an explicitly specified From fa56d226932505dbc841b04da705818b175ecc56 Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Tue, 19 Oct 2021 08:56:21 -0700 Subject: [PATCH 4/6] simply the wording --- specification/metrics/sdk.md | 3 --- specification/metrics/sdk_exporters/otlp.md | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 3abf2b26d6c..c93cbec457f 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -837,9 +837,6 @@ used: temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) supports both Cumulative and Delta: - * If the temporality is explicitly specified (e.g. via environment variables) - on the [MetricExporter](#metricexporter) or [MetricReader](#metricreader), - use the specified temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) has a preferred temporality, use the preferred temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) diff --git a/specification/metrics/sdk_exporters/otlp.md b/specification/metrics/sdk_exporters/otlp.md index 2abd586e189..94966512140 100644 --- a/specification/metrics/sdk_exporters/otlp.md +++ b/specification/metrics/sdk_exporters/otlp.md @@ -26,4 +26,4 @@ Exporter](../../protocol/exporter.md) specification once it reaches | Description | Default | Env variable | | ----------- | ------- | ------------ | -| The output [Aggregation Temporality](../datamodel.md#temporality), either `CUMULATIVE` or `DELTA` (case insensitive) | `CUMULATIVE` | `OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY` +| The preferred output [Aggregation Temporality](../datamodel.md#temporality), either `CUMULATIVE` or `DELTA` (case insensitive) | `CUMULATIVE` | `OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY` From 6459fce1890177eef3fbd62efe6434b2714f529d Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Tue, 19 Oct 2021 09:01:29 -0700 Subject: [PATCH 5/6] remove the invalid example --- specification/metrics/sdk.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index c93cbec457f..46f7dcd46f5 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -844,12 +844,10 @@ used: the View [aggregation](#aggregation) and goto END. * END. -If the above process caused conflicts (e.g. a View has an explicitly specified -temporality Delta, but the `MetricExporter` only supports Cumulative), the SDK -SHOULD treat the conflicts as error. It is unspecified _how_ the SDK should -handle these error (e.g. it could fail fast during the SDK configuration time). -Please refer to [Error handling in OpenTelemetry](../error-handling.md) for the -general guidance. +If the above process caused conflicts, the SDK SHOULD treat the conflicts as +error. It is unspecified _how_ the SDK should handle these error (e.g. it could +fail fast during the SDK configuration time). Please refer to [Error handling in +OpenTelemetry](../error-handling.md) for the general guidance. ## Numerical limits handling From 4ba1aa34a0da13711b5f1097e2dd09c9c9317a8e Mon Sep 17 00:00:00 2001 From: Reiley Yang Date: Tue, 19 Oct 2021 13:47:59 -0700 Subject: [PATCH 6/6] address review feedback --- specification/metrics/sdk.md | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/specification/metrics/sdk.md b/specification/metrics/sdk.md index 46f7dcd46f5..f0a0693ae86 100644 --- a/specification/metrics/sdk.md +++ b/specification/metrics/sdk.md @@ -336,12 +336,6 @@ This Aggregation does not have any configuration parameters. The Sum Aggregation informs the SDK to collect data for the [Sum Metric Point](./datamodel.md#sums). -This Aggregation honors the following configuration parameters: - -| Key | Value | Default Value | Description | -| --- | --- | --- | --- | -| Temporality | Delta, Cumulative | Cumulative | | - The monotonicity of the aggregation is determined by the instrument type: | Instrument Kind | `SumType` | @@ -353,6 +347,8 @@ The monotonicity of the aggregation is determined by the instrument type: | [Asynchronous Counter](./api.md#asynchronous-counter) | Monotonic | | [Asynchrounous UpDownCounter](./api.md#asynchronous-updowncounter) | Non-Monotonic | +This Aggregation does not have any configuration parameters. + This Aggregation informs the SDK to collect: - The arithmetic sum of `Measurement` values. @@ -387,7 +383,6 @@ This Aggregation honors the following configuration parameters: | Key | Value | Default Value | Description | | --- | --- | --- | --- | -| Temporality | Delta, Cumulative | Cumulative | See [Temporality](./datamodel.md#temporality). | | Boundaries | double\[\] | [ 0, 5, 10, 25, 50, 75, 100, 250, 500, 1000 ] | Array of increasing values representing explicit bucket boundary values.

The Default Value represents the following buckets:
(-∞, 0], (0, 5.0], (5.0, 10.0], (10.0, 25.0], (25.0, 50.0], (50.0, 75.0], (75.0, 100.0], (100.0, 250.0], (250.0, 500.0], (500.0, 1000.0], (1000.0, +∞) | Note: This aggregator should not fill out `sum` when used with instruments @@ -840,8 +835,7 @@ used: * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) has a preferred temporality, use the preferred temporality and goto END. * If the [MetricExporter](#metricexporter) or [MetricReader](#metricreader) - does not have a preferred temporality, use the default temporality based on - the View [aggregation](#aggregation) and goto END. + does not have a preferred temporality, use Cumulative and goto END. * END. If the above process caused conflicts, the SDK SHOULD treat the conflicts as