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

Default aggregation for SumObserver and UpDownSumObserver #944

Closed
nilebox opened this issue Sep 14, 2020 · 4 comments
Closed

Default aggregation for SumObserver and UpDownSumObserver #944

nilebox opened this issue Sep 14, 2020 · 4 comments
Assignees
Labels
area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:metrics Related to the specification/metrics directory

Comments

@nilebox
Copy link
Member

nilebox commented Sep 14, 2020

What are you trying to achieve?
Review requirements for default aggregation for SumObserver and UpDownSumObserver, and confirm what should be the behavior in all SDKs for consistency.

First of all, SDKs currently implement aggregation for SumObserver and UpDownSumObserver differently:

According to spec, they should be using the Sum aggregation: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/metrics/api.md#aggregations

The additive instruments (Counter, UpDownCounter, SumObserver, UpDownSumObserver) use a Sum aggregation by default.

If there is exactly one data point per measurement interval, there is no difference between using LastValue vs Sum as far as I understand. It may be important for consistency and correct handling of cases where there happens to be more than one data point per interval though.

Additional context.

There are other cases of inconsistency in aggregation, e.g. ValueObserver which has reached consensus to use LastValue in all SDKs, but the spec still states that it should be using MinMaxSumCount. UPDATE: created #945 to track separately.

/cc @jmacd @james-bebbington @bogdandrutu

@nilebox nilebox added the spec:metrics Related to the specification/metrics directory label Sep 14, 2020
@Oberon00 Oberon00 added area:sdk Related to the SDK area:api Cross language API specification issue and removed area:api Cross language API specification issue labels Sep 15, 2020
@Oberon00
Copy link
Member

It seems the issue in question is already solved in the Spec. However, it may still be sensible to have this issue to make @open-telemetry/java-approvers and @open-telemetry/python-approvers and potentially other SIGs aware their implementation is not compliant. The metric compliance matrix is still TBD currently I think.

@andrewhsu andrewhsu added priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 15, 2020
@bogdandrutu
Copy link
Member

If there is exactly one data point per measurement interval, there is no difference between using LastValue vs Sum as far as I understand. It may be important for consistency and correct handling of cases where there happens to be more than one data point per interval though.

@nilebox in this case it really does not matter, because we specifically say that observers are called once per collection interval, but it may be that the observer returns multiple values for the same combination of labels (which I think is forbidden by the specs). That being said consistency should be there, and we should change all the SDKs to use the same aggregation.

@bogdandrutu bogdandrutu added priority:p1 Highest priority level and removed priority:p2 Medium priority level labels Oct 16, 2020
@reyang reyang assigned reyang and unassigned jmacd Mar 3, 2021
@reyang reyang added this to the Metrics API/SDK Feature Freeze milestone Mar 3, 2021
@jsuereth
Copy link
Contributor

jsuereth commented Oct 7, 2021

Update on Java.

  1. Collection should only be ONCE for observer instruments.
  2. Multiple writes to the same metric-stream (i.e. same set of attributes) is considered an error and is logged, but not allowed. First written should win.

@reyang
Copy link
Member

reyang commented Oct 19, 2021

I think this is addressed by #1842. Please reopen if anything not covered by the SDK spec.

@reyang reyang closed this as completed Oct 19, 2021
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 priority:p1 Highest priority level 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

No branches or pull requests

7 participants