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.

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

Remove the concept of supported temporality, keep preferred #2154

Merged
merged 3 commits into from
Nov 24, 2021

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Nov 23, 2021

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

Fixes #2130

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM. Need to update the changelog.

@reyang reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory labels Nov 23, 2021
@reyang reyang added this to the Metrics API/SDK Stable Release milestone Nov 23, 2021
@reyang reyang added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Nov 23, 2021
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Comment on lines +567 to +568
preferred temporality is explicitly specified then the SDK SHOULD respect that,
otherwise use Cumulative.
Copy link
Contributor

@MrAlias MrAlias Nov 23, 2021

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?

@bogdandrutu bogdandrutu enabled auto-merge (squash) November 24, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify the need and the difference between supported and preferred temporality?
7 participants