Conversation
![hyperspace-pr-bot[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
|
Superseded by #474, which migrates jest→vitest properly (clean exit via forked-pool teardown, HANA subset logic ported, spy/fn converted). This stash's SDK-2.0 half already shipped in v2.0.0 and it referenced since-deleted test files. |
Supersedes the #437 stash. Migrates the test runner jest → vitest. ## Why it's a clean win Vitest with `pool: 'forks'` + per-file isolation tears each test file's child process down when the file finishes — so the OTLP exporter's lingering handles die with the child, and **the suite exits cleanly with no `--forceExit`**. This is the same open-handle class that hangs jest (see #472/#466). Verified: exit 0, ~8s, no hang across repeated runs. ## Config (`vitest.config.mjs`) - `globals: true` — `describe/test/beforeEach/...` stay available, test bodies unchanged. - `pool: 'forks'`, `isolate: true`, `teardownTimeout: 1000` — the clean-exit mechanism (documented inline). - Ports the old `jest.config.js` HANA logic faithfully: default `testTimeout: 42000`; under `CI && HANA_DRIVER` → `include` restricted to `tracing-attributes` + `passport`, timeout ×10, `cds_requires_telemetry_tracing` set when `HANA_PROM`. Verified the subset selection. ## Changes - `package.json`: `test` → `vitest run --silent`; jest removed, vitest added. `jest.config.js` deleted. - 8 `jest.spyOn`/`jest.fn` → `vi.spyOn`/`vi.fn`. - 5 `beforeAll(done => …)` hooks → promise-returning (vitest treats a hook arg as a fixture). - `eslint.config.mjs`: test-files override declaring `vi` global. Lint clean. - Lockfile regenerated against public npm (0 internal-registry URLs). ##⚠️ Behavioral note — HTTP instrumentation disabled in the test app Under jest, OTel's `require-in-the-middle` http patching was **silently broken** by jest's module sandbox, so incoming HTTP SERVER spans never existed in tests (the existing `xtest` skips document this). Under vitest (real `require`) the instrumentation works and reparents trace trees, breaking several assertions. To keep this migration **behavior-neutral**, `test/bookshop/package.json` now sets `disableIncomingRequestInstrumentation` + `disableOutgoingRequestInstrumentation` on the http instrumentation — reproducing jest's effective environment. Consequence: the HTTP-instrumentation path stays untested (same blind spot as jest, now explicit config rather than an accident). **Follow-up issue filed to enable it and assert on the real incoming spans.** The tracing-attributes client-span assertions still pass because those spans come via undici / cloud-sdk, not instrumentation-http. ## Coordination Parallel PR #473 (prettier) touches `package.json` (additive) + lockfile. Lockfile will conflict — whichever merges second rebases. #473 excludes `jest.config.js` from formatting (this PR deletes it). ## Verified `npm run test` 53 pass / 14 skip, exit 0, ~8s, clean exit ×3 · `npm run lint` clean · HANA subset selection confirmed.
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