Conversation
There was a problem hiding this comment.
The PR cleanly migrates test configuration from scattered process.env overrides into a consolidated test/bookshop/.cdsrc.json with named CDS profiles, which is a clear improvement in maintainability. The main concern is the persistent-outbox profile missing the outbox kind configuration that the old code explicitly set — worth verifying before re-enabling that test.
PR Bot Information
Version: 1.26.11
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
6c7ec365-2411-4079-83d3-9a5ab3e24908 - File Content Strategy: Full file content
SummaryThe following content is AI-generated and provides a summary of the pull request: Chore: Upgrade to
|
There was a problem hiding this comment.
The PR is a well-structured migration of test configuration from scattered process.env overrides to a centralized .cdsrc.json with CDS profiles, and includes several legitimate bug fixes (duplicate E1 retry loops, missing E2 assertions). The main concerns are: (1) peerDependencies should be widened to ^9 || ^10 since the changelog advertises cds^10 support; (2) the persistent-outbox profile is missing the outbox kind configuration that the original code set, which will silently break the test when it's re-enabled after the vitest migration.
PR Bot Information
Version: 1.26.11
- LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
- Event Trigger:
pull_request.ready_for_review - Correlation ID:
0e903766-6f6a-4d1f-9589-664bad1bee33
| - run: npm i -g @sap/cds-dk@${{ matrix.cds-version }} | ||
| - run: npm i | ||
| - run: if [ ${{ matrix.cds-version }} -eq 9 ]; then npm i -f @sap/cds@8 @cap-js/sqlite@2 @sap/cds-mtxs@3; fi | ||
| #- run: if [ ${{ matrix.cds-version }} -eq 9 ]; then npm i -f @sap/cds@9 @cap-js/sqlite@2 @sap/cds-mtxs@3; fi |
There was a problem hiding this comment.
Bug: The commented-out step originally forced @sap/cds@8 (now corrected to @sap/cds@9 in the comment), but the active matrix only tests cds-version: [9] while cds-version: [10] is also commented out. The better-sqlite3 rebuild step on line 40 still fires for cds-version -eq 9, so this is fine for now — but the commented line references a workaround that is no longer needed if cds@9 is already installed via line 37. The comment text says @sap/cds@9 but this step was originally meant to downgrade when cds-dk installed a newer version; the comment is now misleading about its purpose.
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
| }, | ||
| "peerDependencies": { | ||
| "@sap/cds": "^8 || ^9" | ||
| "@sap/cds": "^9" |
There was a problem hiding this comment.
Bug: The peerDependencies was updated to "@sap/cds": "^9" but the CHANGELOG and PR description say this release adds support for cds^10 and drops cds^8. The peer dependency range should be "^9 || ^10" (or "^10" if ^9 support is also being dropped) to accurately reflect compatibility.
| "@sap/cds": "^9" | |
| "@sap/cds": "^9 || ^10" |
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
| process.env.cds_requires_messaging = JSON.stringify({ | ||
| kind: 'file-based-messaging', | ||
| outbox: true, | ||
| file: `../${CASE}` |
There was a problem hiding this comment.
Bug: The persistent-outbox profile only sets messaging kind/file but omits the persistent outbox configuration (the original code also set cds_requires_outbox = { kind: 'persistent-outbox' }). When this test is re-enabled, it will not actually exercise persistent outbox behavior — the missing outbox config should be tracked in a REVISIT comment or restored in the profile.
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
# Release: `v2.0.0` — OpenTelemetry SDK 2.0 Support ### New Features 🚀 This major release upgrades `@cap-js/telemetry` to support **OpenTelemetry SDK 2.0**, introduces `@opentelemetry/instrumentation-undici` as a default instrumentation, adds CALM (`@sap/xotel-agent-ext-js`) integration, and drops support for `@sap/cds^8`. ### Changes * `package.json`: Bumped version to `2.0.0`. Updated all `@opentelemetry/*` dependencies to SDK 2.x-compatible versions. Replaced `@opentelemetry/host-metrics` with `@opentelemetry/instrumentation-host-metrics`. Added `@opentelemetry/instrumentation-undici` as a new default instrumentation. Dropped `@sap/cds^8` peer dependency support. Updated `vcap` binding config for Dynatrace and Cloud Logging to also match by tag. * `cds-plugin.js`: Removed the startup check that blocked usage with OpenTelemetry SDK 2.0. Added a workaround for `cds build` command detection. * `lib/index.js`: Refactored to support two setup modes: `setup_standalone` (default) and `setup_with_calm` (when `@sap/xotel-agent-ext-js` is detected). Auto-detection of `@opentelemetry/instrumentation-host-metrics` as an instrumentation, with `metricGroups` limited to `['process.cpu', 'process.memory']` by default. * `lib/tracing/index.js`: Added CALM delegate support for span processors. Replaced deprecated `Resource` with `resourceFromAttributes`. Updated to use `spanProcessors` constructor option instead of `addSpanProcessor`. Added SAP Passport cleanup for new transactions. * `lib/metrics/index.js`: Added CALM delegate support for metric readers. Replaced deprecated `Resource`/`View`/`DropAggregation` APIs with `resourceFromAttributes` and `readers` constructor option. Removed `lib/metrics/host.js` (replaced by instrumentation-based approach). * `lib/logging/index.js`: Added CALM delegate support for log processors. Refactored provider initialization. Improved error message for missing Cloud Logging credentials. * `lib/utils.js`: Replaced `getEnv`/`getEnvWithoutDefaults` with `getStringFromEnv`. Replaced `new Resource(...)` with `resourceFromAttributes(...)` throughout. * `lib/exporter/ConsoleSpanExporter.js`: Updated span parent references from deprecated `parentSpanId` to `parentSpanContext?.spanId`. * `lib/exporter/ConsoleMetricExporter.js`: Updated scope name check for host metrics instrumentation. * `lib/tracing/trace.js`: Updated `instrumentationLibrary` reference to `instrumentationScope` (SDK 2.0 rename). * `README.md`: Removed the "not yet supported" warning for OpenTelemetry SDK 2.0. Updated host metrics docs to reflect the new `metricGroups` configuration. Added `undici` to default instrumentations list. Clarified Cloud Logging user-provided service tag requirement to `"Cloud Logging"` only. * `CHANGELOG.md`: Updated version header to `2.0.0` with added, changed, fixed, and removed entries. * `.github/workflows/ci.yml`: Updated CI matrix to test only with `cds-version: [9]`; added branch `2` to push triggers. * `test/bookshop/.cdsrc.json`: Extracted CDS profile-based test configurations from `package.json` into a dedicated config file. * Various test files: Migrated from `cds.test().in(...)` to `cds.test(..., '--profile', ...)` pattern; reduced wait times from 300ms to 150ms in metrics tests. - [ ] 🔄 Regenerate and Update Summary <details> <summary>PR Bot Information</summary> **Version:** `1.26.11` - Event Trigger: `pull_request.edited` - Output Template: [Default Template](https://github.tools.sap/intelligent-insights/i2-pull-request/blob/main/src/services/llm/prompts/summary_default_output_template.md) - Correlation ID: `99a59df9-b306-487f-9485-bc9d1b221f55` - Summary Prompt: [Default Prompt](https://github.tools.sap/intelligent-insights/i2-pull-request/blob/main/src/services/llm/prompts/summary_instructions_prompt.md) - File Content Strategy: Full file content - LLM: `anthropic--claude-4.6-sonnet` </details> --- compiled of: - #364 + #424 + #442 - #407 - #374 - #425 - #443 --------- 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>
Chore: Upgrade to
cds^10— Migrate Test Config to.cdsrc.jsonand CDS ProfilesRefactor
♻️ Migrates test configuration from inline
process.envoverrides to a dedicated.cdsrc.jsonfile and CDS CLI profiles, aligning with thecds^10approach for environment/profile-based configuration. Also includes test timing optimizations and minor bug fixes.Changes
.github/workflows/ci.yml: Commented out thecds@8downgrade step; updated comment to referencecds@9..github/workflows/release.yml: Added aREVISITcomment to remove thebetter-sqlite3install workaround withcds^10..gitignore: Removedtest/bookshop/.cdsrc.jsonfrom 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_outboxand profile-specific[metrics-outbox]/[metrics-outbox-disabled]blocks (now in.cdsrc.json); added_scheduling: true.test/logging.test.js: Removed inlineprocess.envoverrides for telemetry/log format; switched tocds.test(..., '--profile', 'logging')with a minimal remaining env override forcls_custom_fields.test/metrics.test.js: RemovedexportIntervalMillisenv override; switched tocds.test(..., '--profile', 'metrics').test/metrics-outbox.test.js/test/metrics-outbox-multitenant.test.js: Reduced metric collection wait times from300msto150ms; fixed retry loop bug where both loops checkedE1instead ofE1/E2respectively.test/tracing-attributes.test.js: Removed inline exporter env var; switched tocds.test(..., '--profile', 'tracing-attributes').test/tracing-messaging-persistent-outbox.test.js: RenamedCASEtopersistent-outbox; simplified env setup; markeddescribeblock asskippending vitest migration.test/tracing-messaging-without-outbox.test.js: RenamedCASEtowithout-outbox; simplified env setup usingoutboxed: false.test/tracing-messaging-with-in-memory-outbox.test.js: Removed this test file entirely.test/tracing-messaging.js: Switched fromcds.test().in(...)tocds.test(..., '--profile', CASE).test/tracing.test.js: Switched fromcds.test().in(...)tocds.test(...).PR Bot Information
Version:
1.26.1114c5ae4d-4c97-4eff-a004-330c9fdcf2fepull_request.editedanthropic--claude-4.6-sonnet