feat(sdk-metrics-base): meter identity

[legendecas]

Which problem is this PR solving?

As spec defines:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter
Meters are identified by all of these fields. When more than one Meter of the same name, version, and schema_url is created, it is unspecified whether or under which conditions the same or different Meter instances are returned. The term identical applied to Meters describes instances where all identifying fields are equal. The term distinct applied to Meters describes instances where at least one identifying field has a different value.

Also said:

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#opentelemetry-protocol-data-model-producer-recommendations
Producers SHOULD prevent the presence of multiple Metric identities for a given name with the same Resource and Scope attributes. Producers are expected to aggregate data for identical Metric objects as a basic feature, so the appearance of multiple Metric, considered a "semantic error", generally requires duplicate conflicting instrument registration to have occurred somewhere.

So we need to not create a new MeterSharedState when a given name+version+schemaUrl pair is already created in the MeterProvider.

Fixes Partial #2593

Short description of the changes

• MeterProviderSharedState stores a map of MeterSharedState, keyed with the name+version+schemaUrl pair.

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• MeterProvider.getMeter

Checklist:

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

[codecov]

Codecov Report

Merging #2901 (9e560b7) into main (ed2f033) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 9e560b7 differs from pull request most recent head 3508a7f. Consider uploading reports for the commit 3508a7f to get more accurate results

@@            Coverage Diff             @@
##             main    #2901      +/-   ##
==========================================
+ Coverage   92.48%   92.51%   +0.03%     
==========================================
  Files         151      151              
  Lines        4841     4848       +7     
  Branches     1022     1024       +2     
==========================================
+ Hits         4477     4485       +8     
+ Misses        364      363       -1     

Impacted Files	Coverage Δ
...ckages/opentelemetry-sdk-metrics-base/src/Meter.ts	100.00% <0.00%> (ø)
...ckages/opentelemetry-sdk-metrics-base/src/utils.ts	100.00% <0.00%> (ø)
...pentelemetry-sdk-metrics-base/src/MeterProvider.ts	100.00% <0.00%> (ø)
...etry-sdk-metrics-base/src/state/MetricCollector.ts	100.00% <0.00%> (ø)
...try-sdk-metrics-base/src/state/MeterSharedState.ts	100.00% <0.00%> (ø)
...metrics-base/src/state/MeterProviderSharedState.ts	100.00% <0.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts	93.75% <0.00%> (+6.25%)	⬆️

[seemk]

Looks good 👍

[seemk]

Random trivia: Array.from(states.values(), state => state.collect(this, collectionTime)); would be shorter, but benched it for fun and found that Array.from(states.values()).map(state => state.collect(this, collectionTime)); is ~4x faster on Node.js 🤷‍♂️

[legendecas]

@dyladan updated, please take a look again :)

[dyladan]

Looks good but I think my second suggestion for changelog wording was actually more accurate than the first. The implementation looks fine to me though.

[legendecas]

@dyladan now that the implementation does return the same meter object on identical input, the changelog entry should be fine I think? As the "shared internal state" is not exposed to end-users, I'd find your first suggestion is good though.

[dyladan]

@dyladan now that the implementation does return the same meter object on identical input, the changelog entry should be fine I think? As the "shared internal state" is not exposed to end-users, I'd find your first suggestion is good though.

I missed the meter trailing on line 119. Looks good to me.

[legendecas]

thank you for reviewing :)