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

Do not collect the histogram sum for UpDownCounter or ObservableGauge instruments #4161

Closed
MrAlias opened this issue Jun 1, 2023 · 2 comments · Fixed by #4332
Closed

Do not collect the histogram sum for UpDownCounter or ObservableGauge instruments #4161

MrAlias opened this issue Jun 1, 2023 · 2 comments · Fixed by #4332
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jun 1, 2023

From the specification:

Histogram Aggregations

...

  • Arithmetic sum of Measurement values in population. This SHOULD NOT be collected when used with
    instruments that record negative measurements (e.g. UpDownCounter or ObservableGauge).

Proposal

Update our instrument aggregation selection code to introspect the instrument kind:

case aggregation.ExplicitBucketHistogram:
switch temporality {
case metricdata.CumulativeTemporality:
return internal.NewCumulativeHistogram[N](a), nil
case metricdata.DeltaTemporality:
return internal.NewDeltaHistogram[N](a), nil

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Jun 1, 2023
@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 14, 2023

This becomes much easier to address after #4304

Specifically because InstrumentKind is passed to aggregateFunc:

// aggregateFunc returns new aggregate functions matching agg, kind, and
// monotonic. If the agg is unknown or temporality is invalid, an error is
// returned.
func (i *inserter[N]) aggregateFunc(b aggregate.Builder[N], agg aggregation.Aggregation, kind InstrumentKind) (in aggregate.Input[N], out aggregate.Output, err error) {
switch a := agg.(type) {
case aggregation.Default:
return i.aggregateFunc(b, DefaultAggregationSelector(kind), kind)
case aggregation.Drop:
// Return nil in and out to signify the drop aggregator.
case aggregation.LastValue:
in, out = b.LastValue()
case aggregation.Sum:
switch kind {
case InstrumentKindObservableCounter:
in, out = b.PrecomputedSum(true)
case InstrumentKindObservableUpDownCounter:
in, out = b.PrecomputedSum(false)
case InstrumentKindCounter, InstrumentKindHistogram:
in, out = b.Sum(true)
default:
// InstrumentKindUpDownCounter, InstrumentKindObservableGauge, and
// instrumentKindUndefined or other invalid instrument kinds.
in, out = b.Sum(false)
}
case aggregation.ExplicitBucketHistogram:
in, out = b.ExplicitBucketHistogram(a)
default:
err = errUnknownAggregation
}
return in, out, err
}

@MrAlias
Copy link
Contributor Author

MrAlias commented Jul 17, 2023

Currently the SDK does not allow for the explicit histogram aggregation to be used for up-down counters or gauges. Given the specification explicitly calls out the exception, it seems like this should instead be allowed. With the correct dropping of the sum when done.

@MrAlias MrAlias self-assigned this Jul 17, 2023
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this issue Jul 17, 2023
Any instrument that can record negative values, do not include a sum in
the produced aggregation (like the specification recommends).

Resolves open-telemetry#4161
MrAlias added a commit that referenced this issue Jul 19, 2023
* Allow histogram for all instruments

Any instrument that can record negative values, do not include a sum in
the produced aggregation (like the specification recommends).

Resolves #4161

* Add changes to changelog

* Fix TestBucketsSum
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant