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 diff is small and the full file is now visible. The changes look straightforward — re-enabling the CDS v10 matrix entry and its corresponding conditional install step. No issues to flag beyond what's already visible.
The PR is in a work-in-progress state (the description checklist still has open items: package.json, release job, and the unused-vars check). The CI workflow changes themselves are clean and correct.
PR Bot Information
Version: 1.26.11
- Correlation ID:
2ce8afeb-55a2-48e1-bcb0-a834798ae650 - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
There was a problem hiding this comment.
All the changes are consistent and correct. The _ and req parameters were removed because they were unused in the callback body — the function only uses this. Everything checks out across all the modified files.
The PR is clean and well-structured. All changes are consistent with upgrading to CDS v10 support — removing obsolete CDS version guards, dropping unused imports, and updating dependency ranges. No issues to flag.
PR Bot Information
Version: 1.26.14
- Correlation ID:
ad289550-7608-11f1-9513-431f8e71057d - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review
SummaryThe following content is AI-generated and provides a summary of the pull request: Chore: Add
|
PDT42
left a comment
PDT42
left a comment
There was a problem hiding this comment.
I am slightly worried about the effect of removing the chai import. AFAIK cds.test will now fall-back on expect.js, which does not behave entirely the same. However, the only real issue I did encounter with that so far, was .not.to.have.property silently passing and I didn't find that one being used anywhere in the tests of this repo.
🚀
cds-test has chai and chai-as-promise as peer deps → we get them from there. i will still add a package-lock before merging. fixing hana workflow first... |
## Problem The HANA workflow's `passport.test.js › gets set once for simple queries` fails on `cds-10-9` — see [run 28586742752](https://github.com/cap-js/telemetry/actions/runs/28586742752): ``` Message: expected 4 to strictly equal 2 > 41 | expect(_count).to.equal(2) ``` ## Root cause CDS v10 sets [`cds.requires.scheduling = true`](055f112) by default. Its Scheduler polls `cds.outbox.Messages` in its own database transactions. Those extra transactions overlap request timing and each triggers this plugin's `BEGIN` reset ([lib/tracing/index.js:141-143](https://github.com/cap-js/telemetry/blob/cds-10-9/lib/tracing/index.js#L141-L143)) + per-SELECT passport set ([lib/tracing/trace.js:351](https://github.com/cap-js/telemetry/blob/cds-10-9/lib/tracing/trace.js#L351)). Reproduced locally on `cds-10-9` with `@sap/cds@10.0.3` + `@cap-js/hana@3.0.1`. Instrumented the plugin's `dbc.set()` and captured the exact sequence: ``` [0] '' (reset) ← BEGIN of scheduler outbox tick [1] 2A54482A03… ← SELECT cds.outbox.Messages [2] '' (reset) ← BEGIN of user request [3] 2A54482A03… ← SELECT AdminService.Books ``` 2 transactions × (1 reset + 1 passport) = `_count = 4`. The test's assertion of `2` was correct for CDS v9's single-transaction-per-request behavior — cds v10 broke that invariant. The queue-metrics plugin already needed adjusting for the same scheduling shift — see [#435](#435) ("adapt queue metrics to CDS v10 scheduling changes"). Passport was overlooked. ## Fix Disable `scheduling` for this test only. The test's purpose is to verify SAP_PASSPORT stamping during a user request; the scheduler is irrelevant to that check and its background transactions just produce non-deterministic extra sets. ```diff process.env.SAP_PASSPORT = 'true' +// CDS v10 enables scheduling by default; its periodic outbox reads run in +// their own transactions and cause spurious SAP_PASSPORT set/reset pairs on +// the connection, breaking the deterministic _count assertions below. +process.env.cds_requires_scheduling = 'false' ``` ## Verification Ran the CI-scoped subset (`CI=true HANA_DRIVER=hdb HANA_PROM=true npm test`) locally against the same HDI container / driver stack the workflow uses: ``` PASS test/passport.test.js PASS test/tracing-attributes.test.js (18.184 s) Test Suites: 2 passed, 2 total Tests: 7 passed, 7 total ``` ## Follow-up (out of scope for this PR) There's an underlying semantic issue: the passport plugin currently stamps SAP_PASSPORT on every database call, including scheduler/outbox internal transactions that have no user request to correlate to. That pollutes DSR passport traces on production HANA — worth a separate PR that skips `dbc.set` when `cds.context?.user?.id === 'privileged'` at both sites. --------- Co-authored-by: sjvans <sjvans@users.noreply.github.com>
## Summary
- **DB inner spans** now show `VERB table` instead 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 the
`db.query.text` attribute per OTel convention.
- **Cloud SDK spans** now use `METHOD /path` (query string stripped) by
default, or `METHOD` only when `_append_url_path` is `false` —
consistent with the HTTP root span name behaviour controlled by the same
flag. Destination name removed from span name; still available as
`sap.btp.destination` attribute.
## Before / After
| | Before | After |
|---|---|---|
| DB SELECT | `@cap-js/sqlite - prepare SELECT
json_insert('{}','$."createdAt"'…` | `@cap-js/sqlite - prepare SELECT
sap.capire.bookshop.Books` |
| DB INSERT | `@cap-js/sqlite - exec INSERT INTO
sap_capire_bookshop_Books …` | `@cap-js/sqlite - exec INSERT
sap.capire.bookshop.Books` |
| DB UPDATE | `@cap-js/sqlite - stmt.run UPDATE
sap_capire_bookshop_Books …` | `@cap-js/sqlite - stmt.run UPDATE
sap.capire.bookshop.Books` |
| DB DELETE | `@cap-js/sqlite - exec DELETE FROM
sap_capire_bookshop_Books …` | `@cap-js/sqlite - exec DELETE
sap.capire.bookshop.Books` |
| Cloud SDK (`_append_url_path: true`) | `S4HANA GET
https://s4hana.example.com/sap/opu/…?$top=10` | `GET /sap/opu/…` |
| Cloud SDK (`_append_url_path: false`) | `S4HANA GET https://…` | `GET`
|
| HTTP root (`_append_url_path: true`) | `GET` | `GET
/odata/v4/admin/Books` |
> Raw `db.run('SELECT ...')` is unchanged — the SQL string remains in
the name since there is no `db.operation.name`/`db.sql.table` to
substitute.
## Test plan
- [ ] `test/tracing-span-names.test.js` added — covers DB span name
format (SELECT, INSERT) and Cloud SDK URL-free names
---------
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>
Co-authored-by: sjvans <sjvans@users.noreply.github.com>
2 participants
Uh oh!
There was an error while loading. Please reload this page.