Histogram handle of double.NaN bounds and values

[mic-max]

Fixes #3171

Changes

• It will correctly place double.NaN values in the final histogram bucket
• Exception will be thrown if double.NaN is used as a histogram bound

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

[reyang]

What about other IEEE754 subnormal (e.g. -Inf)?

[mic-max]

Good callout. I could do a follow up change for other subnormals.
@reyang @alanwest How should we treat subnormals being used as bounds or values.
I assume bounds, we should throw exception. But for values, there's decisions to be made (SDK specific) of whether we should have them affect bucket counts (and which bucket), sum, min, and max for histograms. But I guess this also expands to the other instrument types.

[reyang]

Check these:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#numerical-limits-handling

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exponential-histogram-aggregation-handle-all-normal-values

[reyang]

The 2nd link applies to exponential histogram, but many ideas might be helpful for explicit bucket histogram, too.

[alanwest]

Nice. Hadn't seen the new wording for exponential histograms. I think it makes sense to follow this for explicit-bound histograms too. That is, effectively drop all sub-normal values from affecting min, max, and sum. I also think this wording would suggest that it is ok to not modify bucket counts either:

because these values do not map into a valid bucket

[reyang]

Sharing some additional context/thoughts:

[reyang]

I think for Sum, the challenge is that even if we don't take subnormal as the input, the output could still be subnormal:

opentelemetry-dotnet/test/OpenTelemetry.Exporter.Prometheus.Tests/PrometheusSerializerTests.cs

Lines 176 to 186 in d983cb1

	counter.Add(1.0E308);
	counter.Add(1.0E308);
	provider.ForceFlush();
	var cursor = PrometheusSerializer.WriteMetric(buffer, 0, metrics[0]);
	Assert.Matches(
	("^"
	+ "# TYPE test_counter counter\n"
	+ "test_counter \\+Inf \\d+\n"
	+ "$").Replace('\'', '"'),

[mic-max]

@CodeBlanch I changed how recording a double.NaN affects the histogram sum. I thought it should not affect it, this fails the prometheus serializer test. How do you think we should handle this value? I don't see a specific mention of it in the OTel spec

[reyang]

this fails the prometheus serializer test. How do you think we should handle this value?

@mic-max PrometheusExporter is still experimental, feel free to make changes to it (including the tests) if you think it makes sense (based on what the latest Stable SDK spec says).

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.