fix: missing SQL-related attributes (#352)

sjvans · web-flow · commit e2d6dfaf3553 · 2025-06-27T12:56:46.000Z
closes #351
diff --git a/.github/workflows/hana.yml b/.github/workflows/hana.yml
@@ -0,0 +1,35 @@
+name: CI
+
+permissions:
+  contents: read
+
+on:
+  workflow_dispatch:
+  push:
+    branches: [main]
+  pull_request:
+    branches: [main]
+
+jobs:
+  hana:
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      max-parallel: 1
+      matrix:
+        hana-driver: [hdb, hana-client]
+        hana-prom: [true, false]
+    env:
+      HANA_CREDS: ${{ secrets.HANA_CREDS }}
+      HANA_DRIVER: ${{ matrix.hana-driver }}
+      HANA_PROM: ${{ matrix.hana-prom }}
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions/setup-node@v2
+      - run: npm i -g @sap/cds-dk
+      - run: npm add @cap-js/hana
+      - run: npm add hdb
+      - run: npm add @sap/hana-client
+      - run: npm i
+      - run: cds v
+      - run: npm run test
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 The format is based on [Keep a Changelog](http://keepachangelog.com/).
 
+## Version 1.4.1 - 2025-06-27
+
+### Fixed
+
+- Missing SQL-related attributes with `cds.requires.telemetry.tracing._hana_prom = true`
+
 ## Version 1.4.0 - 2025-06-05
 
 ### Added
diff --git a/jest.config.js b/jest.config.js
@@ -3,4 +3,14 @@ const config = {
   testMatch: ['**/*.test.js']
 }
 
+if (process.env.CI && process.env.HANA_DRIVER) {
+  config.testTimeout *= 10
+  config.testMatch = ['**/tracing-attributes.test.js']
+
+  process.env.cds_requires_db = JSON.stringify({ kind: 'hana', credentials: JSON.parse(process.env.HANA_CREDS) })
+
+  if (process.env.HANA_PROM)
+    process.env.cds_requires_telemetry_tracing = JSON.stringify({ _hana_prom: process.env.HANA_PROM === 'true' })
+}
+
 module.exports = config
diff --git a/lib/tracing/cds.js b/lib/tracing/cds.js
@@ -30,6 +30,7 @@ const _wrapStmt = (stmt, impl, sql) => {
 
 module.exports = () => {
   const { _tx, _hana_prom, _wrap_hana } = cds.env.requires.telemetry.tracing
+  const _TX = { BEGIN: 1, COMMIT: 1, ROLLBACK: 1 }
 
   const { emit: _emit, handle: _handle } = cds.Service.prototype
   cds.Service.prototype.emit = wrap(_emit, {
@@ -40,7 +41,7 @@ module.exports = () => {
   })
   cds.Service.prototype.handle = wrap(_handle, {
     wrapper: function handle(req) {
-      if (!_tx && req.event in { BEGIN: 1, COMMIT: 1, ROLLBACK: 1 }) return _handle.apply(this, arguments)
+      if (!_tx && req.event in _TX) return _handle.apply(this, arguments)
       return trace(req, _handle, this, arguments, {})
     }
   })
@@ -108,9 +109,9 @@ module.exports = () => {
             hanaDriver.prom = function (dbc, func) {
               const fn = _prom(dbc, func)
               dbc = dbc._parentConnection || dbc
-              const driver = dbc.constructor.name === 'Client' ? 'hdb' : '@sap/hana-client'
               return function prom() {
-                return trace(`${driver} - ${func}`, fn, this, arguments, { dbc, fn: func, kind: SpanKind.CLIENT })
+                if (!_tx && func.toUpperCase() in _TX) return fn.apply(this, arguments)
+                return trace(`@cap-js/hana - ${func}`, fn, this, arguments, { dbc, fn: func, kind: SpanKind.CLIENT })
               }
             }
             return
@@ -136,7 +137,7 @@ module.exports = () => {
       })
       dbService.prototype.exec = wrap(_exec, {
         wrapper: function exec(sql) {
-          if (!_tx && sql in { BEGIN: 1, COMMIT: 1, ROLLBACK: 1 }) return _exec.apply(this, arguments)
+          if (!_tx && sql in _TX) return _exec.apply(this, arguments)
           return trace(`${impl} - exec ${sql}`, _exec, this, arguments, { sql, fn: 'exec', kind: SpanKind.CLIENT })
         }
       })
diff --git a/lib/tracing/trace.js b/lib/tracing/trace.js
@@ -4,6 +4,7 @@ const LOG = cds.log('telemetry')
 const otel = require('@opentelemetry/api')
 const { SpanKind, SpanStatusCode } = otel
 const {
+  ATTR_SERVICE_NAME,
   ATTR_SERVER_ADDRESS,
   ATTR_SERVER_PORT,
   ATTR_URL_FULL,
@@ -87,11 +88,16 @@ function _getSpanName(arg, fn, that) {
   return `${trgt}${phs}${event}${pth}`
 }
 
-function _getStaticAttributes(fn) {
+function _getStaticAttributes(fn, options) {
   if (!fn[$reqattrs]) {
     const attributes = new Map()
 
-    if (fn.name) attributes.set(ATTR_CODE_FUNCTION, fn.name)
+    if (fn.name) {
+      attributes.set(ATTR_CODE_FUNCTION, fn.name)
+    } else if (options?.fn) {
+      // REVISIT: case _hana_prom = true
+      attributes.set(ATTR_CODE_FUNCTION, options?.fn)
+    }
 
     if (fn.__location) {
       const location = fn.__location
@@ -163,7 +169,10 @@ function _getDynamicDBAttributes(options, args, parent) {
   //       the second time, args is the string query -> we need to get event and target from the previous invocation (i.e., parentSpan).
   const dbAttributes = new Map()
   const db_statement =
-    options.sql || (typeof args[0]?.query === 'string' && args[0].query) || (typeof args[0] === 'string' && args[0])
+    options.sql ||
+    (typeof args[0]?.query === 'string' && args[0].query) ||
+    (typeof args[0] === 'string' && args[0]) ||
+    parent?.attributes[ATTR_DB_STATEMENT]
   if (db_statement) dbAttributes.set(ATTR_DB_STATEMENT, db_statement)
   const db_operation = args[0]?.event || parent?.attributes[ATTR_DB_OPERATION]
   if (db_operation) dbAttributes.set(ATTR_DB_OPERATION, db_operation)
@@ -178,7 +187,7 @@ function _setAttributes(span, attributes) {
 }
 
 function _addAttributes(span, fn, that, options, args, parent) {
-  _setAttributes(span, _getStaticAttributes(fn))
+  _setAttributes(span, _getStaticAttributes(fn, options))
 
   // needed for sampling decision (cf. shouldSample)
   if (cds.context.http?.req) {
@@ -187,9 +196,10 @@ function _addAttributes(span, fn, that, options, args, parent) {
     span.setAttribute('http.target', url_path) //> http.target is deprecated
   }
 
-  if (that.dbc || options.sql) {
-    //> NOTE: !that.dbc is the case during execution of a prepared statement
-
+  // NOTES:
+  // - !that.dbc is the case during execution of a prepared statement
+  // - with _hana_prom = true, options.sql is not there (the sql is in args)
+  if (that.dbc || options.dbc || options.sql) {
     if (cds.requires.multitenancy) {
       const creds = that.dbc?._connection?._settings || that.dbc?._creds //> hdb vs. @sap/hana-client
       _setAttributes(span, _getStaticDBAttributes(cds.context?.tenant, creds))
@@ -216,14 +226,14 @@ function _addAttributes(span, fn, that, options, args, parent) {
 }
 
 const _addDbRowCount = (span, res) => {
-  if (!span.attributes['db.statement']) return
-  if (!['all', 'run', 'exec'].includes(span.attributes['code.function'])) return
+  if (!span.attributes[ATTR_DB_STATEMENT]) return
+  if (!['all', 'run', 'exec'].includes(span.attributes[ATTR_CODE_FUNCTION])) return
 
   let rowCount
-  const operation = span.attributes['db.operation']
+  const operation = span.attributes[ATTR_DB_OPERATION]
   if (operation === 'READ') rowCount = res.length ?? 1
   // there is no attribute for affected rows -> also set returned_rows
-  else if (operation in { CREATE: 1, UPDATE: 1, DELETE: 1 }) rowCount = res.changes
+  else if (operation in { CREATE: 1, UPDATE: 1, DELETE: 1 }) rowCount = res.changes ?? res
   if (rowCount != null) span.setAttribute('db.client.response.returned_rows', rowCount)
 }
 
@@ -276,7 +286,6 @@ function trace(req, fn, that, args, opts = {}) {
   if (name === 'cds.spawn') name += kind === SpanKind.CONSUMER ? ' - run task' : ' - schedule task'
   else if (name.startsWith('messaging - emit'))
     name = name.replace('messaging - emit', `messaging - emit ${kind === SpanKind.CONSUMER ? 'incoming' : 'outgoing'}`)
-  if (name.length > 80 && _truncate_span_name !== false) name = name.substring(0, 79) + '…'
 
   const options = {
     kind,
@@ -295,6 +304,13 @@ 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_STATEMENT]) {
+    name += ' ' + options.attributes[ATTR_DB_STATEMENT]
+  }
+
+  if (name.length > 80 && _truncate_span_name !== false) name = name.substring(0, 79) + '…'
+
   // start a new active span, call the original function, and finally end the span
   return tracer.startActiveSpan(name, options, span => {
     // in case the span is non-recording, just call the original function
@@ -315,7 +331,7 @@ function trace(req, fn, that, args, opts = {}) {
         /* USER            */ '2020202020202020202020202020202020202020202020202020202020202020'}${
         /* ACTION          */ '20202020202020202020202020202020202020202020202020202020202020202020202020202020'}${
         /* ACTIONTYPE      */ '0000'}${
-        /* PREVCOMPONENTID */ Buffer.from(span.resource.attributes['service.name'].substr(0, 32).padEnd(32, ' ')).toString('hex')}${
+        /* PREVCOMPONENTID */ Buffer.from(span.resource.attributes[ATTR_SERVICE_NAME].substr(0, 32).padEnd(32, ' ')).toString('hex')}${
         /* TRANSACTIONID   */ Buffer.from(traceId.toUpperCase()).toString('hex')}${
         /* CLIENT          */ '202020'}${
         /* COMPONENTTYPE   */ '0000'}${
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@cap-js/telemetry",
-  "version": "1.4.0",
+  "version": "1.4.1",
   "description": "CDS plugin providing observability features, incl. automatic OpenTelemetry instrumentation.",
   "repository": {
     "type": "git",
diff --git a/test/_db_spans.js b/test/_db_spans.js
@@ -0,0 +1,39 @@
+module.exports = {
+  SELECT: [
+    {
+      // name: "@cap-js/sqlite - prepare SELECT json_insert('{}','$.\"createdAt\"',createdAt,'$.\"…",
+      name: s => s.match(/@cap-js\/\w+ - prepare /),
+      attributes: {
+        'code.function': 'prepare',
+        'db.system': s => s === 'sqlite' || 'hanadb',
+        'db.name': 'db',
+        'db.connection_string': s => s === ':memory:' || s.match(/jdbc/),
+        // 'db.statement':
+        //   'SELECT json_insert(\'{}\',\'$."createdAt"\',createdAt,\'$."createdBy"\',createdBy,\'$."modifiedAt"\',modifiedAt,\'$."modifiedBy"\',modifiedBy,\'$."ID"\',ID,\'$."title"\',title,\'$."descr"\',descr,\'$."author_ID"\',author_ID,\'$."genre_ID"\',genre_ID,\'$."stock"\',stock,\'$."price"\',price,\'$."currency_code"\',currency_code) as _json_ FROM (SELECT "$B".createdAt,"$B".createdBy,"$B".modifiedAt,"$B".modifiedBy,"$B".ID,"$B".title,"$B".descr,"$B".author_ID,"$B".genre_ID,"$B".stock,"$B".price,"$B".currency_code FROM sap_capire_bookshop_Books as "$B" WHERE "$B".title is not ?)',
+        'db.statement': s => s.match(/SELECT/),
+        'db.operation': 'READ',
+        'db.sql.table': 'sap.capire.bookshop.Books'
+      }
+    },
+    {
+      // name: "@cap-js/sqlite - stmt.all SELECT json_insert('{}','$.\"createdAt\"',createdAt,'$.…",
+      name: s => s.match(/@cap-js\/\w+ - stmt\.all /) || s.match(/@cap-js\/\w+ - exec /),
+      attributes: {
+        'code.function': s => s === 'all' || 'exec',
+        'db.system': s => s === 'sqlite' || 'hanadb',
+        'db.name': 'db',
+        'db.connection_string': s => s === ':memory:' || s.match(/jdbc/),
+        // 'db.statement':
+        //   'SELECT json_insert(\'{}\',\'$."createdAt"\',createdAt,\'$."createdBy"\',createdBy,\'$."modifiedAt"\',modifiedAt,\'$."modifiedBy"\',modifiedBy,\'$."ID"\',ID,\'$."title"\',title,\'$."descr"\',descr,\'$."author_ID"\',author_ID,\'$."genre_ID"\',genre_ID,\'$."stock"\',stock,\'$."price"\',price,\'$."currency_code"\',currency_code) as _json_ FROM (SELECT "$B".createdAt,"$B".createdBy,"$B".modifiedAt,"$B".modifiedBy,"$B".ID,"$B".title,"$B".descr,"$B".author_ID,"$B".genre_ID,"$B".stock,"$B".price,"$B".currency_code FROM sap_capire_bookshop_Books as "$B" WHERE "$B".title is not ?)',
+        'db.statement': s => s.match(/SELECT/),
+        'db.operation': 'READ',
+        'db.sql.table': 'sap.capire.bookshop.Books',
+        'db.client.response.returned_rows': 5
+      }
+    }
+  ]
+  // TODO
+  // INSERT: []
+  // UPDATE: []
+  // DELETE: []
+}
diff --git a/test/tracing-attributes.test.js b/test/tracing-attributes.test.js
@@ -9,30 +9,45 @@ describe('tracing attributes', () => {
   const log = jest.spyOn(console, 'dir')
   beforeEach(log.mockClear)
 
-  describe('db.client.response.returned_rows', () => {
+  describe('db', () => {
+    const _db_spans = require('./_db_spans')
+    // prettier-ignore
+    const _get_db_spans = o => JSON.parse(o).map(o => o[0]).filter(s => !s.name.startsWith('db'))
+    const _match_db_spans = (output, kind) => {
+      const db_spans = _get_db_spans(output)
+      for (const each of _db_spans[kind]) expect(db_spans).to.containSubset([each])
+    }
+
     test('SELECT', async () => {
-      await SELECT.from('sap.capire.bookshop.Books')
+      await SELECT.from('sap.capire.bookshop.Books').where('title !=', 'DUMMY')
       const output = JSON.stringify(log.mock.calls)
       expect(output).to.match(/db\.client\.response.returned_rows":5/)
+      _match_db_spans(output, 'SELECT')
     })
 
     test('INSERT', async () => {
       await INSERT.into('sap.capire.bookshop.Books').entries([{ ID: 1 }, { ID: 2 }])
       const output = JSON.stringify(log.mock.calls)
       expect(output).to.match(/db\.client\.response.returned_rows":2/)
+      // TODO
+      // _match_db_spans(output, 'INSERT')
     })
 
     test('UPDATE', async () => {
       await UPDATE('sap.capire.bookshop.Books').set({ stock: 42 }).where('ID > 250')
       const output = JSON.stringify(log.mock.calls)
       expect(output).to.match(/db\.client\.response.returned_rows":3/)
+      // TODO
+      // _match_db_spans(output, 'UPDATE')
     })
 
     test('DELETE', async () => {
       await DELETE.from('sap.capire.bookshop.Books')
       const output = JSON.stringify(log.mock.calls)
       expect(output).to.match(/db\.client\.response.returned_rows":0/) //> texts
       expect(output).to.match(/db\.client\.response.returned_rows":5/)
+      // TODO
+      // _match_db_spans(output, 'DELETE')
     })
   })
 })

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@cap-js/telemetry",`
`3`		`- "version": "1.4.0",`
	`3`	`+ "version": "1.4.1",`
`4`	`4`	`"description": "CDS plugin providing observability features, incl. automatic OpenTelemetry instrumentation.",`
`5`	`5`	`"repository": {`
`6`	`6`	`"type": "git",`