fix: more consumable span names#447
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>
DB inner spans now show SQL verb + table name instead of raw SQL text (e.g. '@cap-js/sqlite - prepare SELECT sap.capire.bookshop.Books'). CDS operation names are mapped to SQL verbs (READ→SELECT, CREATE→INSERT). Raw SQL remains in the db.query.text attribute per OTel convention. Cloud SDK spans now use 'METHOD /path' (query string stripped) by default, or 'METHOD' only when adjust_root_name is false — consistent with how the HTTP root span name is controlled by the same flag. Destination name removed from the span name; still available as sap.btp.destination attribute.
There was a problem hiding this comment.
The PR is a solid major version upgrade (v2.0.0) adding OpenTelemetry SDK 2.0 support, CALM integration, and improved span names. The main actionable issues found are: a potential undefined processor being passed to both the CALM delegate and the standalone NodeTracerProvider when Dynatrace OneAgent is active, a likely typo in the CI branch filter (2 instead of 2.x), a regression in Cloud SDK span naming when ADJUST_ROOT_NAME is false, and a minor double-load of package.json in the instrumentation auto-discovery logic.
PR Bot Information
Version: 1.26.11
- File Content Strategy: Full file content
- Correlation ID:
c836923f-8153-4916-a50c-553e77bc1f1c - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
|
|
||
| function _cloudSdkSpanName(destination, requestConfig) { | ||
| const method = requestConfig?.method || 'GET' | ||
| if (!ADJUST_ROOT_NAME) return method |
There was a problem hiding this comment.
Logic Error: When ADJUST_ROOT_NAME is false, _cloudSdkSpanName returns only the HTTP method (e.g. "GET") as the span name, dropping the destination name that was previously included in the old span name template. This makes CALM/non-adjusted spans far less identifiable than before.
| if (!ADJUST_ROOT_NAME) return method | |
| if (!ADJUST_ROOT_NAME) return `${destination?.name ? destination.name + ' ' : ''}${method}` | |
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
The augmentation block at trace.js was designed for the HANA-promisified
path where the DB span name arrives as bare "@cap-js/hana - <fn>" (no
SQL). For sqlite/pg/HANA-non-promisified, cds.js interpolates the SQL at
emission time ("@cap-js/sqlite - prepare INSERT INTO ..."), so the strict
"^@cap-js/\w+ - \w+$" regex never matched and raw SQL leaked into names.
Widen the match to also accept a trailing SQL segment (and cover stmt.*
function names with dots, which the old \w+ never matched). The augmented
name uses the impl+verb prefix from the regex capture and appends
"<VERB> <table>" from the attributes we already have. SQL remains in
db.query.text per the OTel semantic convention.
Also tighten the anti-SQL test assertion so it rejects raw SQL patterns
(column lists, function calls, INSERT INTO, etc.) without colliding with
the desired "prepare SELECT <table>" shape.
Fix the load-order bug in the test: destructuring cds.ql at module top
level captures references BEFORE cds.test() wires up the profile-specific
tracer. Use the globals cds.test() injects instead.
SummaryThe following content is AI-generated and provides a summary of the pull request: Fix: More Consumable Span Names for DB and Cloud SDK TracesBug Fix / Refactor🔧 Improved readability of span names for DB and Cloud SDK telemetry traces. Raw SQL and full URLs are no longer exposed in span names; structured, human-readable identifiers are used instead while preserving raw details in OTel-standard attributes. Changes
PR Bot InformationVersion:
|
There was a problem hiding this comment.
The PR improves span name readability but has two concrete issues: the silent rename of adjust_root_name → _append_url_path breaks existing user configurations without any migration path, and the new _cloudSdkSpanName helper drops destination.name from span names, removing the ability to distinguish spans across different named destinations in trace UIs.
PR Bot Information
Version: 1.26.14
- Correlation ID:
7da57180-76c7-11f1-9d5c-dc04f8dad96c - Event Trigger:
pull_request.ready_for_review - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet
| const wrap = require('./wrap') | ||
|
|
||
| const { _append_url_path } = cds.env.requires.telemetry.tracing | ||
| const APPEND_URL_PATH = _append_url_path && _append_url_path !== 'false' |
There was a problem hiding this comment.
Breaking Change: adjust_root_name was the previously documented config key; it has been silently renamed to _append_url_path with no backward-compatibility shim. Existing deployments that set adjust_root_name: true will silently get the old behavior (full URL in span name) after upgrading.
| const APPEND_URL_PATH = _append_url_path && _append_url_path !== 'false' | |
| const APPEND_URL_PATH = (_append_url_path ?? cds.env.requires.telemetry.tracing.adjust_root_name) | |
| const APPEND_URL_PATH_VAL = APPEND_URL_PATH && APPEND_URL_PATH !== 'false' |
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 (!APPEND_URL_PATH) return method | ||
| const url = destination.url || requestConfig?.url || '' | ||
| const path = url.replace(/^https?:\/\/[^/]+/, '').split('?')[0] | ||
| return `${method}${path ? ' ' + path : ''}` |
There was a problem hiding this comment.
Bug: destination.name was previously prepended to the span name (e.g., "MyDest GET /path") and is now silently dropped, making it impossible to distinguish spans for different named destinations with the same HTTP method and path. The outbound attribute carries the name, but span names are what appear in trace UI overviews.
| return `${method}${path ? ' ' + path : ''}` | |
| const prefix = destination?.name ? destination.name + ' ' : '' | |
| return `${prefix}${method}${path ? ' ' + path : ''}` |
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 { hrtime, _append_url_path, _truncate_span_name } = cds.env.requires.telemetry.tracing | ||
| const HRTIME = hrtime && hrtime !== 'false' | ||
| const ADJUST_ROOT_NAME = adjust_root_name && adjust_root_name !== 'false' | ||
| const APPEND_URL_PATH = _append_url_path && _append_url_path !== 'false' |
There was a problem hiding this comment.
Breaking Change: adjust_root_name is renamed to _append_url_path here as well with no fallback, silently breaking existing config. Consider reading both keys during a deprecation window.
| const APPEND_URL_PATH = _append_url_path && _append_url_path !== 'false' | |
| const APPEND_URL_PATH = (_append_url_path ?? cds.env.requires.telemetry.tracing.adjust_root_name) && (_append_url_path ?? cds.env.requires.telemetry.tracing.adjust_root_name) !== 'false' |
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
- [x] package.json - [x] package-lock.json - [x] release job - [x] check for unused vars (cf. #441 (comment)) - [x] rm cds < 9 checks - [x] #447 --------- Co-authored-by: sjvans <sjvans@users.noreply.github.com> Co-authored-by: Vitaly Kozyura <58591662+vkozyura@users.noreply.github.com> Co-authored-by: I548646 <paul.erlenwein@sap.com> Co-authored-by: Paul <paul.erlenwein@gmail.com> Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Summary
DB inner spans now show
VERB tableinstead of raw SQL in the span name (e.g.@cap-js/sqlite - prepare SELECT sap.capire.bookshop.Books). CDS operation names are mapped to SQL verbs (READ→SELECT,CREATE→INSERT). Raw SQL is preserved in thedb.query.textattribute per OTel convention.Cloud SDK spans now use
METHOD /path(query string stripped) by default, orMETHODonly when_append_url_pathisfalse— consistent with the HTTP root span name behaviour controlled by the same flag. Destination name removed from span name; still available assap.btp.destinationattribute.Before / After
@cap-js/sqlite - prepare SELECT json_insert('{}','$."createdAt"'…@cap-js/sqlite - prepare SELECT sap.capire.bookshop.Books@cap-js/sqlite - exec INSERT INTO sap_capire_bookshop_Books …@cap-js/sqlite - exec INSERT sap.capire.bookshop.Books@cap-js/sqlite - stmt.run UPDATE sap_capire_bookshop_Books …@cap-js/sqlite - stmt.run UPDATE sap.capire.bookshop.Books@cap-js/sqlite - exec DELETE FROM sap_capire_bookshop_Books …@cap-js/sqlite - exec DELETE sap.capire.bookshop.Books_append_url_path: true)S4HANA GET https://s4hana.example.com/sap/opu/…?$top=10GET /sap/opu/…_append_url_path: false)S4HANA GET https://…GET_append_url_path: true)GETGET /odata/v4/admin/BooksTest plan
test/tracing-span-names.test.jsadded — covers DB span name format (SELECT, INSERT) and Cloud SDK URL-free names