Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly migrates to OpenTelemetry SDK 2.0 (updating APIs like Resource → resourceFromAttributes, parentSpanId → parentSpanContext?.spanId, instrumentationLibrary → instrumentationScope, and getEnv/getEnvWithoutDefaults → getStringFromEnv), but introduces a few issues: a bug where spanProcessors: [undefined] is passed to NodeTracerProvider in the OneAgent path, misaligned indentation left over from the removed guard block in metrics, two typos, and an unguarded setGlobalLoggerProvider that could overwrite a provider set by another module.
PR Bot Information
Version: 1.26.5
- Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
0ff37167-2947-4247-8ed4-2d99b2aa4751
| // TODO: CALM may have initialized a global provider already | ||
|
|
||
| resource = resourceFromAttributes({}).merge(resource).merge(getDynatraceMetadata()) | ||
| const tracerProvider = new NodeTracerProvider({ resource, spanProcessors: [processor], sampler: _getSampler() }) |
There was a problem hiding this comment.
Bug: When via_one_agent is true, processor remains undefined, and spanProcessors: [processor] passes [undefined] to NodeTracerProvider. This would likely cause a runtime error or silent failure when the OneAgent path is taken.
| const tracerProvider = new NodeTracerProvider({ resource, spanProcessors: [processor], sampler: _getSampler() }) | |
| resource = resourceFromAttributes({}).merge(resource).merge(getDynatraceMetadata()) | |
| const tracerProvider = new NodeTracerProvider({ resource, spanProcessors: processor ? [processor] : [], sampler: _getSampler() }) | |
| tracerProvider.register({ propagator: _getPropagator() }) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const dtmetadata = getDynatraceMetadata(); | ||
| resource = resourceFromAttributes({}).merge(resource).merge(dtmetadata); | ||
| // unfortunately, we have to pass views to the MeterProvider constructor | ||
| // something like meterProvider.addView() would be a lot nicer for locality | ||
| let views = [] | ||
| let views = []; | ||
| if (process.env.HOST_METRICS_RETAIN_SYSTEM) { | ||
| // nothing to do | ||
| } else { | ||
| views.push( | ||
| new View({ | ||
| meterName: '@cap-js/telemetry:host-metrics', | ||
| instrumentName: 'system.*', | ||
| aggregation: new DropAggregation() | ||
| }) | ||
| ) | ||
| views.push({ | ||
| meterName: "@cap-js/telemetry:host-metrics", | ||
| instrumentName: "system.*", | ||
| aggregation: { | ||
| type: AggregationType.DROP, | ||
| }, | ||
| }); | ||
| } | ||
| meterProvider = new MeterProvider({ resource, views }) | ||
| metrics.setGlobalMeterProvider(meterProvider) | ||
| } else { | ||
| LOG._warn && LOG.warn('MeterProvider already initialized by a different module. It will be used as is.') | ||
| } | ||
|
|
||
| const metricsConfig = cds.env.requires.telemetry.metrics.config | ||
| const exporter = _getExporter() | ||
| // push vs. pull | ||
| if (typeof exporter.export === 'function') { | ||
| const metricReader = new PeriodicExportingMetricReader({ ...metricsConfig, exporter }) | ||
| meterProvider.addMetricReader(metricReader) | ||
| } else { | ||
| meterProvider.addMetricReader(exporter) | ||
| } | ||
| // TODO: CALM may have initialized a global provider already | ||
|
|
||
| const meterProvider = new MeterProvider({ resource, readers: [exporter], views }); | ||
| metrics.setGlobalMeterProvider(meterProvider); |
There was a problem hiding this comment.
Logic Error: The indentation here is mismatched — lines 100–120 are indented as if inside a block, but they are actually at the top level of the module.exports function. While JS doesn't care about indentation, the metricsConfig variable (line 91) and exporter (line 92) are also at the outer level, making it clear the inner block was accidentally left indented. This is harmless but was likely a leftover from the removed if (meterProvider.constructor.name === 'NoopMeterProvider') block.
| const dtmetadata = getDynatraceMetadata(); | |
| resource = resourceFromAttributes({}).merge(resource).merge(dtmetadata); | |
| // unfortunately, we have to pass views to the MeterProvider constructor | |
| // something like meterProvider.addView() would be a lot nicer for locality | |
| let views = [] | |
| let views = []; | |
| if (process.env.HOST_METRICS_RETAIN_SYSTEM) { | |
| // nothing to do | |
| } else { | |
| views.push( | |
| new View({ | |
| meterName: '@cap-js/telemetry:host-metrics', | |
| instrumentName: 'system.*', | |
| aggregation: new DropAggregation() | |
| }) | |
| ) | |
| views.push({ | |
| meterName: "@cap-js/telemetry:host-metrics", | |
| instrumentName: "system.*", | |
| aggregation: { | |
| type: AggregationType.DROP, | |
| }, | |
| }); | |
| } | |
| meterProvider = new MeterProvider({ resource, views }) | |
| metrics.setGlobalMeterProvider(meterProvider) | |
| } else { | |
| LOG._warn && LOG.warn('MeterProvider already initialized by a different module. It will be used as is.') | |
| } | |
| const metricsConfig = cds.env.requires.telemetry.metrics.config | |
| const exporter = _getExporter() | |
| // push vs. pull | |
| if (typeof exporter.export === 'function') { | |
| const metricReader = new PeriodicExportingMetricReader({ ...metricsConfig, exporter }) | |
| meterProvider.addMetricReader(metricReader) | |
| } else { | |
| meterProvider.addMetricReader(exporter) | |
| } | |
| // TODO: CALM may have initialized a global provider already | |
| const meterProvider = new MeterProvider({ resource, readers: [exporter], views }); | |
| metrics.setGlobalMeterProvider(meterProvider); | |
| const dtmetadata = getDynatraceMetadata() | |
| resource = resourceFromAttributes({}).merge(resource).merge(dtmetadata) | |
| // unfortunately, we have to pass views to the MeterProvider constructor | |
| // something like meterProvider.addView() would be a lot nicer for locality | |
| let views = [] | |
| if (process.env.HOST_METRICS_RETAIN_SYSTEM) { | |
| // nothing to do | |
| } else { | |
| views.push({ | |
| meterName: "@cap-js/telemetry:host-metrics", | |
| instrumentName: "system.*", | |
| aggregation: { | |
| type: AggregationType.DROP, | |
| }, | |
| }) | |
| } | |
| // TODO: CALM may have initialized a global provider already | |
| const meterProvider = new MeterProvider({ resource, readers: [exporter], views }) | |
| metrics.setGlobalMeterProvider(meterProvider) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const config = { ...(metricsExporter.config || {}) } | ||
| config.temporalityPreference ??= AggregationTemporality.DELTA | ||
|
|
||
| // Augment configruation depending on 'kind' of telementry |
There was a problem hiding this comment.
Typo: "configruation" → "configuration" and "telementry" → "telemetry".
| // Augment configruation depending on 'kind' of telementry | |
| // Augment configuration depending on 'kind' of telemetry |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| if (typeof exporter.export === 'function') { | ||
| // In case export is a function to be called by this runtime (push): | ||
| // > The exporter needs to be wrappeed thus, to set an export interval |
There was a problem hiding this comment.
Typo: "wrappeed" → "wrapped".
| // > The exporter needs to be wrappeed thus, to set an export interval | |
| // > The exporter needs to be wrapped thus, to set an export interval |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| // TODO: CALM may have initialized a global provider already | ||
|
|
||
| const loggerProvider = new LoggerProvider({ resource, processors: [logProcessor]}) | ||
| logs.setGlobalLoggerProvider(loggerProvider) |
There was a problem hiding this comment.
Logic Error: LoggerProvider was previously initialized only when no global provider existed. Now it unconditionally calls logs.setGlobalLoggerProvider(loggerProvider) without checking whether a provider has already been set. The TODO comment acknowledges this, but the removed guard was load-bearing — if CALM (or any other module) sets a global provider first, this will silently overwrite it.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
2 participants
Upgrade to OpenTelemetry SDK 2.0 and Switch to Vitest
Refactor
♻️ This PR upgrades all OpenTelemetry dependencies from SDK v1/v0.57 to SDK v2/v0.219, adapting the codebase to the breaking API changes introduced in OpenTelemetry SDK 2.0. The previously blocked warning about SDK 2.0 incompatibility has been removed now that the upgrade is complete.
Changes
package.json: Bumped all@opentelemetry/*dependencies to v2 (core, resources, sdk-metrics, sdk-trace-base, sdk-trace-node) and v0.219 (instrumentation packages), and updated exporter and host-metrics dev dependencies accordingly.cds-plugin.js: Removed the version-check guard that blocked usage of OpenTelemetry SDK 2.0, along with the_version_ofhelper function.README.md: Removed the warning about OpenTelemetry SDK 2.0 not being supported.lib/utils.js: Replacednew Resource(attrs)withresourceFromAttributes(attrs)and switched fromprocess.env.*togetStringFromEnv()for reading OTEL environment variables.lib/tracing/index.js: Replaced deprecatedgetEnv/getEnvWithoutDefaultswithgetStringFromEnv, replacednew Resource({})withresourceFromAttributes({}), and refactoredNodeTracerProviderinitialization to use the newspanProcessorsconstructor option instead ofaddSpanProcessor().lib/tracing/trace.js: Updated span property access frominstrumentationLibrary.nametoinstrumentationScope.nameto match the SDK 2.0 API.lib/metrics/index.js: Replaced deprecated APIs (new Resource,getEnv,getEnvWithoutDefaults,View,DropAggregation) with their SDK 2.0 equivalents (resourceFromAttributes,getStringFromEnv, object-based view config withAggregationType.DROP). RefactoredMeterProviderinitialization to usereadersconstructor option instead ofaddMetricReader().lib/logging/index.js: ReplacedgetEnv/getEnvWithoutDefaultswithgetStringFromEnv, and refactoredLoggerProviderinitialization to use theprocessorsconstructor option.lib/exporter/ConsoleSpanExporter.js: Updated span parent access fromspan.parentSpanIdtospan.parentSpanContext?.spanIdto match the SDK 2.0 API.lib/index.js: Added import ofgetStringFromEnvanddiagLogLevelFromStringfrom@opentelemetry/core..gitignore: Added.claude/to the ignore list.PR Bot Information
Version:
1.26.5anthropic--claude-4.6-sonnetpull_request.opened0ff37167-2947-4247-8ed4-2d99b2aa4751