feat(sdk-metrics): deprecate MeterProvider.addMetricReader() in favor of 'readers' constructor option #4427
Conversation
…rovider.addMetricReader()
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4427 +/- ##
==========================================
+ Coverage 92.19% 92.21% +0.01%
==========================================
Files 336 336
Lines 9511 9515 +4
Branches 2016 2017 +1
==========================================
+ Hits 8769 8774 +5
+ Misses 742 741 -1
|
|
LGTM |
|
|
||
| if (options?.readers != null && options.readers.length > 0) { | ||
| for (const metricReader of options.readers) { | ||
| this.addMetricReader(metricReader); |
There was a problem hiding this comment.
What's the intention for addMetricReader method?
- to change its access to
private? - to delete it
I'm inclined to completely remove it to avoid people using the private method anyway (because the method would be exposed in the JS class instances). If that's the case I'd suggest to add here the method logic so it will be easier to remove the method in the future
There was a problem hiding this comment.
To avoid breakage, it will be removed in the SDK v2.0 (#4419).
There was a problem hiding this comment.
Yep, it will be removed. I've kept the code in addMetricReader() for now to avoid duplication.
| @@ -261,8 +261,9 @@ describe('PrometheusExporter', () => { | |||
|
|
|||
| beforeEach(done => { | |||
| exporter = new PrometheusExporter({}, () => { | |||
| meterProvider = new MeterProvider(); | |||
| meterProvider.addMetricReader(exporter); | |||
| meterProvider = new MeterProvider({ | |||
There was a problem hiding this comment.
This looks weird to me that we have to create a MeterProvider in the callback of a PrometheusExporter.
I submitted a follow-up at #4431.
Which problem is this PR solving?
Currently a MetricReader is added to a MeterProvider via addMetricReader(). As Meters (and through it instruments) can be created before adding a MetricReader. This gives the false impression that the order of calls does not matter. This PR deprecates the addMetricReader() method in favor of a constructor option as it is the case for most SDKs:
An alternative would be to allow adding a MetricReader like in the current interface, but re-configuring all instruments, their aggregations, assocaited views and their temporality. This however, is not required by the spec and would drastically increase complexity of an already very complex Metrics SDK.
References #4112, open-telemetry/opentelemetry-js-contrib#1900
Removal of
addMetricReaderfor 2.0: #4419Type of change
How Has This Been Tested?