-
Notifications
You must be signed in to change notification settings - Fork 13
fix: more consumable span names #447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7d7a672
3fdad54
a785be4
419698e
b8f5c0c
f623a7f
89c985e
1583096
96eae90
502ecb1
8159699
166ee0f
f74ccac
7f6cdf3
41e06cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,8 +1,20 @@ | ||||||||
| const { SpanKind } = require('@opentelemetry/api') | ||||||||
|
|
||||||||
| const cds = require('@sap/cds') | ||||||||
| const trace = require('./trace') | ||||||||
| const wrap = require('./wrap') | ||||||||
|
|
||||||||
| const { _append_url_path } = cds.env.requires.telemetry.tracing | ||||||||
| const APPEND_URL_PATH = _append_url_path && _append_url_path !== 'false' | ||||||||
|
|
||||||||
| function _cloudSdkSpanName(destination, requestConfig) { | ||||||||
| const method = requestConfig?.method || 'GET' | ||||||||
| if (!APPEND_URL_PATH) return method | ||||||||
| const url = destination.url || requestConfig?.url || '' | ||||||||
| const path = url.replace(/^https?:\/\/[^/]+/, '').split('?')[0] | ||||||||
| return `${method}${path ? ' ' + path : ''}` | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug:
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||||
| } | ||||||||
|
|
||||||||
| // REVISIT: unverified! | ||||||||
| module.exports = () => { | ||||||||
| try { | ||||||||
|
|
@@ -16,9 +28,7 @@ module.exports = () => { | |||||||
| cloudSDK.executeHttpRequest = wrap(_execute, { | ||||||||
| wrapper: function executeHttpRequest(destination, requestConfig) { | ||||||||
| return trace( | ||||||||
| `${destination?.name ? destination?.name + ' ' : ''}${requestConfig?.method || 'GET'} ${ | ||||||||
| destination.url || requestConfig?.url | ||||||||
| }`, | ||||||||
| _cloudSdkSpanName(destination, requestConfig), | ||||||||
| _execute, | ||||||||
| this, | ||||||||
| arguments, | ||||||||
|
|
@@ -29,9 +39,7 @@ module.exports = () => { | |||||||
| cloudSDK.executeHttpRequestWithOrigin = wrap(_executeWithOrigin, { | ||||||||
| wrapper: function executeHttpRequestWithOrigin(destination, requestConfig) { | ||||||||
| return trace( | ||||||||
| `${destination?.name ? destination?.name + ' ' : ''}${requestConfig?.method || 'GET'} ${ | ||||||||
| destination.url || requestConfig?.url | ||||||||
| }`, | ||||||||
| _cloudSdkSpanName(destination, requestConfig), | ||||||||
| _executeWithOrigin, | ||||||||
| this, | ||||||||
| arguments, | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -41,9 +41,9 @@ const MASK_HEADERS = (cds.env.log.mask_headers || ['/authorization/i', '/cookie/ | |||||
|
|
||||||
| const CRUD = { CREATE: 1, READ: 1, UPDATE: 1, DELETE: 1 } | ||||||
|
|
||||||
| const { hrtime, adjust_root_name, _truncate_span_name } = cds.env.requires.telemetry.tracing | ||||||
| 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' | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breaking Change:
Suggested change
Double-check suggestion before committing. Edit this comment for amendments. Please provide feedback on the review comment by checking the appropriate box:
|
||||||
|
|
||||||
| const $hrnow = Symbol('@cap-js/telemetry:hrnow') | ||||||
| const $adjusted = Symbol('@cap-js/telemetry:adjusted') | ||||||
|
|
@@ -283,7 +283,7 @@ function trace(req, fn, that, args, opts = {}) { | |||||
| _setAttributes(parent, _getRequestAttributes()) | ||||||
| const ctx = cds.context | ||||||
| if (ctx?.http?.req?.[$hrnow]) parent.startTime = ctx.http.req[$hrnow] | ||||||
| if (ADJUST_ROOT_NAME && parent.attributes[ATTR_URL_PATH]) parent.name += ' ' + parent.attributes[ATTR_URL_PATH] | ||||||
| if (APPEND_URL_PATH && parent.attributes[ATTR_URL_PATH]) parent.name += ' ' + parent.attributes[ATTR_URL_PATH] | ||||||
| } | ||||||
|
|
||||||
| let name = typeof req === 'string' ? req : _getSpanName(req, fn, that) | ||||||
|
|
@@ -308,9 +308,19 @@ function trace(req, fn, that, args, opts = {}) { | |||||
| LOG._warn && LOG.warn('Failed to determine attributes:', err) | ||||||
| } | ||||||
|
|
||||||
| // REVISIT: with _hana_prom = true, the sql is not yet in the span name | ||||||
| if (name.match(/^@cap-js\/\w+ - \w+$/) && options.attributes[ATTR_DB_QUERY_TEXT]) { | ||||||
| name += ' ' + options.attributes[ATTR_DB_QUERY_TEXT] | ||||||
| // Normalize DB span names built in cds.js (prepare/exec/stmt.*) so raw SQL | ||||||
| // doesn't leak into the name; the SQL is preserved in db.query.text. | ||||||
| // Matches "@cap-js/<impl> - <verb>" optionally followed by " <sql...>", | ||||||
| // where <verb> is prepare | exec | stmt.<fn>. Covers both the sqlite/pg case | ||||||
| // (SQL is already baked in) and the HANA-promisified case (SQL not yet appended). | ||||||
| const dbNameMatch = name.match(/^(@cap-js\/\w+ - (?:prepare|exec|stmt\.\w+))(?:\s.*)?$/) | ||||||
| if (dbNameMatch && (options.attributes[ATTR_DB_OPERATION_NAME] || options.attributes[ATTR_DB_SQL_TABLE])) { | ||||||
| const SQL_VERB = { READ: 'SELECT', CREATE: 'INSERT' } | ||||||
| const op = options.attributes[ATTR_DB_OPERATION_NAME] | ||||||
| const verb = SQL_VERB[op] ?? op | ||||||
| const table = options.attributes[ATTR_DB_SQL_TABLE] | ||||||
| const tail = [verb, table].filter(Boolean).join(' ') | ||||||
| if (tail) name = `${dbNameMatch[1]} ${tail}` | ||||||
| } | ||||||
|
|
||||||
| if (name.length > 80 && _truncate_span_name !== false) name = name.substring(0, 79) + '…' | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| const cds = require('@sap/cds') | ||
| const { expect, data } = cds.test(__dirname + '/bookshop', '--profile', 'tracing-attributes') | ||
| const http = require('http') | ||
|
|
||
| describe('span names', () => { | ||
| beforeEach(data.reset) | ||
|
|
||
| const log = jest.spyOn(console, 'dir') | ||
| beforeEach(log.mockClear) | ||
|
|
||
| const getSpans = () => log.mock.calls.map(c => c[0]).filter(Boolean) | ||
|
|
||
| // Spans from our tracer only (excludes HTTP instrumentation spans) | ||
| const getCapSpans = () => getSpans().filter(s => s.instrumentationScope?.name === '@cap-js/telemetry') | ||
| const capSpanNames = () => getCapSpans().map(s => s.name) | ||
|
|
||
| describe('db', () => { | ||
| test('SELECT: inner DB span name uses operation+table, not SQL', async () => { | ||
| await SELECT.from('sap.capire.bookshop.Books').where('title !=', 'DUMMY') | ||
| const names = capSpanNames() | ||
| // outer CAP span: event+target (unchanged) | ||
| expect(names).to.include('db - READ sap.capire.bookshop.Books') | ||
| // inner SQLite prepare span: SQL verb + table appended, not raw SQL | ||
| expect(names.some(n => /^@cap-js\/\w+ - prepare SELECT sap\.capire\.bookshop\.Books$/.test(n))).to.be.true | ||
| // no span name should carry raw SQL (function calls, column lists, etc.) | ||
| // but a bare "SELECT <table>" is the intended shape | ||
| for (const name of names) { | ||
| expect(name, `span "${name}" contains raw SQL`).not.to.match( | ||
| /SELECT\s+\S+,|SELECT\s+\w*\(|json_insert|INSERT\s+INTO|UPDATE\s+\S+\s+SET|DELETE\s+FROM/ | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| test('SELECT: inner DB span has db.query.text attribute with SQL', async () => { | ||
| await SELECT.from('sap.capire.bookshop.Books').where('title !=', 'DUMMY') | ||
| const spans = getCapSpans() | ||
| const prepareSpan = spans.find(s => /^@cap-js\/\w+ - prepare /.test(s.name)) | ||
| expect(prepareSpan).to.exist | ||
| // SQL lives in the attribute, not the name | ||
| expect(prepareSpan.attributes['db.query.text']).to.match(/SELECT/) | ||
| expect(prepareSpan.attributes['db.operation.name']).to.equal('READ') | ||
| expect(prepareSpan.attributes['db.sql.table']).to.equal('sap.capire.bookshop.Books') | ||
| }) | ||
|
|
||
| test('raw SQL via db.run: span name preserves the SQL string', async () => { | ||
| const db = await cds.connect.to('db') | ||
| await db.run('SELECT ID, title FROM sap_capire_bookshop_Books WHERE ID = 201') | ||
| const names = capSpanNames() | ||
| // _getSpanName: event === undefined path -> "db - <sql>" | ||
| expect(names.some(n => n.startsWith('db - SELECT'))).to.be.true | ||
| }) | ||
|
|
||
| test('INSERT: inner DB span name uses operation+table, not SQL', async () => { | ||
| await INSERT.into('sap.capire.bookshop.Books').entries([{ ID: 999 }]) | ||
| const names = capSpanNames() | ||
| expect(names).to.include('db - CREATE sap.capire.bookshop.Books') | ||
| for (const name of names) { | ||
| expect(name, `span "${name}" contains SQL`).not.to.match(/INSERT\s+INTO|json_insert/) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| describe('cloud sdk', () => { | ||
| let server, port | ||
|
|
||
| beforeAll(done => { | ||
| server = http.createServer((req, res) => { | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }) | ||
| res.end(JSON.stringify({ value: [] })) | ||
| }) | ||
| server.listen(0, () => { | ||
| port = server.address().port | ||
| done() | ||
| }) | ||
| }) | ||
|
|
||
| afterAll(() => new Promise(resolve => server.close(resolve))) | ||
|
|
||
| test('Cloud SDK span name uses destination+method, not URL', async () => { | ||
| // Cloud SDK resilience module resolution issues in cds < 9 | ||
| if (Number(cds.version.split('.')[0]) < 9) return | ||
|
|
||
| cds.env.requires.TestRemote = { kind: 'odata', credentials: { url: `http://localhost:${port}` } } | ||
| const remote = await cds.connect.to('TestRemote') | ||
| await remote.send({ method: 'GET', path: '/test' }) | ||
|
|
||
| // no CAP-level span name should contain a URL | ||
| for (const name of capSpanNames()) { | ||
| expect(name, `span "${name}" contains URL`).not.to.match(/https?:\/\//) | ||
| } | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking Change:
adjust_root_namewas the previously documented config key; it has been silently renamed to_append_url_pathwith no backward-compatibility shim. Existing deployments that setadjust_root_name: truewill silently get the old behavior (full URL in span name) after upgrading.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box: