feat(sdk-node): wire up metric producers from declarative config#6712
feat(sdk-node): wire up metric producers from declarative config#6712MikeGoldsmith wants to merge 6 commits into
Conversation
Pass the `producers` config to PeriodicExportingMetricReader when creating metric readers from declarative config. Currently only opencensus is defined in the config schema. The implementation dynamically requires @opentelemetry/shim-opencensus at runtime to avoid adding it as a hard dependency — users who configure opencensus producers must install the shim package separately. A warning is emitted if the package is not found. Closes open-telemetry#6424 Assisted-by: Claude Opus 4.6
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6712 +/- ##
==========================================
- Coverage 94.86% 94.85% -0.02%
==========================================
Files 376 377 +1
Lines 12717 12765 +48
Branches 2884 2891 +7
==========================================
+ Hits 12064 12108 +44
- Misses 653 657 +4
🚀 New features to boost your workflow:
|
Assisted-by: Claude Opus 4.6
trentm
left a comment
There was a problem hiding this comment.
Just a nit. Not merging yet, to give Mike a chance to address the nit.
Export MetricProducer as MetricProducerConfigModel from the configuration package and use it as the function parameter type instead of PeriodicMetricReaderConfigModel['producers']. This decouples the function from PeriodicMetricReader so it can be reused for PullMetricReader later. Assisted-by: Claude Opus 4.6
|
@MikeGoldsmith I broke test/start.test.ts in a mismerge (using the GitHub UI to resolve a conflict). diff --git a/experimental/packages/opentelemetry-sdk-node/test/start.test.ts b/experimental/packages/opentelemetry-sdk-node/test/start.test.ts
index ede4ccd37..8f6021b06 100644
--- a/experimental/packages/opentelemetry-sdk-node/test/start.test.ts
+++ b/experimental/packages/opentelemetry-sdk-node/test/start.test.ts
@@ -986,6 +986,8 @@ describe('startNodeSDK', function () {
)
);
await (reader as PeriodicExportingMetricReader).shutdown();
+ });
+
it('should warn when exporter timeout is 0', async () => {
const warnSpy = Sinon.spy(diag, 'warn');
const exporter = getSpanExporter({Can you please apply this? which I think is (always/often?) the case with corporate forks. (Aside: I find it inconsistent that I cannot push to this fork, but if there is a merge conflict, the GitHub UI allows me to push a commit along with any edits I've made to that file.) |
There was a problem hiding this comment.
See the fix from #6712 (comment) (comment just above) because I mucked up a merge conflict.
Assisted-by: Claude Opus 4.6
Head branch was pushed to by a user without write access
|
Thanks @trentm - I've applied the fix, should be good now. |
Which problem is this PR solving?
The declarative config schema supports
producerson periodic metric readers (currently onlyopencensus), butgetMeterReadersFromConfigurationignores them — producers are never passed toPeriodicExportingMetricReader.Short description of the changes
getMetricProducersFromConfigurationthat resolves config model producers to SDKMetricProducerinstancesopencensus: dynamicallyrequire('@opentelemetry/shim-opencensus')to avoid a hard dependency — users who configure opencensus producers must install the shim package separatelymetricProducerstoPeriodicExportingMetricReaderfor both OTLP and console exportersType of change
How Has This Been Tested?
189 sdk-node tests pass. Full lint passes.
Checklist:
Closes #6424