fix(sdk-metrics): deprecate instrument type in InstrumentDescriptor

[pichlermarc]

Which problem is this PR solving?

InstrumentDescriptor currently accidentally passes the original instrument type through to the exporter. In #3439 we agreed to deprecate it and rename InstrumentDescriptor to Descriptor.

This PR

• deprecates InstrumentDescriptor and it's type field
  • introduces a Descriptor base interface as replacement, which has no type field.
  • makes InstrumentDescriptor extend Descriptor
  • InstrumentDescriptor is still passed to Exporters via MetricData as the interface needs to stay stable
• introduces an internal type MetricDescriptor to replace InstrumentDescriptor internally. This type:
  • extends Descriptor
  • renames type -> originalInstrumentType
  • keeps the originalInstrumentType around for internal use, as it is necessary to
    • resolve view conflicts
    • apply view selectors to instruments
    • determine default aggregations
    • determine if sum aggregations are monotonic/non-monotonic
    • determine if sum should be included when creating a HistogramDataPoint
  • is converted to InstrumentDescriptor before export.

I'm not aiming to change any of the exporter packages that use the InstrumentDescriptor type in this PR, but keep the changes local to the sdk-metrics package. Changing the exporter packages would blow up the diff significantly and make it hard to determine if the changes are truly non-breaking.

I would be very happy about some feedback regarding the JavaScript/TypeScript-iness of the PR as well as naming of the types, as I'm not entirely confident about those points at the moment. 🙂

Fixes #3439

Type of change

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

How Has This Been Tested?

• Existing Unit tests
• Adapted Unit tests

Checklist:

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

[codecov]

Codecov Report

Merging #3520 (bf9c399) into main (81ee1df) will increase coverage by 0.39%.
The diff coverage is 100.00%.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3520      +/-   ##
==========================================
+ Coverage   93.30%   93.69%   +0.39%     
==========================================
  Files         235      277      +42     
  Lines        5986     8449    +2463     
  Branches     1154     1753     +599     
==========================================
+ Hits         5585     7916    +2331     
- Misses        401      533     +132     

Impacted Files	Coverage Δ
packages/sdk-metrics/src/aggregator/Drop.ts	100.00% <ø> (ø)
packages/sdk-metrics/src/aggregator/types.ts	100.00% <ø> (ø)
...ages/sdk-metrics/src/export/AggregationSelector.ts	100.00% <ø> (ø)
...es/sdk-metrics/src/export/ConsoleMetricExporter.ts	83.33% <ø> (ø)
...s/sdk-metrics/src/export/InMemoryMetricExporter.ts	91.30% <ø> (ø)
packages/sdk-metrics/src/export/MetricData.ts	100.00% <ø> (ø)
packages/sdk-metrics/src/export/MetricReader.ts	100.00% <ø> (ø)
packages/sdk-metrics/src/state/MetricCollector.ts	100.00% <ø> (ø)
...s/sdk-metrics/src/state/TemporalMetricProcessor.ts	98.21% <ø> (ø)
...ackages/sdk-metrics/src/view/InstrumentSelector.ts	100.00% <ø> (ø)
... and 17 more

... and 76 files with indirect coverage changes

[aabmass]

Will this cause deprecation warnings (depending on linter/ts setup) for anyone importing InstrumentDescriptor e.g. in an exporter? If so, is there a way to fix the deprecation warnings by using a different type/field in exposition format?

[pichlermarc]

Will this cause deprecation warnings (depending on linter/ts setup) for anyone importing InstrumentDescriptor e.g. in an exporter? If so, is there a way to fix the deprecation warnings by using a different type/field in exposition format?

It should be possible to just use the newly introduced base type Descriptor instead, which should get rid of any warnings (and also the type field). 🙂

[legendecas]

Overall LGTM % nits.

[dyladan]

LGTM but there are conflicts. This can be merged when they're fixed

[pichlermarc]

Failing tests are unrelated to this PR, see #3700

[dyladan]

@pichlermarc tests are still failing and the linked PR is merged

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[pichlermarc]

There's a better impelmentation of this in #3876 and we've planned removing it from the public interface in SDK 2.0 (#3439). Closing this in favor of #3876