[processor/cumulativetodelta] Output delta histograms include min and max when they shouldnt

[jack-berg]

What happened?

Description

The cumulativetodelta processor retains the min and max from the cumulative histograms when they shouldn't. Its semantically wrong to include cumulative min and max on a resulting delta.

Steps to Reproduce

Its quite clear in the source code that the min and max fields are retained from the data point. However, I've also pushed up a commit with a test case that demonstrates it.

Expected Result

No values for resulting delta histogram min and max.

Actual Result

Cumulative histogram min and max retained.

Collector version

head

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

Its not super easy to resolve this because there isn't an obvious way to clear the min and max values. The HistogramDataPoint has methods to set min and max, but those require a float64. No method exists to set as nil, or clear.

[github-actions]

Pinging code owners: @TylerHelmuth. See Adding Labels via Comments if you do not have permissions to add labels yourself.

[TylerHelmuth]

/cc @balintzs @mistodon

[TylerHelmuth]

@dmitryax whats the best way to clear out a HistogramDataPoint's min and max? Do we need to make an empty HistogramDataPoint and copy over individual fields? Or can I add a RemoveMin/RemoveMax to pdata?

[TylerHelmuth]

@jack-berg in the meantime histogram conversion is still behind a feature gate (which will be enabled by default in 0.63.0) and can be enabled/disabled as needed.

[mx-psi]

If the min/max is smaller/greater than the one on the previous point then we could keep it. It would be weird for only some points in a given time series to have min/max, but it would conform to the specification and it may be useful (it would definitely be useful for the Datadog exporter for example :) ).

[jack-berg]

@jack-berg in the meantime histogram conversion is still behind a feature gate

Understood 🙂

If the min/max is smaller/greater than the one on the previous point then we could keep it.

Meaning that the change in value of min / max from one point to the next indicates that the measurement was recorded in the most recent window? I'd probably lean towards removing it in all cases for consistency / simplicity, but don't feel strongly about that.

[mx-psi]

Meaning that the change in value of min / max from one point to the next indicates that the measurement was recorded in the most recent window?

Yes, indeed.

I'd probably lean towards removing it in all cases for consistency / simplicity, but don't feel strongly about that.

Thinking about it further, it would probably not affect many datapoints (since getting a new min/max on the most recent window will be rare over time), so I don't feel strongly either to be honest

[TylerHelmuth]

@dmitryax can we add a ClearMin/ClearMax to the Histogram API in pdata?

[dmitryax]

We discussed this with @bogdandrutu some time ago and decided to wait until there is a real need for ability to clear optional fields. Looks like it's the time. I'll create a separate issue where we can decide on the naming.

[TylerHelmuth]

@dmitryax thanks.