feat(tracing): improve robustness of custom service naming

Previously, we only added span.data.service to _entry_ spans (when the environment variable INSTANA_SERVICE_NAME is set or the initial in-code configuration object has the `serviceName` attribute set). The requirements around this have changed, now we add this annotation to _all_ spans (but still only if it has been explicitly configured).
instana · Feb 22, 2023 · aadcbff · aadcbff
1 parent 8730dc7
commit aadcbff
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 24 deletions.
diff --git a/packages/collector/test/tracing/common/app.js b/packages/collector/test/tracing/common/app.js
@@ -33,7 +33,7 @@ if (process.env.SCREW_AROUND_WITH_UP_ARRAY_FIND) {
   });
 }
 
-require('../../..')(config);
+const instana = require('../../..')(config);
 
 const http = require('http');
 const pino = require('pino')();
@@ -46,6 +46,11 @@ app.on('request', (req, res) => {
   }
   if (req.url.indexOf('with-log') >= 0) {
     pino.error('This error message should be traced, unless the pino instrumentation is disabled.');
+  } else if (req.url.indexOf('with-intermediate-and-exit-spans') >= 0) {
+    instana.sdk.callback.startIntermediateSpan('dummy-sdk-span', () => {
+      pino.warn('create an exit span');
+      instana.sdk.callback.completeIntermediateSpan();
+    });
   }
   res.end();
 });

diff --git a/packages/collector/test/tracing/common/test.js b/packages/collector/test/tracing/common/test.js
@@ -11,7 +11,7 @@ const constants = require('@instana/core').tracing.constants;
 const supportedVersion = require('@instana/core').tracing.supportedVersion;
 const config = require('../../../../core/test/config');
 const delay = require('../../../../core/test/test_util/delay');
-const { retry, stringifyItems } = require('../../../../core/test/test_util');
+const { getSpansByName, retry, stringifyItems } = require('../../../../core/test/test_util');
 const ProcessControls = require('../../test_util/ProcessControls');
 const globalAgent = require('../../globalAgent');
 
@@ -134,7 +134,10 @@ mochaSuiteFn('tracing/common', function () {
         }
       });
       ProcessControls.setUpHooks(controls);
-      registerServiceNameTest.call(this, globalAgent.instance, controls, 'env var', true);
+      registerServiceNameTest.call(this, globalAgent.instance, controls, {
+        configMethod: 'env var',
+        expectServiceNameOnSpans: 'on-all-spans'
+      });
     });
 
     describe('with config', function () {
@@ -147,7 +150,10 @@ mochaSuiteFn('tracing/common', function () {
         }
       });
       ProcessControls.setUpHooks(controls);
-      registerServiceNameTest.call(this, globalAgent.instance, controls, 'config object', true);
+      registerServiceNameTest.call(this, globalAgent.instance, controls, {
+        configMethod: 'config object',
+        expectServiceNameOnSpans: 'on-all-spans'
+      });
     });
 
     describe('with header when agent is configured to capture the header', function () {
@@ -156,7 +162,10 @@ mochaSuiteFn('tracing/common', function () {
         agentControls,
         dirname: __dirname
       }).registerTestHooks();
-      registerServiceNameTest.call(this, agentControls, controls, 'X-Instana-Service header', true);
+      registerServiceNameTest.call(this, agentControls, controls, {
+        configMethod: 'X-Instana-Service header',
+        expectServiceNameOnSpans: 'on-entry-span'
+      });
     });
 
     describe('with header when agent is _not_ configured to capture the header', function () {
@@ -165,38 +174,64 @@ mochaSuiteFn('tracing/common', function () {
         agentControls,
         dirname: __dirname
       }).registerTestHooks();
-      registerServiceNameTest.call(this, agentControls, controls, 'X-Instana-Service header', false);
+      registerServiceNameTest.call(this, agentControls, controls, {
+        configMethod: 'X-Instana-Service header',
+        expectServiceNameOnSpans: 'no'
+      });
     });
 
-    function registerServiceNameTest(agentControls, controls, configMethod, expectServiceNameOnSpan) {
-      it(`must${expectServiceNameOnSpan ? ' ' : ' _not_ '}respect service name configured via: ${configMethod}`, () => {
+    function registerServiceNameTest(agentControls, controls, { configMethod, expectServiceNameOnSpans }) {
+      if (expectServiceNameOnSpans == null || typeof expectServiceNameOnSpans !== 'string') {
+        throw new Error('Please explicitly pass a string value for expectServiceNameOnSpans.');
+      }
+
+      it(`must${
+        expectServiceNameOnSpans === 'no' ? ' _not_ ' : ' '
+      }respect service name configured via: ${configMethod}`, () => {
         const req = {
-          path: '/'
+          path: '/with-intermediate-and-exit-spans'
         };
         if (configMethod === 'X-Instana-Service header') {
           req.headers = {
             'x-InsTana-sErvice': 'much-custom-very-wow service'
           };
         }
-        return controls.sendRequest(req).then(() => verifyServiceName(agentControls, expectServiceNameOnSpan));
+        return controls.sendRequest(req).then(() => verifyServiceName(agentControls, expectServiceNameOnSpans));
       });
     }
 
-    function verifyServiceName(agentControls, expectServiceNameOnSpan) {
-      return retry(() =>
-        agentControls.getSpans().then(spans => {
-          expect(spans.length).to.equal(1);
-          const span = spans[0];
-          expect(span.n).to.equal('node.http.server');
-          if (expectServiceNameOnSpan == null) {
-            throw new Error('Please explicitly pass true or false for expectServiceNameOnSpan');
-          } else if (expectServiceNameOnSpan) {
-            expect(span.data.service).to.equal('much-custom-very-wow service');
+    async function verifyServiceName(agentControls, expectServiceNameOnSpans) {
+      const spans = await retry(async () => {
+        const _spans = await agentControls.getSpans();
+        expect(_spans.length).to.equal(3);
+        return _spans;
+      });
+
+      const [entrySpan] = getSpansByName(spans, 'node.http.server');
+
+      if (expectServiceNameOnSpans === 'on-all-spans') {
+        spans.forEach(span => {
+          expect(span.data.service, `Missing span.data.service annotation on span ${span.n}`).to.equal(
+            'much-custom-very-wow service'
+          );
+        });
+      } else if (expectServiceNameOnSpans === 'on-entry-span') {
+        spans.forEach(span => {
+          if (span === entrySpan) {
+            expect(span.data.service, `Missing span.data.service annotation on span ${span.n}`).to.equal(
+              'much-custom-very-wow service'
+            );
           } else {
-            expect(span.data.service).to.not.exist;
+            expect(span.data.service, `Unexpected span.data.service annotation on span ${span.n}`).to.not.exist;
           }
-        })
-      );
+        });
+      } else if (expectServiceNameOnSpans === 'no') {
+        spans.forEach(span => {
+          expect(span.data.service, `Unexpected span.data.service annotation on span ${span.n}`).to.not.exist;
+        });
+      } else {
+        throw new Error(`Unknown value for parameter expectServiceNameOnSpans: ${expectServiceNameOnSpans}`);
+      }
     }
   });
 

diff --git a/packages/core/src/tracing/cls.js b/packages/core/src/tracing/cls.js
@@ -249,7 +249,7 @@ function startSpan(spanName, kind, traceId, parentSpanId, w3cTraceContext) {
   const parentSpan = getCurrentSpan();
   const parentW3cTraceContext = getW3cTraceContext();
 
-  if (serviceName != null && !parentSpan) {
+  if (serviceName != null) {
     span.data.service = serviceName;
   }