feat(histogram): align collection of optional Histogram properties with spec

[pichlermarc]

Which problem is this PR solving?

With a Histogram Aggregation, sum was collected even when using it with instruments like UpDownCounter or ObservableGauge, which allow negative measurements. However, the specification states this:

Arithmetic sum of Measurement values in population. This SHOULD NOT be collected when used with
instruments that record negative measurements (e.g. UpDownCounter or ObservableGauge).

This PR changes the Aggregation so that sum is only exported when used with instruments that only allow non-negative values. With the previous approach this would be indicated with a flag. This approach became unwieldy, so instead the type for sum is changed to number | undefined. To prevent a mix of approaches for the same problem, min and max's types are also changed to number | undefined and the hasMinMax field is dropped.

Fixes #3090

Short description of the changes

• Changes the Histogram type's fields sum, min and max to be of type number | undefined
• Removes hasMinMax field from the Histogram type.
• Aligns the Prometheus Exporter's behavior on undefined Histogram sum with the exporter in collector-contrib (exports sum = 0).

Type of change

Please delete options that are not relevant.

• New feature
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • sum is now optional on Histogram and might be undefined
  • min and max are now optional on Histogram and might be undefined
  • hasMinMax has been removed on Histogram

How Has This Been Tested?

• Added unit tests
• Adapted existing unit tests

Checklist:

• Followed the style guidelines of this project
• Unit tests have been added
• Documentation has been updated

[codecov]

Codecov Report

Merging #3102 (a9526bf) into main (d5cf120) will decrease coverage by 0.97%.
The diff coverage is 100.00%.

❗ Current head a9526bf differs from pull request most recent head 2d6739d. Consider uploading reports for the commit 2d6739d to get more accurate results

@@            Coverage Diff             @@
##             main    #3102      +/-   ##
==========================================
- Coverage   93.08%   92.10%   -0.98%     
==========================================
  Files         188      181       -7     
  Lines        6244     5778     -466     
  Branches     1313     1228      -85     
==========================================
- Hits         5812     5322     -490     
- Misses        432      456      +24     

Impacted Files	Coverage Δ
...telemetry-sdk-metrics-base/src/aggregator/types.ts	100.00% <ø> (ø)
...elemetry-sdk-metrics-base/src/export/MetricData.ts	100.00% <ø> (ø)
.../packages/otlp-transformer/src/metrics/internal.ts	97.61% <ø> (-0.11%)	⬇️
...ry-exporter-prometheus/src/PrometheusSerializer.ts	95.27% <100.00%> (+0.11%)	⬆️
...metry-sdk-metrics-base/src/aggregator/Histogram.ts	92.00% <100.00%> (+0.95%)	⬆️
packages/opentelemetry-sdk-trace-web/src/utils.ts	65.62% <0.00%> (-29.38%)	⬇️
...ry-context-zone-peer-dep/src/ZoneContextManager.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
... and 3 more

[legendecas]

I'd find coalescing the sum value to 0 can be confusing. Would it be possible to not output this value?

[pichlermarc]

I also though it was weird, I did that as this seems to be the behavior of the exporter in collector-contrib. Not outputting the value sounds like the better approach, I adopted the idea in 695aeb8. 🙂

I'll also open an issue in collector contrib to see what people there think. 🙂

[pichlermarc]

Opened a discussion on collector-contrib here: open-telemetry/opentelemetry-collector-contrib#12646 🙂