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

[Bug]: ObservableGauge no longer dropping data points from old Attributes values #2213

Open
markdingram opened this issue Oct 15, 2024 · 2 comments
Assignees
Labels
A-metrics Area: issues related to metrics bug Something isn't working
Milestone

Comments

@markdingram
Copy link
Contributor

Related Problems?

No response

Describe the solution you'd like:

A change in behaviour is present between 0.24 & 0.26 for ObservableGauge & possibly other Async Instruments.

In 0.26 once any reading is made for a given combination of attributes that reading is always sent in subsequent collections. There's no way to remove any attributes that are no longer present in the data. This doesn't match the SDK Spec:

For asynchronous instruments with Delta or Cumulative aggregation temporality, MetricReader.Collect MUST only receive data points with measurements recorded since the previous collection.

Adapted one of the existing tests to show an error - markdingram@68d375b

Comment from @cijothomas on CNCF Slack:

OTel Rust currently is likely violating the above part of spec, as it never forgets data points for Cumulative temporality, but the spec says it should forget data points for CUMULATIVE too, for Observable instruments.

Considered Alternatives

No response

Additional Context

No response

@markdingram markdingram added enhancement New feature or request triage:todo Needs to be traiged. labels Oct 15, 2024
@kensimon
Copy link

kensimon commented Nov 7, 2024

I believe this bug was introduced here: #2021

The LastValue aggregation function used to have a comment saying "[Builder::temporality] is ignored and delta is always used", and "Delta temporality is the only temporality that makes semantic sense for a last-value aggregate", but now it obeys self.temporality and will apply cumulative data if self.temporality is not Delta.

Interestingly the doc comments for AggregateBuilder still say that Delta temporality will be used for last_value:

/// If this is not provided, a default of cumulative will be used (except for the

It would seem to me that the original comments were right... last_value should mean last value even if the AggregateBuilder's temporality is cumulative.

This is effecting me because the opentelemetry_prometheus's PrometheusExporter only supports Temporality::Cumulative:

impl TemporalitySelector for PrometheusExporter {

    /// Note: Prometheus only supports cumulative temporality so this will always be
    /// [Temporality::Cumulative].

Meaning that trying to use the prometheus exporter, there's no way to clear any gauge fields in v0.26.

@cijothomas
Copy link
Member

Thanks for reporting! Yes, this looks like a bug.

(Co-incidentally, this bug was also discovered in OTel .NET recently : open-telemetry/opentelemetry-dotnet#5952)

@cijothomas cijothomas added bug Something isn't working A-metrics Area: issues related to metrics and removed enhancement New feature or request triage:todo Needs to be traiged. labels Nov 7, 2024
@cijothomas cijothomas added this to the 0.27.1 milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metrics Area: issues related to metrics bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants