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

Remove the concept of supported temporality, keep preferred #2154

Merged
merged 3 commits into from
Nov 24, 2021
Merged

Remove the concept of supported temporality, keep preferred #2154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b272de
Remove the concept of supported temporality, keep preferred
bogdandrutu Nov 23, 2021
dec99a9
Merge branch 'main' into removetemp
bogdandrutu Nov 24, 2021
44ba8a4
Merge branch 'main' into removetemp
bogdandrutu Nov 24, 2021
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ release.
([#2101](https://github.com/open-telemetry/opentelemetry-specification/pull/2101))
- Mark In-memory, OTLP and Stdout exporter specs as Feature-freeze.
([#2131](https://github.com/open-telemetry/opentelemetry-specification/pull/2131))
- Remove the concept of supported temporality, keep preferred.
([#2154](https://github.com/open-telemetry/opentelemetry-specification/pull/2154))

### Logs

Expand Down
36 changes: 5 additions & 31 deletions specification/metrics/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,33 +581,11 @@ The SDK SHOULD provide a way to allow `MetricReader` to respond to
idiomatic approach, for example, as `OnForceFlush` and `OnShutdown` callback
functions.

The SDK SHOULD provide a way to allow [Aggregation
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. The
following logic MUST be followed to determine which temporality to be used for a
`MetricReader`:

* If the temporality is explicitly specified during `MetricReader` creation:
* If the specified temporality is supported by the `MetricReader`, use the
specified temporality.
* If the specified temporality is not supported by the `MetricReader`, treat
the conflicts as an error. It is unspecified _how_ these error should be
handled (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 temporality is not explicitly specified:
* If the `MetricReader` only supports one temporality (either Cumulative or
Delta), use the supported temporality.
* If the `MetricReader` supports both Cumulative and Delta:
* If the `MetricReader` has a preferred temporality, use the preferred
temporality.
* If the `MetricReader` does not have a preferred temporality, use
Cumulative.

If a `MetricReader` is backed by a `MetricExporter` (e.g. a [Periodic exporting
MetricReader](#periodic-exporting-metricreader) configured with an [OTLP
Exporter](./sdk_exporters/otlp.md)) it MUST use the underlying
`MetricExporter`'s preferred + supported temporality.
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.
Comment on lines +587 to +588
Copy link
Contributor

@MrAlias MrAlias Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We should strengthen the normative requirements here to ensure compatibility and keep error behavior explicitly to avoid ambiguity.

Suggested change
preferred temporality is explicitly specified then the SDK SHOULD respect that,
otherwise use Cumulative.
preferred temporality is explicitly specified then the SDK MUST provide data using that temporality if it can.
If the SDK is not capable of providing data with the explicitly specified temporality it MUST be treated as an error.
Otherwise, if no preferred temporality is specified, the SDK SHOULD provide data with Cumulative temporality.

Copy link
Member Author

Choose a reason for hiding this comment

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

MUST provide data using that temporality if it can.

MUST and "if it can" are kind of opposite, is it a MUST or not.

The reason not to use must is because 20 lines below we have:

OpenTelemetry 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    , or downgrade it to a
    Gauge, or keep consuming it as Cumulative due to the
    consideration of memory
    efficiency    ?

Copy link
Contributor

Choose a reason for hiding this comment

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

MUST provide data using that temporality if it can.

MUST and "if it can" are kind of opposite, is it a MUST or not.

The reason not to use must is because 20 lines below we have:

OpenTelemetry 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    , or downgrade it to a
    Gauge, or keep consuming it as Cumulative due to the
    consideration of memory
    efficiency    ?

Gotcha 👍 . Thoughts on being explicit with the errors response?

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 most (all?) implementations currently do the cumulative -> delta conversion, right? This would not be an error state. So since everything is allowed, do you mean that there should be a standard way of letting users know what is happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

golang does not, and probably other languages may not do as well (not sure).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on being explicit with the errors response?

Wait, how was this resolved?


[OpenTelemetry SDK](../overview.md#sdk)
authors MAY choose the best idiomatic design for their language:
Expand All @@ -619,10 +597,6 @@ authors MAY choose the best idiomatic design for their language:
[Gauge](./datamodel.md#gauge), or keep consuming it as Cumulative due to the
consideration of [memory
efficiency](./supplementary-guidelines.md#memory-management)?
* If an invalid combination of settings occurred (e.g. if a `MetricReader`
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 [supplementary
guidelines](./supplementary-guidelines.md#aggregation-temporality), which have
more context and suggestions.
Expand Down Expand Up @@ -698,7 +672,7 @@ example:
pipe.

`MetricExporter` SHOULD provide a way to allow `MetricReader` to retrieve its
preferred and supported temporality.
preferred temporality.

### Push Metric Exporter

Expand Down