feat(sdk-metrics-base): meter registration #2666
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2666 +/- ##
==========================================
+ Coverage 92.72% 93.05% +0.33%
==========================================
Files 155 155
Lines 5372 5373 +1
Branches 1133 1133
==========================================
+ Hits 4981 5000 +19
+ Misses 391 373 -18
|
| constructor(options: MeterProviderOptions) { | ||
| this._sharedState = new MeterProviderSharedState(options.resource ?? Resource.empty()); | ||
| constructor(options?: MeterProviderOptions) { | ||
| this._sharedState = new MeterProviderSharedState(options?.resource ?? Resource.empty()); | ||
| } | ||
|
|
||
| getMeter(name: string, version = '', options: metrics.MeterOptions = {}): metrics.Meter { |
There was a problem hiding this comment.
If the schema URL is part of the identity, shouldn't it be a top-level property and not buried in options?
There was a problem hiding this comment.
Actually, there is only one requirement for meter creation:
Implementations MUST NOT require users to repeatedly obtain a Meter again with the same name+version+schema_url to pick up configuration changes. This can be achieved either by allowing to work with an outdated configuration or by ensuring that new configuration applies also to previously returned Meters.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#get-a-meter
It also suggests that:
It is unspecified whether or under which conditions the same or different Meter instances are returned from this function.
So, as long as we propagate the configurations to all meters created without another getMeter call, it should be spec-compliant.
As we are using the MeterProviderSharedState pattern to share the meter provider configurations, I'd believe we've already met the bar that the created meters can automatically get configuration updates without the need to call getMeter again. So it seems that we actually don't have to return an identical meter for each getMeter call.
There was a problem hiding this comment.
Another problem is that the spec is said instrument names are unique in a meter:
Meter implementations MUST return an error when multiple Instruments are registered under the same Meter instance using the same name.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument
So, if we are going to return a new meter instance for every getMeter, how do we define "same Meter instance" in the above context?
There was a problem hiding this comment.
Submitted a spec issue for this: open-telemetry/opentelemetry-specification#2226
# Conflicts: # experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts # packages/exporter-trace-otlp-grpc/protos # packages/exporter-trace-otlp-proto/protos
legendecas
changed the title
|
@dyladan I updated the PR to just register the created meters to MeterProviderSharedState so that we can wire up basic API usage like collecting metrics with MetricReaders. I'll wait for the meter identity spec issue to be resolved before submitting another PR for it. |
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts
Which problem is this PR solving?
Register created meters in MeterProviderSharedState so that their metric instruments can be collected.
Extracted from #2636
Related: #2593
Type of change
How Has This Been Tested?
Checklist: