-
Notifications
You must be signed in to change notification settings - Fork 887
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
Clarify preferred aggregation temporality; give Views a selection strategy #2314
Changes from 9 commits
67db63e
a0fe16a
c3baa57
eea4d5c
5e00cda
81cde39
ca93c51
f687822
a8fe8f9
c0789de
b897c87
0f0c500
271866e
6e3b892
c20fb6f
98bae87
8e88e9e
de5188e
77e5fdf
c5a5a74
1a2d371
6f4965d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,7 +181,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. | ||
|
@@ -624,25 +625,55 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I feel that we complicate things here because we have this "notion" of 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
|
||
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, synchronous and asynchronous instruments | ||
are given separate treatment. When configuring the preferred | ||
aggregation temporality for a `MetricReaderExporter`, the | ||
jmacd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
implementation MUST provide a mechanism that supports configuring the | ||
exporter's preferred temporality on the basis of instrument kind for | ||
the five instruments `Counter`, `Asynchronous Counter`, | ||
`UpDownCounter`, `Asynchronous UpDownCounter`, and `Histogram`. | ||
|
||
A configured `MetricReader` instance MUST expose data in the preferred | ||
aggregation temporality for points deriving from the five instruments | ||
that define aggregation temporality. | ||
|
||
Two well-known preferences are named, offering a simple way to express | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to JUST provide these two options instead of per-instrument? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought it might be easier to specify with a general hook, but I'm easy to convince on this. |
||
exporter preferences through environment variables and in | ||
configuration files. The following named preferences SHOULD be | ||
supported: | ||
|
||
- **AllCumulative**: All data points with aggregation temporality are | ||
exported using Cumulative aggregation temporality. | ||
- **DeltaPreferred**: Data points from `Counter`, `Asynchronous | ||
Counter`, and `Histogram` instruments use Delta aggregation | ||
temporality, whereas whereas data points from `UpDownCounter` and | ||
`Asynchronous UpDownCounter` are exported with Cumualtive | ||
jmacd marked this conversation as resolved.
Show resolved
Hide resolved
jmacd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
aggregation temporality. | ||
|
||
If the preferred temporality is not explicitly specified, the SDK | ||
SHOULD use the AllCumulative 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 | ||
|
||
|
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.
Understand the change, but would prefer this in a separate PR since is about just "clarification" for API.