Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ jobs:
fail-fast: false
matrix:
node-version: [24.x, 22.x]
cds-version: [9, 8]
#cds-version: [10, 9]
cds-version: [9]
steps:
- uses: actions/checkout@v6.0.3
- name: Use Node.js ${{ matrix.node-version }}
Expand All @@ -35,7 +36,7 @@ jobs:
node-version: ${{ matrix.node-version }}
- run: npm i -g @sap/cds-dk@${{ matrix.cds-version }}
- run: npm i
- run: if [ ${{ matrix.cds-version }} -eq 8 ]; then npm i -f @sap/cds@8 @cap-js/sqlite@1 @sap/cds-mtxs@2; fi
- run: npm explore better-sqlite3 -- npm run install
#- run: if [ ${{ matrix.cds-version }} -eq 9 ]; then npm i -f @sap/cds@9 @cap-js/sqlite@2 @sap/cds-mtxs@3; fi
- run: if [ ${{ matrix.cds-version }} -eq 9 ]; then npm explore better-sqlite3 -- npm run install; fi
- run: cds v
- run: npm run test
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,5 @@ dist
# test
test/msg-box
test/bookshop/gen
test/bookshop/.cdsrc.json

.claude/
102 changes: 102 additions & 0 deletions test/bookshop/.cdsrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
{
"[logging]": {
"requires": {
"telemetry": {
"tracing": {
"sampler": {
"ignoreIncomingPaths": ["/odata/v4/admin/Genres"]
}
},
"logging": {
"exporter": {
"module": "@opentelemetry/sdk-logs",
"class": "ConsoleLogRecordExporter"
},
"processor": {
"module": "./lib/MySimpleLogRecordProcessor.js",
"class": "MySimpleLogRecordProcessor"
}
}
}
},
"log": {
"format": "json",
"cls_custom_fields": ["foo"]
}
},
"[metrics]": {
"requires": {
"telemetry": {
"metrics": {
"config": {
"exportIntervalMillis": 100
}
}
}
}
},
"[metrics-outbox]": {
"requires": {
"queue": true,
"telemetry": {
"metrics": {
"config": {
"exportIntervalMillis": 100
},
"_db_pool": false,
"_queue": true,
"exporter": {
"module": "@opentelemetry/sdk-metrics",
"class": "ConsoleMetricExporter"
}
}
}
}
},
"[metrics-outbox-disabled]": {
"requires": {
"queue": true,
"telemetry": {
"metrics": {
"config": {
"exportIntervalMillis": 250
},
"_db_pool": false,
"_queue": false,
"exporter": {
"module": "@opentelemetry/sdk-metrics",
"class": "ConsoleMetricExporter"
}
}
}
}
},
"[tracing-attributes]": {
"requires": {
"telemetry": {
"tracing": {
"exporter": {
"module": "@opentelemetry/sdk-trace-node"
}
}
}
}
},
"[persistent-outbox]": {
"requires": {
"messaging": {
"kind": "file-based-messaging",
"file": "../persistent-outbox"
}
}
},
"[without-outbox]": {
"requires": {
"messaging": {
"kind": "file-based-messaging",
"file": "../without-outbox",
"outboxed": false
}
}
}
}
43 changes: 4 additions & 39 deletions test/bookshop/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
"@sap-cloud-sdk/http-client": "^4",
"@sap-cloud-sdk/resilience": "^4"
},
"devDependencies": {
"@sap/cds-dk": "*"
},
"cds": {
"requires": {
"telemetry": {
Expand Down Expand Up @@ -45,12 +48,10 @@
"_kind": "file-based-messaging",
"file": "../msg-box"
},
"_outbox": {
"kind": "persistent-outbox"
},
"queue": {
"legacyLocking": false
},
"_scheduling": true,
"ExternalServiceOne": {
"kind": "odata",
"model": "srv/external/ExternalServiceOne"
Expand Down Expand Up @@ -80,42 +81,6 @@
}
}
},
"[metrics-outbox]": {
"requires": {
"queue": true,
"telemetry": {
"metrics": {
"config": {
"exportIntervalMillis": 100
},
"_db_pool": false,
"_queue": true,
"exporter": {
"module": "@opentelemetry/sdk-metrics",
"class": "ConsoleMetricExporter"
}
}
}
}
},
"[metrics-outbox-disabled]": {
"requires": {
"queue": true,
"telemetry": {
"metrics": {
"config": {
"exportIntervalMillis": 250
},
"_db_pool": false,
"_queue": false,
"exporter": {
"module": "@opentelemetry/sdk-metrics",
"class": "ConsoleMetricExporter"
}
}
}
}
},
"log": {
"cls_custom_fields": [
"tenant_id"
Expand Down
27 changes: 3 additions & 24 deletions test/logging.test.js
Original file line number Diff line number Diff line change
@@ -1,31 +1,10 @@
/* eslint-disable no-console */

process.env.cds_log = JSON.stringify({
format: 'json',
cls_custom_fields: ['foo']
})

process.env.cds_requires_telemetry = JSON.stringify({
tracing: {
sampler: {
ignoreIncomingPaths: ['/odata/v4/admin/Genres']
}
},
logging: {
exporter: {
module: '@opentelemetry/sdk-logs',
class: 'ConsoleLogRecordExporter'
},
// experimental feature of the experimental feature!!!
processor: {
module: './lib/MySimpleLogRecordProcessor.js',
class: 'MySimpleLogRecordProcessor'
}
}
})
// REVISIT: even with profile "logging", cls_custom_fields from package.json wins
process.env.cds_log = JSON.stringify({ cls_custom_fields: ['foo'] })

const cds = require('@sap/cds')
const { expect, GET } = cds.test().in(__dirname + '/bookshop')
const { expect, GET } = cds.test(__dirname + '/bookshop', '--profile', 'logging')

describe('logging', () => {
const admin = { auth: { username: 'alice' } }
Expand Down
17 changes: 11 additions & 6 deletions test/metrics-outbox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,16 @@ describe('queue metrics for single tenant service', () => {

// Wait for the first retry to be initiated
while (currentRetryCount[E1] < 2) await wait(100)
while (currentRetryCount[E1] < 2) await wait(100)
await wait(150) // ... for the retry to be processed and metrics to be collected
while (currentRetryCount[E2] < 2) await wait(100)
await wait(300) // ... for the retry to be processed and metrics to be collected
Comment on lines +175 to +176

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic Error: The snapshot at line 182-183 is taken after await wait(300) at line 176, meaning E2's retry has already been processed and totalOut/totalFailed may already reflect the extra increment from retry 2. The snapshot should be taken before the 300ms wait to reliably capture the state before the retry fires.

Suggested change
while (currentRetryCount[E2] < 2) await wait(100)
await wait(300) // ... for the retry to be processed and metrics to be collected
while (currentRetryCount[E2] < 2) await wait(100)
// Snapshot counters now: retry 2 fires at t+1250ms (waitingTime(2)), which overlaps
// with the wait below, causing totalOut/totalFailed to advance ahead of the metric export
const snapOut = { ...totalOut }
const snapFailed = { ...totalFailed }
await wait(300) // ... for the retry to be processed and metrics to be collected

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

expect(currentRetryCount[E1]).to.eq(2)
expect(currentRetryCount[E2]).to.eq(2)

// Snapshot counters now: retry 2 fires at t+1250ms (waitingTime(2)), which overlaps
// with the wait below, causing totalOut/totalFailed to advance ahead of the metric export
const snapOut = { ...totalOut }
const snapFailed = { ...totalFailed }

// Wait until at least 1 second has passed since the initial call
const timeAfterFirstRetry = Date.now()
if (timeAfterFirstRetry - timeOfInitialCall < 1000) {
Expand All @@ -188,17 +193,17 @@ describe('queue metrics for single tenant service', () => {
expect(metricValue('cold_entries', E1)).to.eq(0)
expect(metricValue('remaining_entries', E1)).to.eq(1)
expect(metricValue('incoming_messages', E1)).to.eq(totalInc[E1])
expect(metricValue('outgoing_messages', E1)).to.eq(totalOut[E1])
expect(metricValue('processing_failures', E1)).to.eq(totalFailed[E1])
expect(metricValue('outgoing_messages', E1)).to.eq(snapOut[E1])
expect(metricValue('processing_failures', E1)).to.eq(snapFailed[E1])
expect(metricValue('min_storage_time_in_seconds', E1)).to.be.gte(1)
expect(metricValue('med_storage_time_in_seconds', E1)).to.be.gte(1)
expect(metricValue('max_storage_time_in_seconds', E1)).to.be.gte(1)

expect(metricValue('cold_entries', E2)).to.eq(0)
expect(metricValue('remaining_entries', E2)).to.eq(1)
expect(metricValue('incoming_messages', E2)).to.eq(totalInc[E2])
expect(metricValue('outgoing_messages', E2)).to.eq(totalOut[E2])
expect(metricValue('processing_failures', E2)).to.eq(totalFailed[E2])
expect(metricValue('outgoing_messages', E2)).to.eq(snapOut[E2])
expect(metricValue('processing_failures', E2)).to.eq(snapFailed[E2])
expect(metricValue('min_storage_time_in_seconds', E2)).to.be.gte(1)
expect(metricValue('med_storage_time_in_seconds', E2)).to.be.gte(1)
expect(metricValue('max_storage_time_in_seconds', E2)).to.be.gte(1)
Expand Down
3 changes: 1 addition & 2 deletions test/metrics.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
// process.env.HOST_METRICS_RETAIN_SYSTEM = 'true' //> with this the test would fail
process.env.HOST_METRICS_LOG_SYSTEM = 'true'
process.env.cds_requires_telemetry_metrics_config = JSON.stringify({ exportIntervalMillis: 100 })

const cds = require('@sap/cds')
const { expect, GET } = cds.test().in(__dirname + '/bookshop')
const { expect, GET } = cds.test(__dirname + '/bookshop', '--profile', 'metrics')
const log = cds.test.log()

const wait = require('node:timers/promises').setTimeout
Expand Down
4 changes: 1 addition & 3 deletions test/tracing-attributes.test.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
process.env.cds_requires_telemetry_tracing_exporter_module = '@opentelemetry/sdk-trace-node'

const cds = require('@sap/cds')
const { expect, data } = cds.test().in(__dirname + '/bookshop')
const { expect, data } = cds.test(__dirname + '/bookshop', '--profile', 'tracing-attributes')

describe('tracing attributes', () => {
beforeEach(data.reset)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,20 @@
const CASE = 'with_persistent-outbox'
const CASE = 'persistent-outbox'

const env = {
// REVISIT: even with profile "persistent-outbox", messaging kind and file from package.json wins
process.env.cds_requires_messaging = JSON.stringify({
kind: 'file-based-messaging',
outbox: true,
file: `../${CASE}`
}
process.env.cds_requires_messaging = JSON.stringify(env)
process.env.cds_requires_outbox = JSON.stringify({ kind: 'persistent-outbox' })

process.env.cds_requires_telemetry_metrics = null
})

// REVISIT: check json exports
const CHECK = (log, expect) => {
// 3: outbox -> consumers get new root context
// REVISIT: for some reason, span "cds.spawn run task" has no parent when running in jest
expect(log.output.match(/\[telemetry\] - elapsed times:/g).length).to.equal(4) //> actually 3
expect(log.output.match(/cds.spawn - schedule task/g).length).to.equal(1)
}

describe(`tracing messaging - ${CASE}`, () => {
// REVISIT: re-enable with switch to vitest
describe.skip(`tracing messaging - ${CASE}`, () => {
require('./tracing-messaging')(CASE, CHECK)
})
18 changes: 0 additions & 18 deletions test/tracing-messaging-with-in-memory-outbox.test.js

This file was deleted.

13 changes: 7 additions & 6 deletions test/tracing-messaging-without-outbox.test.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
const CASE = 'without_outbox'
const CASE = 'without-outbox'

const env = {
// REVISIT: even with profile "without-outbox", messaging kind and file from package.json wins
process.env.cds_requires_messaging = JSON.stringify({
kind: 'file-based-messaging',
outbox: false,
file: `../${CASE}`
}
process.env.cds_requires_messaging = JSON.stringify(env)
file: `../${CASE}`,
outboxed: false
})

// REVISIT: check json exports
const CHECK = (log, expect) => {
// 2: no outbox -> consumer gets new root context
expect(log.output.match(/\[telemetry\] - elapsed times:/g).length).to.equal(2)
Expand Down
2 changes: 1 addition & 1 deletion test/tracing-messaging.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module.exports = (CASE, CHECK) => {
const cds = require('@sap/cds')
const { expect, POST } = cds.test().in(__dirname + '/bookshop')
const { expect, POST } = cds.test(__dirname + '/bookshop', '--profile', CASE)
const log = cds.test.log()

const wait = require('node:timers/promises').setTimeout
Expand Down
2 changes: 1 addition & 1 deletion test/tracing.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ process.env.cds_requires_telemetry_tracing_sampler = JSON.stringify({
})

const cds = require('@sap/cds')
const { expect, GET, POST } = cds.test().in(__dirname + '/bookshop')
const { expect, GET, POST } = cds.test(__dirname + '/bookshop')
const log = cds.test.log()

const wait = require('node:timers/promises').setTimeout
Expand Down