fix: change SpanContext.traceFlags to mandatory (#818)

* fix: change SpanContext.traceFlags to mandatory According to spec SpanContext represents the W3C tracestate which includes traceId, spanId and traceFlags. As a side effect a new LinkContext types was added as links don't have traceFlags according to spec. * chore: review findings, rename TraceFlags.UNSAMPLED to NONE * fix: build * fix: tests * fix: correct merge Co-authored-by: Daniel Dyla <[email protected]>
open-telemetry · Mar 6, 2020 · 3c41f56 · 3c41f56
1 parent 5c4c57e
commit 3c41f56
Show file tree

Hide file tree

Showing 35 changed files with 114 additions and 104 deletions.
diff --git a/packages/opentelemetry-api/src/index.ts b/packages/opentelemetry-api/src/index.ts
@@ -31,6 +31,7 @@ export * from './metrics/ObserverResult';
 export * from './trace/attributes';
 export * from './trace/Event';
 export * from './trace/instrumentation/Plugin';
+export * from './trace/link_context';
 export * from './trace/link';
 export * from './trace/NoopSpan';
 export * from './trace/NoopTracer';

diff --git a/packages/opentelemetry-api/src/trace/NoopSpan.ts b/packages/opentelemetry-api/src/trace/NoopSpan.ts
@@ -26,7 +26,7 @@ export const INVALID_SPAN_ID = '0';
 const INVALID_SPAN_CONTEXT: SpanContext = {
   traceId: INVALID_TRACE_ID,
   spanId: INVALID_SPAN_ID,
-  traceFlags: TraceFlags.UNSAMPLED,
+  traceFlags: TraceFlags.NONE,
 };
 
 /**
@@ -59,11 +59,6 @@ export class NoopSpan implements Span {
     return this;
   }
 
-  // By default does nothing
-  addLink(spanContext: SpanContext, attributes?: Attributes): this {
-    return this;
-  }
-
   // By default does nothing
   setStatus(status: Status): this {
     return this;

diff --git a/packages/opentelemetry-api/src/trace/link.ts b/packages/opentelemetry-api/src/trace/link.ts
@@ -15,16 +15,16 @@
  */
 
 import { Attributes } from './attributes';
-import { SpanContext } from './span_context';
+import { LinkContext } from './link_context';
 
 /**
  * A pointer from the current {@link Span} to another span in the same trace or
  * in a different trace. Used (for example) in batching operations, where a
  * single batch handler processes multiple requests from different traces.
  */
 export interface Link {
-  /** The {@link SpanContext} of a linked span. */
-  spanContext: SpanContext;
+  /** The {@link LinkContext} of a linked span. */
+  context: LinkContext;
   /** A set of {@link Attributes} on the link. */
   attributes?: Attributes;
 }
diff --git a/packages/opentelemetry-api/src/trace/link_context.ts b/packages/opentelemetry-api/src/trace/link_context.ts
@@ -0,0 +1,22 @@
+/*!
+ * Copyright 2020, OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { SpanContext } from './span_context';
+
+/**
+ * A pointer to another span.
+ */
+export type LinkContext = Pick<SpanContext, 'traceId' | 'spanId'>;
diff --git a/packages/opentelemetry-api/src/trace/span_context.ts b/packages/opentelemetry-api/src/trace/span_context.ts
@@ -47,9 +47,9 @@ export interface SpanContext {
    * caller may have recorded trace data. A caller who does not record trace
    * data out-of-band leaves this flag unset.
    *
-   * SAMPLED = 0x1 and UNSAMPLED = 0x0;
+   * SAMPLED = 0x1 and NONE = 0x0;
    */
-  traceFlags?: TraceFlags;
+  traceFlags: TraceFlags;
   /**
    * Tracing-system-specific info to propagate.
    *

diff --git a/packages/opentelemetry-api/src/trace/trace_flags.ts b/packages/opentelemetry-api/src/trace/trace_flags.ts
@@ -20,8 +20,8 @@
  * whether a Span should be traced. It is implemented as a bitmask.
  */
 export enum TraceFlags {
-  /** Bit to represent whether trace is unsampled in trace flags. */
-  UNSAMPLED = 0x0,
+  /** Represents no flag set. */
+  NONE = 0x0,
   /** Bit to represent whether trace is sampled in trace flags. */
-  SAMPLED = 0x1,
+  SAMPLED = 0x1 << 0,
 }
diff --git a/packages/opentelemetry-api/test/api/api.test.ts b/packages/opentelemetry-api/test/api/api.test.ts
@@ -42,7 +42,7 @@ describe('API', () => {
     const spanContext = {
       traceId: 'd4cda95b652f4a1592b449d5929fda1b',
       spanId: '6e0c63257de34c92',
-      traceFlags: TraceFlags.UNSAMPLED,
+      traceFlags: TraceFlags.NONE,
     };
     const dummySpan = new NoopSpan(spanContext);
 

diff --git a/packages/opentelemetry-api/test/noop-implementations/noop-span.test.ts b/packages/opentelemetry-api/test/noop-implementations/noop-span.test.ts
@@ -39,18 +39,6 @@ describe('NoopSpan', () => {
     span.addEvent('sent');
     span.addEvent('sent', { id: '42', key: 'value' });
 
-    span.addLink({
-      traceId: 'd4cda95b652f4a1592b449d5929fda1b',
-      spanId: '6e0c63257de34c92',
-    });
-    span.addLink(
-      {
-        traceId: 'd4cda95b652f4a1592b449d5929fda1b',
-        spanId: '6e0c63257de34c92',
-      },
-      { id: '42', key: 'value' }
-    );
-
     span.setStatus({ code: CanonicalCode.CANCELLED });
 
     span.updateName('my-span');
@@ -59,7 +47,7 @@ describe('NoopSpan', () => {
     assert.deepStrictEqual(span.context(), {
       traceId: INVALID_TRACE_ID,
       spanId: INVALID_SPAN_ID,
-      traceFlags: TraceFlags.UNSAMPLED,
+      traceFlags: TraceFlags.NONE,
     });
     span.end();
   });

diff --git a/packages/opentelemetry-core/src/context/propagation/B3Format.ts b/packages/opentelemetry-core/src/context/propagation/B3Format.ts
@@ -80,9 +80,7 @@ export class B3Format implements HttpTextFormat {
         traceId,
         spanId,
         isRemote: true,
-        traceFlags: isNaN(Number(options))
-          ? TraceFlags.UNSAMPLED
-          : Number(options),
+        traceFlags: isNaN(Number(options)) ? TraceFlags.NONE : Number(options),
       });
     }
     return context;

diff --git a/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts b/packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts
@@ -70,7 +70,7 @@ export class HttpTraceContext implements HttpTextFormat {
 
     const traceParent = `${VERSION}-${spanContext.traceId}-${
       spanContext.spanId
-    }-0${Number(spanContext.traceFlags || TraceFlags.UNSAMPLED).toString(16)}`;
+    }-0${Number(spanContext.traceFlags || TraceFlags.NONE).toString(16)}`;
 
     setter(carrier, TRACE_PARENT_HEADER, traceParent);
     if (spanContext.traceState) {

diff --git a/packages/opentelemetry-core/src/trace/spancontext-utils.ts b/packages/opentelemetry-core/src/trace/spancontext-utils.ts
@@ -21,7 +21,7 @@ export const INVALID_TRACEID = '0';
 export const INVALID_SPAN_CONTEXT: SpanContext = {
   traceId: INVALID_TRACEID,
   spanId: INVALID_SPANID,
-  traceFlags: TraceFlags.UNSAMPLED,
+  traceFlags: TraceFlags.NONE,
 };
 
 /**

diff --git a/packages/opentelemetry-core/test/context/B3Format.test.ts b/packages/opentelemetry-core/test/context/B3Format.test.ts
@@ -67,7 +67,7 @@ describe('B3Format', () => {
       const spanContext: SpanContext = {
         traceId: 'd4cda95b652f4a1592b449d5929fda1b',
         spanId: '6e0c63257de34c92',
-        traceFlags: TraceFlags.UNSAMPLED,
+        traceFlags: TraceFlags.NONE,
         traceState: new TraceState('foo=bar,baz=qux'),
         isRemote: false,
       };
@@ -82,13 +82,14 @@ describe('B3Format', () => {
         'd4cda95b652f4a1592b449d5929fda1b'
       );
       assert.deepStrictEqual(carrier[X_B3_SPAN_ID], '6e0c63257de34c92');
-      assert.deepStrictEqual(carrier[X_B3_SAMPLED], TraceFlags.UNSAMPLED);
+      assert.deepStrictEqual(carrier[X_B3_SAMPLED], TraceFlags.NONE);
     });
 
     it('should not inject empty spancontext', () => {
       const emptySpanContext = {
         traceId: '',
         spanId: '',
+        traceFlags: TraceFlags.NONE,
       };
       b3Format.inject(
         setExtractedSpanContext(Context.ROOT_CONTEXT, emptySpanContext),
@@ -98,25 +99,6 @@ describe('B3Format', () => {
       assert.deepStrictEqual(carrier[X_B3_TRACE_ID], undefined);
       assert.deepStrictEqual(carrier[X_B3_SPAN_ID], undefined);
     });
-
-    it('should handle absence of sampling decision', () => {
-      const spanContext: SpanContext = {
-        traceId: 'd4cda95b652f4a1592b449d5929fda1b',
-        spanId: '6e0c63257de34c92',
-      };
-
-      b3Format.inject(
-        setExtractedSpanContext(Context.ROOT_CONTEXT, spanContext),
-        carrier,
-        defaultSetter
-      );
-      assert.deepStrictEqual(
-        carrier[X_B3_TRACE_ID],
-        'd4cda95b652f4a1592b449d5929fda1b'
-      );
-      assert.deepStrictEqual(carrier[X_B3_SPAN_ID], '6e0c63257de34c92');
-      assert.deepStrictEqual(carrier[X_B3_SAMPLED], undefined);
-    });
   });
 
   describe('.extract()', () => {
@@ -131,7 +113,7 @@ describe('B3Format', () => {
         spanId: 'b7ad6b7169203331',
         traceId: '0af7651916cd43dd8448eb211c80319c',
         isRemote: true,
-        traceFlags: TraceFlags.UNSAMPLED,
+        traceFlags: TraceFlags.NONE,
       });
     });
 
@@ -179,7 +161,7 @@ describe('B3Format', () => {
         spanId: 'b7ad6b7169203331',
         traceId: '0af7651916cd43dd8448eb211c80319c',
         isRemote: true,
-        traceFlags: TraceFlags.UNSAMPLED,
+        traceFlags: TraceFlags.NONE,
       });
     });
 

diff --git a/packages/opentelemetry-core/test/trace/NoRecordingSpan.test.ts b/packages/opentelemetry-core/test/trace/NoRecordingSpan.test.ts
@@ -23,7 +23,7 @@ describe('NoRecordingSpan', () => {
     const spanContext = {
       traceId: 'd4cda95b652f4a1592b449d5929fda1b',
       spanId: '6e0c63257de34c92',
-      traceFlags: TraceFlags.UNSAMPLED,
+      traceFlags: TraceFlags.NONE,
     };
 
     const span = new NoRecordingSpan(spanContext);

diff --git a/packages/opentelemetry-core/test/trace/ProbabilitySampler.test.ts b/packages/opentelemetry-core/test/trace/ProbabilitySampler.test.ts
@@ -24,13 +24,7 @@ import {
 describe('ProbabilitySampler', () => {
   it('should return a always sampler for 1', () => {
     const sampler = new ProbabilitySampler(1);
-    assert.strictEqual(
-      sampler.shouldSample({
-        traceId: 'd4cda95b652f4a1592b449d5929fda1b',
-        spanId: '6e0c63257de34c92',
-      }),
-      true
-    );
+    assert.strictEqual(sampler.shouldSample(), true);
   });
 
   it('should return a always sampler for >1', () => {

diff --git a/packages/opentelemetry-core/test/trace/spancontext-utils.test.ts b/packages/opentelemetry-core/test/trace/spancontext-utils.test.ts
@@ -16,12 +16,14 @@
 
 import * as assert from 'assert';
 import * as context from '../../src/trace/spancontext-utils';
+import { TraceFlags } from '@opentelemetry/api';
 
 describe('spancontext-utils', () => {
   it('should return true for valid spancontext', () => {
     const spanContext = {
       traceId: 'd4cda95b652f4a1592b449d5929fda1b',
       spanId: '6e0c63257de34c92',
+      traceFlags: TraceFlags.NONE,
     };
     assert.ok(context.isValid(spanContext));
   });
@@ -30,6 +32,7 @@ describe('spancontext-utils', () => {
     const spanContext = {
       traceId: context.INVALID_TRACEID,
       spanId: '6e0c63257de34c92',
+      traceFlags: TraceFlags.NONE,
     };
     assert.ok(!context.isValid(spanContext));
   });
@@ -38,6 +41,7 @@ describe('spancontext-utils', () => {
     const spanContext = {
       traceId: 'd4cda95b652f4a1592b449d5929fda1b',
       spanId: context.INVALID_SPANID,
+      traceFlags: TraceFlags.NONE,
     };
     assert.ok(!context.isValid(spanContext));
   });
@@ -46,6 +50,7 @@ describe('spancontext-utils', () => {
     const spanContext = {
       traceId: context.INVALID_TRACEID,
       spanId: context.INVALID_SPANID,
+      traceFlags: TraceFlags.NONE,
     };
     assert.ok(!context.isValid(spanContext));
   });

diff --git a/packages/opentelemetry-exporter-collector/src/transform.ts b/packages/opentelemetry-exporter-collector/src/transform.ts
@@ -140,8 +140,8 @@ export function toCollectorLinkType(
   span: ReadableSpan,
   link: Link
 ): collectorTypes.LinkType {
-  const linkSpanId = link.spanContext.spanId;
-  const linkTraceId = link.spanContext.traceId;
+  const linkSpanId = link.context.spanId;
+  const linkTraceId = link.context.traceId;
   const spanParentId = span.parentSpanId;
   const spanTraceId = span.spanContext.traceId;
 
@@ -158,8 +158,8 @@ export function toCollectorLinkType(
 export function toCollectorLinks(span: ReadableSpan): collectorTypes.Links {
   const collectorLinks: collectorTypes.Link[] = span.links.map((link: Link) => {
     const collectorLink: collectorTypes.Link = {
-      traceId: hexToBase64(link.spanContext.traceId),
-      spanId: hexToBase64(link.spanContext.spanId),
+      traceId: hexToBase64(link.context.traceId),
+      spanId: hexToBase64(link.context.spanId),
       type: toCollectorLinkType(span, link),
     };
 

diff --git a/packages/opentelemetry-exporter-collector/test/helper.ts b/packages/opentelemetry-exporter-collector/test/helper.ts
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import { TraceFlags } from '@opentelemetry/api';
 import * as core from '@opentelemetry/core';
 import { ReadableSpan } from '@opentelemetry/tracing';
 import * as assert from 'assert';
@@ -27,7 +28,7 @@ export const mockedReadableSpan: ReadableSpan = {
   spanContext: {
     traceId: '1f1008dc8e270e85c40a0d7c3939b278',
     spanId: '5e107261f64fa53e',
-    traceFlags: 1,
+    traceFlags: TraceFlags.SAMPLED,
   },
   parentSpanId: '78a8915098864388',
   startTime: [1574120165, 429803070],
@@ -36,10 +37,9 @@ export const mockedReadableSpan: ReadableSpan = {
   attributes: { component: 'document-load' },
   links: [
     {
-      spanContext: {
+      context: {
         traceId: '1f1008dc8e270e85c40a0d7c3939b278',
         spanId: '78a8915098864388',
-        traceFlags: 1,
       },
       attributes: { component: 'document-load' },
     },

diff --git a/packages/opentelemetry-exporter-jaeger/src/transform.ts b/packages/opentelemetry-exporter-jaeger/src/transform.ts
@@ -103,12 +103,12 @@ function spanLinksToThriftRefs(
 ): ThriftReference[] {
   return links
     .map((link): ThriftReference | null => {
-      if (link.spanContext.spanId === parentSpanId) {
+      if (link.context.spanId === parentSpanId) {
         const refType = ThriftReferenceType.CHILD_OF;
-        const traceId = link.spanContext.traceId;
+        const traceId = link.context.traceId;
         const traceIdHigh = Utils.encodeInt64(traceId.slice(0, 16));
         const traceIdLow = Utils.encodeInt64(traceId.slice(16));
-        const spanId = Utils.encodeInt64(link.spanContext.spanId);
+        const spanId = Utils.encodeInt64(link.context.spanId);
         return { traceIdLow, traceIdHigh, spanId, refType };
       }
       return null;

diff --git a/packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts b/packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts
@@ -21,6 +21,7 @@ import * as types from '@opentelemetry/api';
 import { ThriftProcess } from '../src/types';
 import { ReadableSpan } from '@opentelemetry/tracing';
 import { ExportResult } from '@opentelemetry/base';
+import { TraceFlags } from '@opentelemetry/api';
 
 describe('JaegerExporter', () => {
   describe('constructor', () => {
@@ -110,6 +111,7 @@ describe('JaegerExporter', () => {
       const spanContext = {
         traceId: 'd4cda95b652f4a1592b449d5929fda1b',
         spanId: '6e0c63257de34c92',
+        traceFlags: TraceFlags.NONE,
       };
       const readableSpan: ReadableSpan = {
         name: 'my-span1',