fix(metrics): updates unit option behavior to be spec-compliant

[andyfleming]

Fixes #2910

Short description of the changes

Changes the behavior of the unit option for an instrument descriptor to be compliant with the specification. See additional discussion on issue.

How Has This Been Tested?

Unit tests were added to confirm behavior.

Checklist:

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

[andyfleming]

There is a downstream behavior here that will need resolved before merging. The MetricOptions interface expects any string, including a default of '', but the additional tests expect an integer value. I could use some guidance as what the behavior/update should be. I see a few paths.

1. Instruments accept a non-integer value
2. Instruments default to 1 for a blank string
3. Instruments throw an error if a blank string is passed.

  1) Instruments
       Counter
         should add valid INT values:

      AssertionError [ERR_ASSERTION]: unit not equals: <no-message>
      + expected - actual

      +1

      at assertPartialDeepStrictEqual (test/util.ts:134:12)
      at assertMetricData (test/util.ts:79:5)
      at validateExport (test/Instruments.test.ts:560:19)
      at Context.<anonymous> (test/Instruments.test.ts:63:7)

  2) Instruments
       UpDownCounter
         should add INT values:

      AssertionError [ERR_ASSERTION]: unit not equals: <no-message>
      + expected - actual

      +1

      at assertPartialDeepStrictEqual (test/util.ts:134:12)
      at assertMetricData (test/util.ts:79:5)
      at validateExport (test/Instruments.test.ts:560:19)
      at Context.<anonymous> (test/Instruments.test.ts:179:7)

  3) Instruments
       Histogram
         should record INT values:

      AssertionError [ERR_ASSERTION]: unit not equals: <no-message>
      + expected - actual

      +1

      at assertPartialDeepStrictEqual (test/util.ts:134:12)
      at assertMetricData (test/util.ts:79:5)
      at validateExport (test/Instruments.test.ts:560:19)
      at Context.<anonymous> (test/Instruments.test.ts:264:7)

[codecov]

Codecov Report

Merging #2983 (4a3fd25) into main (7149f6c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2983   +/-   ##
=======================================
  Coverage   92.71%   92.71%           
=======================================
  Files         183      183           
  Lines        6134     6134           
  Branches     1299     1299           
=======================================
  Hits         5687     5687           
  Misses        447      447           

Impacted Files	Coverage Δ
...metry-sdk-metrics-base/src/InstrumentDescriptor.ts	100.00% <100.00%> (ø)

[pichlermarc]

There is a downstream behavior here that will need resolved before merging. The MetricOptions interface expects any string, including a default of '', but the additional tests expect an integer value. I could use some guidance as what the behavior/update should be. I see a few paths.

1. Instruments accept a non-integer value

2. Instruments default to 1 for a blank string

3. Instruments throw an error if a blank string is passed.

Hmm I'm not sure I understand correctly. As far as I can tell the reason these tests are failing are because they expect a unit string of '1' where there is none. 🙂

You can, in that case, just change the expected unit to be '' if it is not specified on instrument creation. I suspect that this might also come up in other tests that relied on '1' being the default (like the exporter tests), you can do the same there. 🙂

[dyladan]

Can you mark this as a draft until it is complete and has passing tests?

[andyfleming]

I don't think there's an option to switch a PR back to draft without closing and reopening a new one.

I've pushed an update that changes the expectation for unit values. (Tests should pass, but waiting for CI to run).

[pichlermarc]

You might still have to replace the expected unit: '1' with unit: '' in these files of the exporter tests:

• experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts
• experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts
• experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts

Then this PR should be ready to go 🙂
Thanks for taking the time to work on this! 🙂

[dyladan]

Please add a changelog entry in experimental/CHANGELOG.md