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. |
trentm
left a comment
There was a problem hiding this comment.
I'll add to the JS SIG agenda for tomorrow for the side questions I have below.
Approving to remove my block on this, though I think the test case should be updated (see comment below).
| const { | ||
| OpenCensusMetricProducer, | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| } = require('@opentelemetry/shim-opencensus'); |
There was a problem hiding this comment.
This will be one of the very few conditional imports that we have in OTel JS.
FWIW, we currently do this for:
require(): lazy load of@grpc/grpc-jsinotlp-grpc-exporter-baseto avoid loading it before instrumentation-grpc is possibly in placeawait import(): for platform-specific code in packages/opentelemetry-resources/src/detectors/platform/node/machine-id/getMachineId.tsawait import(): lazy load ofhttporhttpsin experimental/packages/otlp-exporter-base/src/transport/http-exporter-transport.ts and experimental/packages/otlp-exporter-base/src/configuration/otlp-node-http-configuration.ts
I think having a dynamic import here is a better option than having a dep on shim-opencensus and always importing it.
Aside: I'm kinda curious if, given https://opentelemetry.io/blog/2023/sunsetting-opencensus/, we could eventually/soon/now remove opencensus support. https://opentelemetry.io/blog/2023/sunsetting-opencensus/#fn:3 mentions supporting the OpenCensus shims for a year. It has been more than a year since the shim-opencensus package was released.
@maryliag Do you happen to know?
Whereever we have Node.js-specific usage docs for declarative config, I think we should add a note that using producers: ["opencensus"] requires the user to separately install the @opentelemetry/shim-opencensus dep.
| }); | ||
| assert.ok(reader !== undefined); | ||
| await (reader as PeriodicExportingMetricReader).shutdown(); | ||
| }); |
There was a problem hiding this comment.
This test isn't actually testing that the shim producer is working. If I make this change:
diff --git a/experimental/packages/opentelemetry-sdk-node/src/utils.ts b/experimental/packages/opentelemetry-sdk-node/src/utils.ts
index d80eb9a36..9495a2d01 100644
--- a/experimental/packages/opentelemetry-sdk-node/src/utils.ts
+++ b/experimental/packages/opentelemetry-sdk-node/src/utils.ts
@@ -529,7 +529,7 @@ function getMetricProducersFromConfiguration(
const {
OpenCensusMetricProducer,
// eslint-disable-next-line @typescript-eslint/no-require-imports
- } = require('@opentelemetry/shim-opencensus');
+ } = require('@opentelemetry/shim-opencensusXXX');
result.push(new OpenCensusMetricProducer());
} catch {
diag.warn(the test case still passes.
I think this could cheat and look at reader._metricProducers[0].constructor.name matches "OpenCensusMetricProducer".
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