Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clarify preferred aggregation temporality; give Views a selection strategy #2314

Closed
wants to merge 22 commits into from
Closed

Clarify preferred aggregation temporality; give Views a selection strategy #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
67db63e
Specify preferred aggregation temporality
jmacd Feb 3, 2022
a0fe16a
edit
jmacd Feb 4, 2022
c3baa57
PR num
jmacd Feb 4, 2022
eea4d5c
Update specification/metrics/api.md
jmacd Feb 4, 2022
5e00cda
Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…
jmacd Feb 16, 2022
81cde39
Merge branch 'jmacd/preferred_tempo' of github.com:jmacd/opentelemetr…
jmacd Feb 16, 2022
ca93c51
revert spurious API diff
jmacd Feb 16, 2022
f687822
clarify 5-way function & two named prefs
jmacd Feb 16, 2022
a8fe8f9
cumulative->current
jmacd Feb 16, 2022
c0789de
Apply suggestions from code review
jmacd Feb 22, 2022
b897c87
Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…
jmacd Mar 2, 2022
0f0c500
Merge branch 'jmacd/preferred_tempo' of github.com:jmacd/opentelemetr…
jmacd Mar 2, 2022
271866e
blank
jmacd Mar 2, 2022
6e3b892
rephrase per jack-berg and alanwest
jmacd Mar 2, 2022
c20fb6f
lint
jmacd Mar 2, 2022
98bae87
Update specification/metrics/sdk.md
jmacd Mar 3, 2022
8e88e9e
remove stateless
jmacd Mar 4, 2022
de5188e
Merge branch 'jmacd/preferred_tempo' of github.com:jmacd/opentelemetr…
jmacd Mar 4, 2022
77e5fdf
update changelog
jmacd Mar 4, 2022
c5a5a74
add preference in OTLP exporter
jmacd Mar 4, 2022
1a2d371
typo
jmacd Mar 4, 2022
6f4965d
revert api change
jmacd Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ release.
([#2317](https://github.com/open-telemetry/opentelemetry-specification/pull/2317)).
- Clarify that expectations for user callback behavior are documentation REQUIREMENTs.
([#2361](https://github.com/open-telemetry/opentelemetry-specification/pull/2361)).
- Clarify what is meant by "preferred" aggregation temporality. Introduce aggregation
strategy to Sum, Histogram, and ExponentialHistogram Aggregations, for use in
View configuration to override the exporter preference.
([#2314](https://github.com/open-telemetry/opentelemetry-specification/pull/2314))

### Logs

Expand Down
87 changes: 61 additions & 26 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
* [ExemplarReservoir](#exemplarreservoir)
* [Exemplar defaults](#exemplar-defaults)
- [MetricReader](#metricreader)
* [Preferred aggregation temporality](#preferred-aggregation-temporality)
* [MetricReader operations](#metricreader-operations)
+ [Collect](#collect)
+ [Shutdown](#shutdown-1)
Expand Down Expand Up @@ -183,7 +184,8 @@ are the inputs:
apply a [default aggregation](#default-aggregation). If the aggregation
outputs metric points that use aggregation temporality (e.g. Histogram,
Sum), the SDK SHOULD handle the aggregation temporality based on the
temporality of each [MetricReader](#metricreader) instance.
[preferred aggregation temporality](#preferred-aggregation-temporality)
of each [MetricReader](#metricreader) instance.
* 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.
Expand Down Expand Up @@ -347,12 +349,12 @@ to select an aggregation and configuration parameters.

| Instrument Kind | Selected Aggregation |
| --- | --- |
| [Counter](./api.md#counter) | [Sum Aggregation](./sdk.md#sum-aggregation) |
| [Asynchronous Counter](./api.md#asynchronous-counter) | [Sum Aggregation](./sdk.md#sum-aggregation) |
| [UpDownCounter](./api.md#updowncounter) | [Sum Aggregation](./sdk.md#sum-aggregation) |
| [Asynchrounous UpDownCounter](./api.md#asynchronous-updowncounter) | [Sum Aggregation](./sdk.md#sum-aggregation) |
| [Counter](./api.md#counter) | [Sum Aggregation](./sdk.md#sum-aggregation) with preferred temporality strategy |
| [Asynchronous Counter](./api.md#asynchronous-counter) | [Sum Aggregation](./sdk.md#sum-aggregation) with preferred temporality strategy |
| [UpDownCounter](./api.md#updowncounter) | [Sum Aggregation](./sdk.md#sum-aggregation) with cumulative temporality strategy |
| [Asynchrounous UpDownCounter](./api.md#asynchronous-updowncounter) | [Sum Aggregation](./sdk.md#sum-aggregation) with cumulative temporality strategy |
| [Asynchronous Gauge](./api.md#asynchronous-gauge) | [Last Value Aggregation](./sdk.md#last-value-aggregation) |
| [Histogram](./api.md#histogram) | [Histogram Aggregation](./sdk.md#histogram-aggregation) |
| [Histogram](./api.md#histogram) | [Histogram Aggregation](./sdk.md#histogram-aggregation) with preferred temporality strategy |

This Aggregation does not have any configuration parameters.

Expand All @@ -372,7 +374,9 @@ 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 has the following configuration parameters:

- Aggregation temporality `strategy` [determines how the temporality choice is made](#preferred-aggregation-temporality).

This Aggregation informs the SDK to collect:

Expand All @@ -396,7 +400,9 @@ The Histogram Aggregation informs the SDK to select the best Histogram
Aggregation available. i.e. [Explicit Bucket Histogram
Aggregation](./sdk.md#explicit-bucket-histogram-aggregation).

This Aggregation does not have any configuration parameters.
This Aggregation has the following configuration parameters:

- Aggregation temporality `strategy` [determines how the temporality choice is made](#preferred-aggregation-temporality).

#### Explicit Bucket Histogram Aggregation

Expand All @@ -418,6 +424,7 @@ This Aggregation informs the SDK to collect:
instruments that record negative measurements, e.g. `UpDownCounter` or `ObservableGauge`.
- Min (optional) `Measurement` value in population.
- Max (optional) `Measurement` value in population.
- Aggregation temporality `strategy` [determines how the temporality choice is made](#preferred-aggregation-temporality).

### Observations inside asynchronous callbacks

Expand Down Expand Up @@ -672,25 +679,53 @@ The SDK SHOULD provide a way to allow `MetricReader` to respond to
idiomatic approach, for example, as `OnForceFlush` and `OnShutdown` callback
functions.

### Preferred aggregation temporality
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is too complicated for our users. I would like to think of a simpler solution.

Can we have our MetricReader support a configuration option that says exactly for each type of instrument what to produce?

I feel that we complicate things here because we have this "notion" of preferred, because of these we are trying to say "you may prefer delta, but you don't know what you want, I will give you cumulative for this".

I think this is too complicated, have we consider that every "exporter" instead of having a "preferred" to actually come with a concrete temporality <type, temporality> list so they have full control, and we don't try to be smarter like "Preferred".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have our MetricReader support a configuration option that says exactly for each type of instrument what to produce?

That was one of the initial proposals, the "5-way" function as it has been called in the review thread above. I think that's fine as a means of configuring the exporters that support both options. If we accept the 5-way function, which seems easy for the in-memory exporter, and fix the output temporality for the console and OTLP exporters to always cumulative, then:

(1) vendors that want always-delta or sometimes-delta will provide implementations of the 5-way function they prefer
(2) Does this mean we can erase the concept of preferred temporality entirely?


The SDK SHOULD provide a way to allow the preferred [Aggregation
Temporality](./datamodel.md#temporality) to be specified for a `MetricReader`
instance during the setup (e.g. initialization, registration, etc.) time. If the
preferred temporality is explicitly specified then the SDK SHOULD respect that,
otherwise use Cumulative.

[OpenTelemetry SDK](../overview.md#sdk)
authors MAY choose the best idiomatic design for their language:

* Whether to treat the temporality settings as recommendation or enforcement.
For example, if the temporality is set to Delta, would the SDK want to perform
Cumulative->Delta conversion for an [Asynchronous
Counter](./api.md#asynchronous-counter), or downgrade it to a
[Gauge](./datamodel.md#gauge), or keep consuming it as Cumulative due to the
consideration of [memory
efficiency](./supplementary-guidelines.md#memory-management)?
* Refer to the [supplementary
guidelines](./supplementary-guidelines.md#aggregation-temporality), which have
more context and suggestions.
Temporality](./datamodel.md#temporality) to be specified for a
`MetricReader` instance during setup (e.g. initialization,
registration, etc.). This preference gives the SDK user control over the
amount of long-term memory dedicated to reading metrics.

The synchronous instruments generally define data points having
aggregation temporality (e.g., `Sum`, `Histogram`). For these points,
a `MetricReader` configured with Cumulative aggregation temporality
implies conversion from Delta into Cumulative aggregation temporality.

The asynchronous `Counter` and `UpDownCounter` instruments are defined
by observations that are cumulative values to begin with. For these
jmacd marked this conversation as resolved.
Show resolved Hide resolved
points, a change of aggregation temporality implies conversion from
Cumulative into Delta aggregation temporality.

Because of these differences, when configuring Aggregations that
support aggregation temporality, there is a choice of Aggregation
temporality strategy.

Typically (and by default), `UpDownCounter` instruments are converted
to Cumulative temporality, ignoring the `MetricReader` preference.

The Aggregation temporality preference of the `MetricReader` combined
with the Aggregation temporality strategy of the `Aggregation`
determines the exported aggregation temporality.

The two supported `Aggregation` temporality strategies are:

- **Cumulative**: The Aggregation Temporality will be selected as
Cumulative, overriding the `MetricReader` preference.
- **Preferred**: The Aggregation Temporality will be selected
Comment on lines +713 to +715
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have a use case in mind, but does it make sense to have a Delta strategy just for completeness?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it makes sense for completeness. It takes a very contrived setup for me to imagine this being useful, but you're right!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be content with its omission without a practical use case imagined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be useful in unit tests (e.g. InMemoryExporter) but I'm fine not having it for now (it can be additive change at any time).

according to the `MetricReader` preference.

Two `MetricReader` preferences are available:

- **Cumulative**: The Metric Exporter prefers Cumulative temporality.
- **Delta**: The Metric Exporter prefers Delta temporality.

If the preferred temporality is not explicitly specified, the SDK
SHOULD use the Cumulative aggregation temporality preference.

Refer to the [data model section on Stream
Manipulations](datamodel.md#stream-manipulations) for more detail on
changing aggregation temporality.

### MetricReader operations

Expand Down
12 changes: 6 additions & 6 deletions specification/metrics/sdk_exporters/otlp.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# OpenTelemetry Metrics Exporter - OTLP
1# OpenTelemetry Metrics Exporter - OTLP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1# OpenTelemetry Metrics Exporter - OTLP
# OpenTelemetry Metrics Exporter - OTLP


**Status**: [Stable](../../document-status.md)

Expand All @@ -9,12 +9,12 @@ Exporter](../sdk.md#push-metric-exporter) which sends metrics via the
OTLP Metrics Exporter MUST support both Cumulative and Delta
[Temporality](../datamodel.md#temporality).

OTLP Metrics Exporter MUST allow [Aggregation
Temporality](../datamodel.md#temporality) to be specified, as described in
[MetricExporter](../sdk.md#metricexporter).
OTLP Metrics Exporter MUST allow [Preferred Aggregation
Temporality](../datamodel.md#preferred-aggregation-temporality) to be specified, as described in
[MetricReader](../sdk.md#metricreader).

If the temporality is not specified, OTLP Metrics Exporter SHOULD use Cumulative
as the default temporality.
If the temporality preference is not specified, OTLP Metrics Exporter SHOULD use Cumulative
as the default aggregation temporality preference.

The exporter MUST provide configuration according to the [OpenTelemetry Protocol
Exporter](../../protocol/exporter.md) specification.
Expand Down