Adding MinMax to Histograms

[mic-max]

Fixes #2560

Changes

Histograms will now record the minimum and maximum measurement values.

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[codecov]

Codecov Report

Merging #2735 (a282844) into main (af7ab89) will decrease coverage by 0.42%.
The diff coverage is 76.59%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2735      +/-   ##
==========================================
- Coverage   87.35%   86.92%   -0.43%     
==========================================
  Files         277      278       +1     
  Lines       10068    10151      +83     
==========================================
+ Hits         8795     8824      +29     
- Misses       1273     1327      +54     

Impacted Files	Coverage Δ
...ry/Metrics/ExplicitBucketHistogramConfiguration.cs	83.33% <ø> (+22.22%)	⬆️
src/OpenTelemetry/Metrics/MetricPoint.cs	75.00% <71.79%> (-8.00%)	⬇️
...tryProtocol/Implementation/MetricItemExtensions.cs	93.95% <100.00%> (+0.12%)	⬆️
src/OpenTelemetry/Metrics/HistogramBuckets.cs	100.00% <100.00%> (ø)
...rc/OpenTelemetry/Metrics/HistogramConfiguration.cs	100.00% <100.00%> (ø)
src/OpenTelemetry/Metrics/Metric.cs	95.50% <100.00%> (+1.12%)	⬆️
src/OpenTelemetry/Metrics/MetricReaderExt.cs	86.36% <100.00%> (+1.81%)	⬆️
src/OpenTelemetry/Metrics/MetricStreamIdentity.cs	90.62% <100.00%> (+0.62%)	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	35.71% <0.00%> (-42.86%)	⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	36.36% <0.00%> (-40.91%)	⬇️
... and 14 more

[cijothomas]

#2718 changes the layout, so this will be affected.

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/pull/2705/files - need to bring this back as well. (it was reverted)

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/MetricType.cs also see this class.

[cijothomas]

The PR looks good except for 2 things:

1. MinMwx should be part of Identity. Must be fixed in this PR.
2. Other exporters will break with this change. The fix is in other exporters, but blocking PR to prevent accidental merge until exporters are hotfixed. See https://github.com/open-telemetry/opentelemetry-dotnet/pull/2735/files#r989476122

[cijothomas]

The PR looks good except for 2 things:

1. MinMwx should be part of Identity. Must be fixed in this PR.
2. Other exporters will break with this change. The fix is in other exporters, but blocking PR to prevent accidental merge until exporters are hotfixed. See https://github.com/open-telemetry/opentelemetry-dotnet/pull/2735/files#r989476122

if we keep the metric type same, then it wont break existing exporters. and can use Nan/Infinity to indicate lack of min/max

[cijothomas]

double.IsNaN instead?

[alanwest]

Would that work? Reading this

IsNaN returns false if a Double value is either PositiveInfinity or NegativeInfinity. To test for these values, use the IsInfinity, IsPositiveInfinity, and IsNegativeInfinity methods.

[alanwest]

Though, I will admit that !double.IsNegativeInfinity(max) does hurt my 🧠 just a little. Maybe we should push the implementation details of negative/positive infinity meaning "unset" somewhere else.

Maybe something like

var maxIsSet = metricPoint.TryGetHistogramMax(out var max)

[alanwest]

Another thought... would it ever be the case where max is set but min is not? Maybe it would be nicer for an exporter author to be able to write something like

if (metricPoint.HasMinMax()) // This new method does the negative/positive infinity check
{
    dataPoint.Min = metricPoint.GetHistogramMin();
    dataPoint.Max = metricPoint.GetHistogramMax();
}

[mic-max]

I liked the TryGetHistogramMax way so switched it to that.

[cijothomas]

would it ever be the case where max is set but min is not

^ I cannot think of such case. I like the HasMinMax() slightly better. TryGetHistogramMax/Min() is okay as well. Will unblock PR, and we can adjust if needed

[mic-max]

Well, I set the value of Max to -Inf and Min to +Inf at initialization. So, if you record a value of -Inf then the Min and Max will be set to -Inf and the TryGetHistogramMax would incorrectly return false. Maybe I could switch the initial values to double.NaN instead.

[mic-max]

Is recording an infinity value something we should support?
Setting max default to -Inf, allows double.MinValue to be returned in the first max = Math.max(curMax, double.MinValue) at least.

Math.Min(a, b) will return NaN when a == NaN || b == NaN which I am hoping to be able to use Math.Min directly without adding another if statement for perf reasons

[mic-max]

I think I will go with the HasMinMax way now after evaluating more carefully. It also seems like this is what Java did. It is complex to use just the current value to determine if a Min/Max is present :P

[alanwest]

Is recording an infinity value something we should support?

The spec address this:

Implementations SHOULD NOT incorporate non-normal values (i.e., +Inf, -Inf, and NaNs) into the sum, min, and max fields, because these values do not map into a valid bucket.

Edit: I guess this is specifically a statement as applied to the exponential histogram aggregation. That said, I think this should equally apply to explicit bounds as well. No one wants a single non-normal value measurement to blow up their min and max...

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/pull/2735/files#r993773985
See the above.

Rest all looks good. We definitely need to add more tests for validation, esp. the delta/cumulate, and across multiple snapshots. But it can be a future PR.

[cijothomas]

Merging. @alanwest please take a final look and share any issues you see!

[CodeBlanch]

@mic-max Should this be HasMinMaxHistogram or something like that? All the other histogram specific methods have it in their names so this one feels a bit off.

[mic-max]

Ya that's valid, we could change to HistogramHasMinMax maybe?
Please share any other nits I can take care of before inclusion in its initial release :)

[cijothomas]

the initial release train just left the station 🤣 . Lets include it in next beta. (its okay to break between betas)

[CodeBlanch]

HistogramHasMinMax works for me!

Please share any other nits I can take care of before inclusion in its initial release :)

I was going to suggest maybe folding it all into a single method like...

bool TryGetHistogramMinMaxValues(out double histogramMinimumValue, out double histogramMaximumValue)

That might be faster (probably needs inlining) with less API surface but not totally sure. Would need to measure it to see feel free to try if you are interested.

[CodeBlanch]

@cijothomas Hey I was just looking at this API as part of some other work. I think we should change it! Reason being, as it is right now...

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public double GetHistogramMin()
        {
            return this.histogramBuckets.SnapshotMin;
        }

...that is a NullRef just waiting to be thrown 🤣 All the other methods in here validate the type before accessing the value.

This would be much safer and have comparable perf...

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool TryGetHistogramMinMaxValues(out double histogramMinimumValue, out double histogramMaximumValue)
{
   if (this.aggType == AggregationType.HistogramMinMax ||
                   this.aggType == AggregationType.HistogramSumCountMinMax)
   {
      Debug.Assert(this.histogramBuckets != null, "histogramBuckets was null");

      histogramMinimumValue = this.histogramBuckets.SnapshotMin;
      histogramMaximumValue = this.histogramBuckets.SnapshotMax;
      return true;
   }

   histogramMinimumValue = 0;
   histogramMaximumValue = 0;
   return false;
}

[utpilla]

Instead of returning true or false, could we follow the pattern used for other public methods in MetricPoint such as GetSumLong etc. of throwing a not supported metric type exception when called for a non-applicable metric type?

[CodeBlanch]

Works for me. The reason I have the "Try"/bool bit is because the existing API has a "HasMinMax" check before looking at the values. Not sure if there was a reason for that or not. One thing I think we should avoid is two calls (eg GetHistogramMin & GetHistogramMax) which each have to do the validation because that would suck for perf.

[cijothomas]

this is slightly different. Its possible to be histogram, but min/max is not there (as min max is optional).