fix(sdk-metrics): ignore NaN value recordings for histograms

[pichlermarc]

Which problem is this PR solving?

Currently you can add NaN to your instruments, which currently can cause two problems:

• sum data points being NaN forever
• inconsistent histogram count, where NaN is added to the total count, but not to any bucket

This PR changes the Histogram aggregations so that NaN is dropped.

Possibly relates to #4450 (cause for that was something entirely different)

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Added tests
• Adapted existing tests

[codecov]

Codecov Report

Merging #4455 (1414982) into main (6d276f4) will increase coverage by 2.12%.
The diff coverage is 100.00%.

❗ Current head 1414982 differs from pull request most recent head 8c00b6f. Consider uploading reports for the commit 8c00b6f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4455      +/-   ##
==========================================
+ Coverage   90.28%   92.41%   +2.12%     
==========================================
  Files         155      330     +175     
  Lines        3603     9518    +5915     
  Branches      787     2030    +1243     
==========================================
+ Hits         3253     8796    +5543     
- Misses        350      722     +372     

Files	Coverage Δ
...sdk-metrics/src/aggregator/ExponentialHistogram.ts	98.13% <100.00%> (ø)
packages/sdk-metrics/src/aggregator/Histogram.ts	92.20% <100.00%> (ø)

... and 176 files with indirect coverage changes

[dyladan]

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

Implementations are REQUIRED to accept the entire normal range of IEEE floating point values (i.e., all values except for +Inf, -Inf and NaN values).

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.

Implementations MAY round subnormal values away from zero to the nearest normal value.

[pichlermarc]

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

Implementations are REQUIRED to accept the entire normal range of IEEE floating point values (i.e., all values except for +Inf, -Inf and NaN values).
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.
Implementations MAY round subnormal values away from zero to the nearest normal value.

Ah. Should've checked that first. Thanks for pointing that out. I'll convert to draft and change it to fit the specification.

[pichlermarc]

Changed it to ignore NaN in the Accumulation instead of the SyncInstrument, should be spec compliant now.

[legendecas]

I think it would be better to handle NaN in the SumAccumulation as well since it is mentioned that sum, min, and max fields should not incorporate NaN values. Feel free to handle this in this PR or a follow-up.

[pichlermarc]

I think it would be better to handle NaN in the SumAccumulation as well since it is mentioned that sum, min, and max fields should not incorporate NaN values. Feel free to handle this in this PR or a follow-up.

@legendecas I think that section fo the spec it only intended for Histogram Aggregations.

The behavior for counters seems to be unspecified as per this section of the spec:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#numerical-limits-handling

I agree that allowing NaN can cause weird problems for users and I think the spirit of the spec is that we're free to drop NaN if we want. I'll bring it up in the SIG meeting to see if there are any counter-arguments for doing so. 🙂 I'll then follow up with a PR accordingly.