[stash] improve tracing#450
Draft
sjvans wants to merge 19 commits into
Draft
Conversation
Adds HTTP semantic convention attributes to remote service spans by wrapping `fetchClient.executeHttpRequest`. Alternative considered: Using `@opentelemetry/instrumentation-undici` response hooks to propagate attributes from HTTP child spans. This approach was rejected as it requires an additional dependency and creates duplicate spans. --------- Co-authored-by: D050513 <sebastian.van.syckel@sap.com> Co-authored-by: sjvans <30337871+sjvans@users.noreply.github.com>
- [x] traces - [x] metrics - [x] logs - [x] #442 --------- Co-authored-by: I548646 <paul.erlenwein@sap.com> Co-authored-by: Paul <paul.erlenwein@gmail.com>
) # Improve `VCAP_SERVICES` Service Binding Resolution with Greedy Matchers ### Chore 🔧 Updated service instance matching for SAP Cloud Logging and Dynatrace to use greedy (array-based) VCAP matchers, extending the default matching without breaking existing behavior. Error messages and documentation have been updated to guide users toward the correct configuration. ### Changes * `package.json`: Updated `vcap` matching for `telemetry-to-dynatrace` and `telemetry-to-cloud-logging` from a single object to an array of matchers. Dynatrace now matches by `label: "dynatrace"` or `tag: "dynatrace"`; Cloud Logging matches by `label: "cloud-logging"` or `tag: "Cloud Logging"`. * `lib/utils.js`: Improved inline comments to clearly document the legacy compatibility behavior for `getCredsForDTAsUPS()` and `getCredsForCLSAsUPS()`. Updated the warning message for Cloud Logging user-provided services to recommend only the `"Cloud Logging"` tag (dropped `"cloud-logging"` from the recommendation). * `lib/logging/index.js`, `lib/metrics/index.js`, `lib/tracing/index.js`: Improved error messages when credentials are not found — now explicitly instruct users to ensure the bound service instance uses the correct tag (`"Cloud Logging"` or `"dynatrace"`). * `README.md`: Clarified that user-provided service instances for SAP Cloud Logging must use the tag `"Cloud Logging"` (removed reference to the legacy `cloud-logging` tag). Added a tip with the `cf update-user-provided-service` command and a link to CAP's service binding documentation. - [ ] 🔄 Regenerate and Update Summary <details> <summary>PR Bot Information</summary> **Version:** `1.26.5` - Correlation ID: `81bbd7c7-2290-4812-89e9-d178ac318c25` - Event Trigger: `issue_comment.edited` </details> --------- Co-authored-by: sjvans <30337871+sjvans@users.noreply.github.com>
# Fix: Workaround for `cds.cli.command` Bug with `build` Command ### Bug Fix 🐛 Added a workaround to prevent the telemetry plugin from loading when the `cds build` command is invoked. Due to a bug in `cds.cli.command`, the `build` command incorrectly resolves to an empty string (`''`), causing the plugin initialization check to pass unintentionally. ### Changes * `cds-plugin.js`: Added an early return guard that checks if `build` is present in `process.argv`. This ensures the plugin exits before proceeding with initialization when the `cds build` command is used, mirroring the existing workaround for `cds add`. - [ ] 🔄 Regenerate and Update Summary <details> <summary>PR Bot Information</summary> **Version:** `1.26.5` - Correlation ID: `4743ca7e-193a-4675-a529-8872b09ede9e` - Summary Prompt: [Default Prompt](https://github.tools.sap/intelligent-insights/i2-pull-request/blob/main/src/services/llm/prompts/summary_instructions_prompt.md) - LLM: `anthropic--claude-4.6-sonnet` - Output Template: [Default Template](https://github.tools.sap/intelligent-insights/i2-pull-request/blob/main/src/services/llm/prompts/summary_default_output_template.md) - Event Trigger: `pull_request.ready_for_review` - File Content Strategy: Full file content </details> --------- Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
# Chore: Upgrade to `cds^10` — Migrate Test Config to `.cdsrc.json` and CDS Profiles ### Refactor ♻️ Migrates test configuration from inline `process.env` overrides to a dedicated `.cdsrc.json` file and CDS CLI profiles, aligning with the `cds^10` approach for environment/profile-based configuration. Also includes test timing optimizations and minor bug fixes. ### Changes * `.github/workflows/ci.yml`: Commented out the `cds@8` downgrade step; updated comment to reference `cds@9`. * `.github/workflows/release.yml`: Added a `REVISIT` comment to remove the `better-sqlite3` install workaround with `cds^10`. * `.gitignore`: Removed `test/bookshop/.cdsrc.json` from the ignore list so it can be tracked in version control. * `test/bookshop/.cdsrc.json`: New file defining CDS profiles (`logging`, `metrics`, `metrics-outbox`, `metrics-outbox-disabled`, `tracing-attributes`, `persistent-outbox`, `without-outbox`) consolidating telemetry, messaging, and metrics settings previously scattered across test files. * `test/bookshop/package.json`: Removed `_outbox` and profile-specific `[metrics-outbox]`/`[metrics-outbox-disabled]` blocks (now in `.cdsrc.json`); added `_scheduling: true`. * `test/logging.test.js`: Removed inline `process.env` overrides for telemetry/log format; switched to `cds.test(..., '--profile', 'logging')` with a minimal remaining env override for `cls_custom_fields`. * `test/metrics.test.js`: Removed `exportIntervalMillis` env override; switched to `cds.test(..., '--profile', 'metrics')`. * `test/metrics-outbox.test.js` / `test/metrics-outbox-multitenant.test.js`: Reduced metric collection wait times from `300ms` to `150ms`; fixed retry loop bug where both loops checked `E1` instead of `E1`/`E2` respectively. * `test/tracing-attributes.test.js`: Removed inline exporter env var; switched to `cds.test(..., '--profile', 'tracing-attributes')`. * `test/tracing-messaging-persistent-outbox.test.js`: Renamed `CASE` to `persistent-outbox`; simplified env setup; marked `describe` block as `skip` pending vitest migration. * `test/tracing-messaging-without-outbox.test.js`: Renamed `CASE` to `without-outbox`; simplified env setup using `outboxed: false`. * `test/tracing-messaging-with-in-memory-outbox.test.js`: Removed this test file entirely. * `test/tracing-messaging.js`: Switched from `cds.test().in(...)` to `cds.test(..., '--profile', CASE)`. * `test/tracing.test.js`: Switched from `cds.test().in(...)` to `cds.test(...)`. - [ ] 🔄 Regenerate and Update Summary <details> <summary>PR Bot Information</summary> **Version:** `1.26.11` - Summary Prompt: [Default Prompt](https://github.tools.sap/intelligent-insights/i2-pull-request/blob/main/src/services/llm/prompts/summary_instructions_prompt.md) - Correlation ID: `14c5ae4d-4c97-4eff-a004-330c9fdcf2fe` - Event Trigger: `pull_request.edited` - File Content Strategy: Full file content - Output Template: [Default Template](https://github.tools.sap/intelligent-insights/i2-pull-request/blob/main/src/services/llm/prompts/summary_default_output_template.md) - LLM: `anthropic--claude-4.6-sonnet` </details>
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This PR is a significant major version bump (1.x → 2.0) that upgrades the entire OpenTelemetry stack to SDK 2.0, adds CALM integration, refactors span/metric/log provider initialization, and substantially improves test quality. The core logic changes are well-structured, but there are a few concrete bugs to address: the via_one_agent path passes undefined as a span processor (in both standalone and CALM branches), the CALM setup path silently skips instrumentation registration, and the CI workflow references a non-existent actions/checkout@v6.0.3 version that will break CI.
PR Bot Information
Version: 1.26.14
- Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- Correlation ID:
d1ff2bc0-76e7-11f1-96e3-e8ee43c0a9a4 - LLM:
anthropic--claude-4.6-sonnet
Comment on lines
+176
to
+180
| if (!resource) { | ||
| LOG.warn("@sap/xotel-agent-ext-js found, adding @cap-js/telemetry's span processor as delegate") | ||
| try { | ||
| const { getCompositeSpanProcessor } = require('@sap/xotel-agent-ext-js') | ||
| getCompositeSpanProcessor().addDelegate(processor) |
Contributor
There was a problem hiding this comment.
Bug: When via_one_agent is true, processor remains undefined and is passed to getCompositeSpanProcessor().addDelegate(undefined) in the CALM path — this will likely throw or silently register a broken delegate.
Suggested change
| if (!resource) { | |
| LOG.warn("@sap/xotel-agent-ext-js found, adding @cap-js/telemetry's span processor as delegate") | |
| try { | |
| const { getCompositeSpanProcessor } = require('@sap/xotel-agent-ext-js') | |
| getCompositeSpanProcessor().addDelegate(processor) | |
| if (!resource) { | |
| if (processor) { | |
| LOG.warn("@sap/xotel-agent-ext-js found, adding @cap-js/telemetry's span processor as delegate") | |
| try { | |
| const { getCompositeSpanProcessor } = require('@sap/xotel-agent-ext-js') | |
| getCompositeSpanProcessor().addDelegate(processor) | |
| } catch (error) { | |
| LOG.error('Failed to add span processor as delegate:', error) | |
| throw error | |
| } | |
| } | |
| return | |
| } |
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
| require('./cloud_sdk')() | ||
|
|
||
| resource = resourceFromAttributes({}).merge(resource).merge(getDynatraceMetadata()) | ||
| const tracerProvider = new NodeTracerProvider({ resource, spanProcessors: [processor], sampler: _getSampler() }) |
Contributor
There was a problem hiding this comment.
Bug: When via_one_agent is true in the standalone path, processor is undefined and passing it to new NodeTracerProvider({ spanProcessors: [undefined] }) will likely cause a runtime error or silently register a no-op processor.
Suggested change
| const tracerProvider = new NodeTracerProvider({ resource, spanProcessors: [processor], sampler: _getSampler() }) | |
| resource = resourceFromAttributes({}).merge(resource).merge(getDynatraceMetadata()) | |
| const spanProcessors = processor ? [processor] : [] | |
| const tracerProvider = new NodeTracerProvider({ resource, spanProcessors, sampler: _getSampler() }) | |
| tracerProvider.register({ propagator: _getPropagator() }) | |
| return tracerProvider |
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
Comment on lines
+113
to
+118
| function setup_with_calm() { | ||
| // setup tracing, metrics, and logging | ||
| tracing() | ||
| metrics() | ||
| if (cds.env.requires.telemetry.logging) logging() | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: setup_with_calm calls tracing(), metrics(), and logging() without registering instrumentations via registerInstrumentations. In setup_standalone, instrumentations are explicitly registered after the providers are set up. The CALM path silently skips all instrumentations (e.g., HttpInstrumentation, UndiciInstrumentation).
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 { | ||
| AggregationTemporality, | ||
| DropAggregation, | ||
| AggregationType, |
Contributor
There was a problem hiding this comment.
Bug: AggregationType is imported from @opentelemetry/sdk-metrics but never used anywhere in the file after the removal of DropAggregation — this is a stale import that should be removed.
Suggested change
| AggregationType, | |
| AggregationTemporality, | |
| MeterProvider, | |
| PeriodicExportingMetricReader |
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
| # cds-version: [10, 9] | ||
| cds-version: [9] | ||
| steps: | ||
| - uses: actions/checkout@v6.0.3 |
Contributor
There was a problem hiding this comment.
Bug: The actions/checkout@v6.0.3 reference is invalid — the latest major release is v4. This will cause the workflow to fail with a "not found" error.
Suggested change
| - uses: actions/checkout@v6.0.3 | |
| - uses: actions/checkout@v4 |
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade to OpenTelemetry SDK 2.0 and
@sap/cds^10(v2.0.0)New Feature / Refactor
✨ This is a major version bump (
1.x → 2.0.0) of@cap-js/telemetry, bringing full support for OpenTelemetry SDK 2.0,@sap/cds^10, and significantly improved tracing coverage — especially for CAP's persistent outbox/queue worker path and CALM (@sap/xotel-agent-ext-js) integration.Key highlights:
@opentelemetry/*dependencies to^2.x/ latest^0.219instrumentation packages. Removed the previous SDK 2.0 compatibility check/block.@opentelemetry/instrumentation-undiciadded as a default instrumentation.@opentelemetry/instrumentation-host-metricsreplaces deprecated@opentelemetry/host-metrics. Host metric collection now uses themetricGroupsconfig option to limit to['process.cpu', 'process.memory']by default.@sap/xotel-agent-ext-jsand add processors/readers/log processors as delegates to the CALM composite providers instead of initializing standalone providers.cds.tx(fn)wrapping: The telemetry plugin now wrapscds.Service.prototype.txso that transactional units of work (queue worker transactions) appear as proper root spans under a singlecds.spawn - run taskroot, instead of each top-level call becoming an orphan root.ConsoleSpanExporter: Indent state is no longer mutated onReadableSpaninstances; it is threaded through the recursive_list2treetraversal. Span parent lookup now usesparentSpanContext?.spanId(OTel SDK 2.0 API). Time formatting now usestoFixed(2)(rounds) instead of string truncation.getEnvWithoutDefaults/getEnvreplaced throughout withgetStringFromEnv(SDK 2.0 API).new Resource()replaced withresourceFromAttributes()(SDK 2.0 API).@sap/cds^8peer dependency support.Changes
package.json/CHANGELOG.md: Bump to v2.0.0; update all OTel dependencies to SDK 2.0 versions; addundiciinstrumentation; update peer dependency to@sap/cds^10; expandvcapbinding config to match both label and tag for Dynatrace and Cloud Logging.cds-plugin.js: Added workaround forcds buildcommand triggering telemetry; removed the SDK 2.0 compatibility version check (now supported).lib/index.js: Split intosetup_standaloneandsetup_with_calmpaths; added automatic detection and configuration of@opentelemetry/instrumentation-host-metricsviametricGroups.lib/tracing/index.js: Refactored to support CALM delegate pattern; moved SAP Passport clearing; updated toresourceFromAttributesandgetStringFromEnv; improved error messages.lib/tracing/cds.js: Addedcds.Service.prototype.txwrapping to properly scope queue-worker transactions as child spans under thecds.spawn - run taskroot.lib/tracing/trace.js: UpdatedinstrumentationScope.namecheck (wasinstrumentationLibrary.name).lib/metrics/index.js: Refactored to support CALM delegate pattern; removedView/DropAggregation(superseded bymetricGroups); updated toresourceFromAttributes.lib/metrics/host.js: Removed (host metrics now handled viainstrumentation-host-metricsconfig inlib/index.js).lib/logging/index.js: Refactored provider initialization order (log interception registered before provider setup); added CALM delegate support; updated togetStringFromEnv.lib/exporter/ConsoleSpanExporter.js: Fixed span mutation (indent carried in traversal, not on span object); fixedparentSpanContext?.spanIdusage; improved time formatting; updated parent detection for root spans.lib/exporter/ConsoleMetricExporter.js: Updated scope name match for@opentelemetry/instrumentation-host-metrics.lib/utils.js: Updated toresourceFromAttributes,getStringFromEnv; improved legacy compatibility comments for Dynatrace and Cloud Logging UPS bindings.README.md: Removed SDK 2.0 warning banner; updated host metrics docs; addedundiciinstrumentation to defaults; added Cloud Logging UPS tag guidance.MyInMemorySpanExporterthat collectsReadableSpanobjects directly. Added new test files for persistent outbox, scheduled tasks, outboxed batch fan-out, inbox/outbox combinations, and aConsoleSpanExporterunit test. Replaced fixedwait(N)sleeps in metrics tests withexpectEventually()polling helpers. Moved profile configs from inlineprocess.envoverrides to.cdsrc.jsonprofiles.PR Bot Information
Version:
1.26.14pull_request.openedanthropic--claude-4.6-sonnetd1ff2bc0-76e7-11f1-96e3-e8ee43c0a9a4