feat(sdk-metrics)!: Drop deprecated InstrumentDescriptor export

[chancancode]

Which problem is this PR solving?

Drop the @deprecated InstrumentDescriptor type export

Short description of the changes

Note that this public InstrumentDescriptor has always been just an alias of MetricDescriptor, and does not actually point to the internal type with the same name.

Separately:

• Refactor the internal InstrumentDescriptor type to extend from the public MetricDescriptor, adding only the advice field

• Move the InstrumentType enum into ./src/export/MetricData.ts as it is a public export, plus to avoid a circular dependency after the refactor (hence the big diff)

Fixes #5254 (comment) (cc @pichlermarc)

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Internal refactor

How Has This Been Tested?

• npm run compile
• npm run lint
• npm run test:*

Checklist:

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.63%. Comparing base (107637e) to head (befe9b5).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5266      +/-   ##
==========================================
- Coverage   94.64%   94.63%   -0.02%     
==========================================
  Files         323      323              
  Lines        8084     8083       -1     
  Branches     1643     1643              
==========================================
- Hits         7651     7649       -2     
- Misses        433      434       +1     

Files with missing lines	Coverage Δ
packages/sdk-metrics/src/InstrumentDescriptor.ts	100.00% <ø> (ø)
packages/sdk-metrics/src/Meter.ts	100.00% <100.00%> (ø)
...sdk-metrics/src/aggregator/ExponentialHistogram.ts	98.15% <ø> (-0.01%)	⬇️
packages/sdk-metrics/src/aggregator/Histogram.ts	92.20% <ø> (-0.10%)	⬇️
...ages/sdk-metrics/src/export/AggregationSelector.ts	100.00% <ø> (ø)
...es/sdk-metrics/src/export/ConsoleMetricExporter.ts	91.66% <ø> (ø)
...s/sdk-metrics/src/export/InMemoryMetricExporter.ts	100.00% <ø> (ø)
packages/sdk-metrics/src/export/MetricData.ts	100.00% <100.00%> (ø)
packages/sdk-metrics/src/export/MetricReader.ts	100.00% <ø> (ø)
.../sdk-metrics/src/state/MeterProviderSharedState.ts	100.00% <ø> (ø)
... and 4 more

... and 1 file with indirect coverage changes

[chancancode]

rebased and retargeted

[pichlermarc]

Thank you @chancancode for working on this.

In case you're interested in making some further changes, the next step after this PR would then be to move the deprecated type property from MetricDescriptor to the now internal InstrumentDescriptor - this would solve #3439 (edit: the change seems very mechanical at first but there might be some challenges along the way to make it fit properly) 🙂

[chancancode]

@pichlermarc Sure, happy to add that to my queue and give it a shot.

the next step after this PR would then be to move the deprecated type property from MetricDescriptor to the now internal InstrumentDescriptor...

Ah, maybe I could have left it in the same file then, do you want it moved back or leave it?

Additionally, to be clear, the enum itself is not deprecated and still remains a public export, and is still an independently useful concept/type/constants to have for other purposes, yea?

...the change seems very mechanical at first but there might be some challenges along the way to make it fit properly

You are right that moving stuff around is the easy part, taking a quick look reveals that:

1. This is used pretty extensively internal to the package. I'd have to look deeper to see if all of those can be safely/soundly changed to a InstrumentDescriptor.

2. This is still used in several places outside the package:

   • https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts#L213
   • https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-exporter-prometheus/src/PrometheusSerializer.ts#L226-L248
   • https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/shim-opencensus/src/metric-transform.ts#L42-L43

It is not immediately obvious to me what we expect them to do instead from the deprecation note.

[pichlermarc]

Additionally, to be clear, the enum itself is not deprecated and still remains a public export, and is still an independently useful concept/type/constants to have for other purposes, yea?

Yes, it's used for mapping Views to instruments, so we still need to have it exported. 🙂

This is used pretty extensively internal to the package. I'd have to look deeper to see if all of those can be safely/soundly changed to a InstrumentDescriptor.

I think any usages for types that are not exported can be safely changed to InstrumentDescriptor.

This is still used in several places outside the package:

Yes - for Prometheus, it uses the type to map to enforce a naming convention. There you can replace that check for the InstrumentType to check if

• metricData.dataPointType is DataPointType.SUM and
• metricData.isMonotonic is true

That's the type that by default maps to Counter instruments so that's actually the more robust way to enforce the naming convention, as one can re-map a Histogram, for instance, to a monotonic sum aggregation, since histograms don't allow negative values to be recorded.

So today, a re-mapped Histogram will not get the naming convention enforced and will end up with the wrong name in Prometheus. That's one of the issues we try to avoid users from making by removing the type from the data that's passed to the exporters (#3439).

For the OpenCencus shim, the type can be safely removed so that it is not part of the export anymore. That would also make the mapping obsolete.