[core-rest-pipeline] Update to core-tracing 1.0.0 GA (#21264)

Migrate core-rest-pipeline to core-tracing 1.0 GA and expose new type guard for RestError.

dataplane.code-workspace

@@ -279,6 +279,9 @@
     },
     {
       "path": "sdk/purview/purview-administration-rest"
+    },
+    {
+      "path": "sdk/instrumentation/opentelemetry-instrumentation-azure-sdk"
     }
   ],
   "settings": {

sdk/core/core-rest-pipeline/CHANGELOG.md

@@ -4,12 +4,18 @@
 
 ### Features Added
 
+- Exposed type guard for RestError called `isRestError` for typesafe exception handling.
+
 ### Breaking Changes
 
 ### Bugs Fixed
 
 ### Other Changes
 
+- Updated our `@azure/core-tracing` dependency to the latest version (1.0.0).
+  - Notable changes include Removal of `@opentelemetry/api` as a transitive dependency and ensuring that the active context is properly propagated.
+  - Customers who would like to continue using OpenTelemetry driven tracing should visit our [OpenTelemetry Instrumentation](https://www.npmjs.com/package/@azure/opentelemetry-instrumentation-azure-sdk) package for instructions.
+
 ## 1.8.0 (2022-03-31)
 
 ### Features Added

sdk/core/core-rest-pipeline/package.json

@@ -88,7 +88,7 @@
   "dependencies": {
     "@azure/abort-controller": "^1.0.0",
     "@azure/core-auth": "^1.3.0",
-    "@azure/core-tracing": "1.0.0-preview.13",
+    "@azure/core-tracing": "^1.0.0",
     "@azure/logger": "^1.0.0",
     "form-data": "^4.0.0",
     "tslib": "^2.2.0",

sdk/core/core-rest-pipeline/review/core-rest-pipeline.api.md

@@ -150,6 +150,9 @@ export interface InternalPipelineOptions extends PipelineOptions {
     loggingOptions?: LogPolicyOptions;
 }
 
+// @public
+export function isRestError(e: unknown): e is RestError;
+
 // @public
 export function logPolicy(options?: LogPolicyOptions): PipelinePolicy;
 

sdk/core/core-rest-pipeline/src/index.ts

@@ -33,7 +33,7 @@ export {
 export { createDefaultHttpClient } from "./defaultHttpClient";
 export { createHttpHeaders } from "./httpHeaders";
 export { createPipelineRequest, PipelineRequestOptions } from "./pipelineRequest";
-export { RestError, RestErrorOptions } from "./restError";
+export { RestError, RestErrorOptions, isRestError } from "./restError";
 export {
   decompressResponsePolicy,
   decompressResponsePolicyName,

sdk/core/core-rest-pipeline/src/policies/tracingPolicy.ts

@@ -2,23 +2,18 @@
 // Licensed under the MIT license.
 
 import {
-  Span,
-  SpanOptions,
-  SpanStatusCode,
-  createSpanFunction,
-  getTraceParentHeader,
-  isSpanContextValid,
+  TracingSpan,
+  createTracingClient,
+  TracingClient,
+  TracingContext,
 } from "@azure/core-tracing";
-import { SpanKind } from "@azure/core-tracing";
+import { SDK_VERSION } from "../constants";
 import { PipelineRequest, PipelineResponse, SendRequest } from "../interfaces";
 import { PipelinePolicy } from "../pipeline";
 import { getUserAgentValue } from "../util/userAgent";
 import { logger } from "../log";
-
-const createSpan = createSpanFunction({
-  packagePrefix: "",
-  namespace: "",
-});
+import { isError, getErrorMessage } from "../util/helpers";
+import { isRestError } from "../restError";
 
 /**
  * The programmatic identifier of the tracingPolicy.
@@ -45,22 +40,23 @@ export interface TracingPolicyOptions {
  */
 export function tracingPolicy(options: TracingPolicyOptions = {}): PipelinePolicy {
   const userAgent = getUserAgentValue(options.userAgentPrefix);
+  const tracingClient = tryCreateTracingClient();
 
   return {
     name: tracingPolicyName,
     async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
-      if (!request.tracingOptions?.tracingContext) {
+      if (!tracingClient || !request.tracingOptions?.tracingContext) {
         return next(request);
       }
 
-      const span = tryCreateSpan(request, userAgent);
+      const { span, tracingContext } = tryCreateSpan(tracingClient, request, userAgent) ?? {};
 
-      if (!span) {
+      if (!span || !tracingContext) {
         return next(request);
       }
 
       try {
-        const response = await next(request);
+        const response = await tracingClient.withContext(tracingContext, next, request);
         tryProcessResponse(span, response);
         return response;
       } catch (err) {
@@ -71,19 +67,38 @@ export function tracingPolicy(options: TracingPolicyOptions = {}): PipelinePolic
   };
 }
 
-function tryCreateSpan(request: PipelineRequest, userAgent?: string): Span | undefined {
+function tryCreateTracingClient(): TracingClient | undefined {
   try {
-    const createSpanOptions: SpanOptions = {
-      ...(request.tracingOptions as any)?.spanOptions,
-      kind: SpanKind.CLIENT,
-    };
-
-    // Passing spanOptions as part of tracingOptions to maintain compatibility @azure/core-tracing@preview.13 and earlier.
-    // We can pass this as a separate parameter once we upgrade to the latest core-tracing.
-    // As per spec, we do not need to differentiate between HTTP and HTTPS in span name.
-    const { span } = createSpan(`HTTP ${request.method}`, {
-      tracingOptions: { ...request.tracingOptions, spanOptions: createSpanOptions },
+    return createTracingClient({
+      namespace: "",
+      packageName: "@azure/core-rest-pipeline",
+      packageVersion: SDK_VERSION,
     });
+  } catch (e: unknown) {
+    logger.warning(`Error when creating the TracingClient: ${getErrorMessage(e)}`);
+    return undefined;
+  }
+}
+
+function tryCreateSpan(
+  tracingClient: TracingClient,
+  request: PipelineRequest,
+  userAgent?: string
+): { span: TracingSpan; tracingContext: TracingContext } | undefined {
+  try {
+    // As per spec, we do not need to differentiate between HTTP and HTTPS in span name.
+    const { span, updatedOptions } = tracingClient.startSpan(
+      `HTTP ${request.method}`,
+      { tracingOptions: request.tracingOptions },
+      {
+        spanKind: "client",
+        spanAttributes: {
+          "http.method": request.method,
+          "http.url": request.url,
+          requestId: request.requestId,
+        },
+      }
+    );
 
     // If the span is not recording, don't do any more work.
     if (!span.isRecording()) {
@@ -91,58 +106,40 @@ function tryCreateSpan(request: PipelineRequest, userAgent?: string): Span | und
       return undefined;
     }
 
-    const namespaceFromContext = request.tracingOptions?.tracingContext?.getValue(
-      Symbol.for("az.namespace")
-    );
-
-    if (typeof namespaceFromContext === "string") {
-      span.setAttribute("az.namespace", namespaceFromContext);
-    }
-
-    span.setAttributes({
-      "http.method": request.method,
-      "http.url": request.url,
-      requestId: request.requestId,
-    });
-
     if (userAgent) {
       span.setAttribute("http.user_agent", userAgent);
     }
 
     // set headers
-    const spanContext = span.spanContext();
-    const traceParentHeader = getTraceParentHeader(spanContext);
-    if (traceParentHeader && isSpanContextValid(spanContext)) {
-      request.headers.set("traceparent", traceParentHeader);
-      const traceState = spanContext.traceState && spanContext.traceState.serialize();
-      // if tracestate is set, traceparent MUST be set, so only set tracestate after traceparent
-      if (traceState) {
-        request.headers.set("tracestate", traceState);
-      }
+    const headers = tracingClient.createRequestHeaders(
+      updatedOptions.tracingOptions.tracingContext
+    );
+    for (const [key, value] of Object.entries(headers)) {
+      request.headers.set(key, value);
     }
-    return span;
-  } catch (error) {
-    logger.warning(`Skipping creating a tracing span due to an error: ${error.message}`);
+    return { span, tracingContext: updatedOptions.tracingOptions.tracingContext };
+  } catch (e) {
+    logger.warning(`Skipping creating a tracing span due to an error: ${getErrorMessage(e)}`);
     return undefined;
   }
 }
 
-function tryProcessError(span: Span, err: any): void {
+function tryProcessError(span: TracingSpan, error: unknown): void {
   try {
     span.setStatus({
-      code: SpanStatusCode.ERROR,
-      message: err.message,
+      status: "error",
+      error: isError(error) ? error : undefined,
     });
-    if (err.statusCode) {
-      span.setAttribute("http.status_code", err.statusCode);
+    if (isRestError(error) && error.statusCode) {
+      span.setAttribute("http.status_code", error.statusCode);
     }
     span.end();
-  } catch (error) {
-    logger.warning(`Skipping tracing span processing due to an error: ${error.message}`);
+  } catch (e) {
+    logger.warning(`Skipping tracing span processing due to an error: ${getErrorMessage(e)}`);
   }
 }
 
-function tryProcessResponse(span: Span, response: PipelineResponse): void {
+function tryProcessResponse(span: TracingSpan, response: PipelineResponse): void {
   try {
     span.setAttribute("http.status_code", response.status);
     const serviceRequestId = response.headers.get("x-ms-request-id");
@@ -150,10 +147,10 @@ function tryProcessResponse(span: Span, response: PipelineResponse): void {
       span.setAttribute("serviceRequestId", serviceRequestId);
     }
     span.setStatus({
-      code: SpanStatusCode.OK,
+      status: "success",
     });
     span.end();
-  } catch (error) {
-    logger.warning(`Skipping tracing span processing due to an error: ${error.message}`);
+  } catch (e) {
+    logger.warning(`Skipping tracing span processing due to an error: ${getErrorMessage(e)}`);
   }
 }

sdk/core/core-rest-pipeline/src/restError.ts

@@ -4,6 +4,7 @@
 import { PipelineRequest, PipelineResponse } from "./interfaces";
 import { custom } from "./util/inspect";
 import { Sanitizer } from "./util/sanitizer";
+import { isError } from "./util/helpers";
 
 const errorSanitizer = new Sanitizer();
 
@@ -84,3 +85,14 @@ export class RestError extends Error {
     return `RestError: ${this.message} \n ${errorSanitizer.sanitize(this)}`;
   }
 }
+
+/**
+ * Typeguard for RestError
+ * @param e Something caught by a catch clause.
+ */
+export function isRestError(e: unknown): e is RestError {
+  if (e instanceof RestError) {
+    return true;
+  }
+  return isError(e) && e.name === "RestError";
+}

sdk/core/core-rest-pipeline/src/util/helpers.ts

@@ -122,3 +122,42 @@ export function parseHeaderValueAsNumber(
   if (Number.isNaN(valueAsNum)) return;
   return valueAsNum;
 }
+
+/**
+ * @internal
+ * Typeguard for an error object shape (has name and message)
+ * @param e Something caught by a catch clause.
+ */
+export function isError(e: unknown): e is Error {
+  if (isObject(e)) {
+    const hasName = typeof e.name === "string";
+    const hasMessage = typeof e.message === "string";
+    return hasName && hasMessage;
+  }
+  return false;
+}
+
+/**
+ * @internal
+ * Given what is thought to be an error object, return the message if possible.
+ * If the message is missing, returns a stringified version of the input.
+ * @param e Something thrown from a try{} block
+ * @returns The error message or a string of the input
+ */
+export function getErrorMessage(e: unknown): string {
+  if (isError(e)) {
+    return e.message;
+  } else {
+    let stringified: string;
+    try {
+      if (typeof e === "object" && e) {
+        stringified = JSON.stringify(e);
+      } else {
+        stringified = String(e);
+      }
+    } catch (e) {
+      stringified = "[unable to stringify input]";
+    }
+    return `Unknown error ${stringified}`;
+  }
+}

sdk/core/core-rest-pipeline/test/tracingPolicy.spec.ts

@@ -4,6 +4,7 @@
 import { assert } from "chai";
 import * as sinon from "sinon";
 import {
+  PipelineRequest,
   PipelineResponse,
   RestError,
   SendRequest,
@@ -12,217 +13,117 @@ import {
   tracingPolicy,
 } from "../src";
 import {
-  SpanAttributeValue,
-  SpanAttributes,
-  SpanContext,
-  SpanOptions,
+  Instrumenter,
+  InstrumenterSpanOptions,
   SpanStatus,
-  SpanStatusCode,
-  TraceFlags,
-  TraceState,
-  context,
-  setSpan,
+  TracingContext,
+  TracingSpan,
+  TracingSpanOptions,
+  useInstrumenter,
 } from "@azure/core-tracing";
-import { Span, Tracer, TracerProvider, trace } from "@opentelemetry/api";
 
-export class MockSpan implements Span {
-  private _endCalled = false;
-  private _status: SpanStatus = {
-    code: SpanStatusCode.UNSET,
-  };
-  private _attributes: SpanAttributes = {};
+class MockSpan implements TracingSpan {
+  spanAttributes: Record<string, unknown> = {};
+  endCalled: boolean = false;
+  status?: SpanStatus;
+  exceptions: Array<Error | string> = [];
 
-  constructor(
-    private name: string,
-    private traceId: string,
-    private spanId: string,
-    private flags: TraceFlags,
-    private state: string,
-    options?: SpanOptions
-  ) {
-    this._attributes = options?.attributes || {};
-  }
-
-  addEvent(): this {
-    throw new Error("Method not implemented.");
+  constructor(public name: string, spanOptions: TracingSpanOptions = {}) {
+    this.spanAttributes = spanOptions.spanAttributes ?? {};
   }
 
   isRecording(): boolean {
     return true;
   }
 
-  recordException(): void {
-    throw new Error("Method not implemented.");
-  }
-
-  updateName(): this {
-    throw new Error("Method not implemented.");
-  }
-
-  didEnd(): boolean {
-    return this._endCalled;
+  recordException(exception: Error | string): void {
+    this.exceptions.push(exception);
   }
 
   end(): void {
-    this._endCalled = true;
+    this.endCalled = true;
   }
 
-  getStatus(): SpanStatus {
-    return this._status;
+  setStatus(status: SpanStatus): void {
+    this.status = status;
   }
 
-  setStatus(status: SpanStatus): this {
-    this._status = status;
+  setAttribute(name: string, value: unknown): void {
+    this.spanAttributes[name] = value;
+  }
+
+  getAttribute(name: string): unknown {
+    return this.spanAttributes[name];
+  }
+}
+
+const noopTracingContext: TracingContext = {
+  deleteValue() {
     return this;
-  }
+  },
+  getValue() {
+    return undefined;
+  },
+  setValue() {
+    return this;
+  },
+};
 
-  setAttributes(attributes: SpanAttributes): this {
-    for (const key in attributes) {
-      this.setAttribute(key, attributes[key]!);
+class MockInstrumenter implements Instrumenter {
+  lastSpanCreated: MockSpan | undefined;
+  staticSpan: MockSpan | undefined;
+
+  setStaticSpan(span: MockSpan): void {
+    this.staticSpan = span;
+  }
+  startSpan(
+    name: string,
+    spanOptions: InstrumenterSpanOptions
+  ): {
+    span: TracingSpan;
+    tracingContext: TracingContext;
+  } {
+    const tracingContext = spanOptions.tracingContext ?? noopTracingContext;
+    if (this.staticSpan) {
+      return { span: this.staticSpan, tracingContext };
     }
-    return this;
-  }
-
-  setAttribute(key: string, value: SpanAttributeValue): this {
-    this._attributes[key] = value;
-    return this;
-  }
-
-  getName(): string {
-    return this.name;
-  }
-
-  getAttribute(key: string): SpanAttributeValue | undefined {
-    return this._attributes[key];
-  }
-
-  spanContext(): SpanContext {
-    const state = this.state;
-
-    const traceState = {
-      set(): TraceState {
-        /* empty */
-        return traceState;
-      },
-      unset(): TraceState {
-        /* empty */
-        return traceState;
-      },
-      get(): string | undefined {
-        return;
-      },
-      serialize() {
-        return state;
-      },
-    };
-
+    const span = new MockSpan(name, spanOptions);
+    this.lastSpanCreated = span;
     return {
-      traceId: this.traceId,
-      spanId: this.spanId,
-      traceFlags: this.flags,
-      traceState,
+      span,
+      tracingContext,
     };
   }
-}
-
-export class MockTracer implements Tracer {
-  private spans: MockSpan[] = [];
-  private _startSpanCalled = false;
-
-  constructor(
-    private traceId = "",
-    private spanId = "",
-    private flags = TraceFlags.NONE,
-    private state = ""
-  ) {}
-
-  startActiveSpan(): never {
-    throw new Error("Method not implemented.");
+  withContext<
+    CallbackArgs extends unknown[],
+    Callback extends (...args: CallbackArgs) => ReturnType<Callback>
+  >(
+    _context: TracingContext,
+    callback: Callback,
+    ...callbackArgs: CallbackArgs
+  ): ReturnType<Callback> {
+    return callback(...callbackArgs);
   }
 
-  getStartedSpans(): MockSpan[] {
-    return this.spans;
+  parseTraceparentHeader(_traceparentHeader: string): TracingContext | undefined {
+    return undefined;
   }
-
-  startSpanCalled(): boolean {
-    return this._startSpanCalled;
-  }
-
-  startSpan(name: string, options?: SpanOptions): MockSpan {
-    this._startSpanCalled = true;
-    const span = new MockSpan(name, this.traceId, this.spanId, this.flags, this.state, options);
-    this.spans.push(span);
-    return span;
+  createRequestHeaders(_tracingContext?: TracingContext): Record<string, string> {
+    return {};
   }
 }
 
-export class MockTracerProvider implements TracerProvider {
-  private mockTracer: Tracer = new MockTracer();
-
-  setTracer(tracer: Tracer): void {
-    this.mockTracer = tracer;
-  }
-
-  getTracer(): Tracer {
-    return this.mockTracer;
-  }
-
-  register(): void {
-    trace.setGlobalTracerProvider(this);
-  }
-
-  disable(): void {
-    trace.disable();
-  }
-}
-
-const ROOT_SPAN = new MockSpan("name", "root", "root", TraceFlags.SAMPLED, "");
-
 describe("tracingPolicy", function () {
-  const TRACE_VERSION = "00";
-  const mockTracerProvider = new MockTracerProvider();
-
-  beforeEach(() => {
-    mockTracerProvider.register();
-  });
-
-  afterEach(() => {
-    mockTracerProvider.disable();
-  });
-
-  it("will not create a span if tracingContext is missing", async () => {
-    const mockTracer = new MockTracer();
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-    await policy.sendRequest(request, next);
-
-    assert.isFalse(mockTracer.startSpanCalled());
-  });
-
-  it("will create a span with the correct data", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED);
-    mockTracerProvider.setTracer(mockTracer);
+  let activeInstrumenter: MockInstrumenter;
 
+  function createTestRequest({ noContext = false } = {}): {
+    request: PipelineRequest;
+    next: sinon.SinonStub;
+  } {
     const request = createPipelineRequest({
       url: "https://bing.com",
       method: "POST",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN).setValue(
-          Symbol.for("az.namespace"),
-          "test"
-        ),
-      },
+      tracingOptions: { tracingContext: noContext ? undefined : noopTracingContext },
     });
 
     const response: PipelineResponse = {
@@ -230,369 +131,110 @@ describe("tracingPolicy", function () {
       request: request,
       status: 200,
     };
-    const policy = tracingPolicy();
     const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
     next.resolves(response);
-    await policy.sendRequest(request, next);
+    return { request, next };
+  }
 
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.equal(span.getName(), "HTTP POST");
-    assert.equal(span.getAttribute("az.namespace"), "test");
-    assert.equal(span.getAttribute("http.method"), "POST");
-    assert.equal(span.getAttribute("http.url"), request.url);
-    assert.equal(span.getAttribute("requestId"), request.requestId);
-    assert.equal(span.getAttribute("http.status_code"), response.status);
+  afterEach(() => {
+    sinon.restore();
   });
 
-  it("will create a span and correctly set trace headers if tracingContext is available", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED);
-    mockTracerProvider.setTracer(mockTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-    await policy.sendRequest(request, next);
-
-    assert.isTrue(mockTracer.startSpanCalled());
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.isTrue(span.didEnd());
-    assert.deepEqual(span.getStatus(), { code: SpanStatusCode.OK });
-    assert.equal(span.getAttribute("http.status_code"), 200);
-
-    const expectedFlag = "01";
-
-    assert.equal(
-      request.headers.get("traceparent"),
-      `${TRACE_VERSION}-${mockTraceId}-${mockSpanId}-${expectedFlag}`
-    );
-    assert.notExists(request.headers.get("tracestate"));
+  beforeEach(() => {
+    activeInstrumenter = new MockInstrumenter();
+    useInstrumenter(activeInstrumenter);
   });
 
-  it("will create a span and correctly set trace headers if tracingContext is available (no TraceOptions)", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    // leave out the TraceOptions
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId);
-    mockTracerProvider.setTracer(mockTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
+  it("will create a span with the correct data", async () => {
     const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
+    const { request, next } = createTestRequest();
     await policy.sendRequest(request, next);
 
-    assert.isTrue(mockTracer.startSpanCalled());
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.isTrue(span.didEnd());
-    assert.deepEqual(span.getStatus(), { code: SpanStatusCode.OK });
-    assert.equal(span.getAttribute("http.status_code"), 200);
-
-    const expectedFlag = "00";
-
-    assert.equal(
-      request.headers.get("traceparent"),
-      `${TRACE_VERSION}-${mockTraceId}-${mockSpanId}-${expectedFlag}`
-    );
-    assert.notExists(request.headers.get("tracestate"));
+    const createdSpan = activeInstrumenter.lastSpanCreated;
+    assert.exists(createdSpan);
+    const mockSpan = createdSpan!;
+    assert.isTrue(mockSpan.endCalled, "expected span to be ended");
+    assert.equal(mockSpan.name, "HTTP POST");
+    assert.equal(mockSpan.getAttribute("http.method"), "POST");
+    assert.equal(mockSpan.getAttribute("http.url"), request.url);
+    assert.equal(mockSpan.getAttribute("requestId"), request.requestId);
+    assert.equal(mockSpan.getAttribute("http.status_code"), 200); // createTestRequest's response will return 200 OK
   });
 
-  it("will create a span and correctly set trace headers tracingContext is available (TraceState)", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTraceState = "foo=bar";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED, mockTraceState);
-    mockTracerProvider.setTracer(mockTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      method: "PUT",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
+  it("will set request headers correctly", async () => {
+    sinon.stub(activeInstrumenter, "createRequestHeaders").returns({
+      testheader: "testvalue",
     });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
+    const { request, next } = createTestRequest();
+
     const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
     await policy.sendRequest(request, next);
-
-    assert.isTrue(mockTracer.startSpanCalled());
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.isTrue(span.didEnd());
-    assert.deepEqual(span.getStatus(), { code: SpanStatusCode.OK });
-
-    const expectedFlag = "01";
-
-    assert.equal(
-      request.headers.get("traceparent"),
-      `${TRACE_VERSION}-${mockTraceId}-${mockSpanId}-${expectedFlag}`
-    );
-    assert.equal(request.headers.get("tracestate"), mockTraceState);
+    assert.equal(request.headers.get("testheader"), "testvalue");
   });
 
   it("will close a span if an error is encountered", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTraceState = "foo=bar";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED, mockTraceState);
-    mockTracerProvider.setTracer(mockTracer);
-
     const request = createPipelineRequest({
       url: "https://bing.com",
       tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
+        tracingContext: noopTracingContext,
       },
     });
+
     const policy = tracingPolicy();
     const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.rejects(new RestError("Bad Request.", { statusCode: 400 }));
+    const requestError = new RestError("Bad Request.", { statusCode: 400 });
+    next.rejects(requestError);
 
-    try {
-      await policy.sendRequest(request, next);
-      throw new Error("Test Failure");
-    } catch (err) {
-      assert.notEqual(err.message, "Test Failure");
-      assert.isTrue(mockTracer.startSpanCalled());
-      assert.lengthOf(mockTracer.getStartedSpans(), 1);
-      const span = mockTracer.getStartedSpans()[0];
-      assert.isTrue(span.didEnd());
-      assert.deepEqual(span.getStatus(), {
-        code: SpanStatusCode.ERROR,
-        message: "Bad Request.",
-      });
-      assert.equal(span.getAttribute("http.status_code"), 400);
-
-      const expectedFlag = "01";
-
-      assert.equal(
-        request.headers.get("traceparent"),
-        `${TRACE_VERSION}-${mockTraceId}-${mockSpanId}-${expectedFlag}`
-      );
-      assert.equal(request.headers.get("tracestate"), mockTraceState);
+    await assert.isRejected(policy.sendRequest(request, next), requestError);
+    const createdSpan = activeInstrumenter.lastSpanCreated;
+    assert.exists(createdSpan);
+    const mockSpan = createdSpan!;
+    assert.equal(mockSpan.status?.status, "error");
+    if (mockSpan.status?.status === "error") {
+      assert.equal(mockSpan.status?.error, requestError);
     }
+    assert.isTrue(mockSpan.endCalled, "end was expected to be called!");
+    assert.equal(mockSpan.getAttribute("http.status_code"), 400);
   });
 
-  it("will not set headers if span is a NoOpSpan", async () => {
-    mockTracerProvider.disable();
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
+  it("will not create a span if tracingContext is missing", async () => {
     const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
+    const { request, next } = createTestRequest({ noContext: true });
     await policy.sendRequest(request, next);
 
-    assert.notExists(request.headers.get("traceparent"));
-    assert.notExists(request.headers.get("tracestate"));
+    const createdSpan = activeInstrumenter.lastSpanCreated;
+    assert.notExists(createdSpan, "span was created without tracingContext being passed!");
   });
 
-  it("will not set headers if context is invalid", async () => {
-    // This will create a tracer that produces invalid trace-id and span-id
-    const mockTracer = new MockTracer("invalid", "00", TraceFlags.SAMPLED, "foo=bar");
-    mockTracerProvider.setTracer(mockTracer);
+  describe("span errors", () => {
+    it("will not fail the request when creating a span throws", async () => {
+      sinon.stub(activeInstrumenter, "startSpan").throws("boom");
+      const { request, next } = createTestRequest();
+      const policy = tracingPolicy();
 
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
+      await assert.isFulfilled(policy.sendRequest(request, next));
     });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-    await policy.sendRequest(request, next);
 
-    assert.notExists(request.headers.get("traceparent"));
-    assert.notExists(request.headers.get("tracestate"));
-  });
+    it("will not fail the request when post-processing success fails", async () => {
+      const mockSpan = sinon.createStubInstance(MockSpan);
+      mockSpan.end.throws(new Error("end is not a function"));
+      activeInstrumenter.setStaticSpan(mockSpan);
+      const { request, next } = createTestRequest();
+      const policy = tracingPolicy();
 
-  it("will not fail the request if span setup fails", async () => {
-    const errorTracer = new MockTracer("", "", TraceFlags.SAMPLED, "");
-    sinon.stub(errorTracer, "startSpan").throws(new Error("Test Error"));
-    mockTracerProvider.setTracer(errorTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
+      await assert.isFulfilled(policy.sendRequest(request, next));
     });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
 
-    // Does not throw
-    const result = await policy.sendRequest(request, next);
-    assert.equal(result, response);
-  });
+    it("will not fail the request when post-processing error fails", async () => {
+      const mockSpan = sinon.createStubInstance(MockSpan);
+      mockSpan.end.throws(new Error("end is not a function"));
+      const { request, next } = createTestRequest();
+      const policy = tracingPolicy();
+      const expectedError = new RestError("Bad Request.", { statusCode: 400 });
+      next.rejects(expectedError);
 
-  it("will not fail the request if response processing fails", async () => {
-    const errorTracer = new MockTracer("", "", TraceFlags.SAMPLED, "");
-    mockTracerProvider.setTracer(errorTracer);
-    const errorSpan = new MockSpan("", "", "", TraceFlags.SAMPLED, "");
-    sinon.stub(errorSpan, "end").throws(new Error("Test Error"));
-    sinon.stub(errorTracer, "startSpan").returns(errorSpan);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
+      // Expect the pipeline request error, _not_ the error that is thrown when ending a span.
+      await assert.isRejected(policy.sendRequest(request, next), expectedError);
     });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-
-    // Does not throw
-    const result = await policy.sendRequest(request, next);
-    assert.equal(result, response);
-  });
-
-  it("will give priority to context's az.namespace over spanOptions", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED);
-    mockTracerProvider.setTracer(mockTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN).setValue(
-          Symbol.for("az.namespace"),
-          "value_from_context"
-        ),
-      },
-    });
-    Object.assign(request.tracingOptions, {
-      spanOptions: { attributes: { "az.namespace": "value_from_span_options" } },
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-    await policy.sendRequest(request, next);
-
-    assert.isTrue(mockTracer.startSpanCalled());
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.equal(span.getAttribute("az.namespace"), "value_from_context");
-  });
-
-  it("will use spanOptions if context does not have namespace", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED);
-    mockTracerProvider.setTracer(mockTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
-    });
-    Object.assign(request.tracingOptions, {
-      spanOptions: { attributes: { "az.namespace": "value_from_span_options" } },
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-    await policy.sendRequest(request, next);
-
-    assert.isTrue(mockTracer.startSpanCalled());
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.equal(span.getAttribute("az.namespace"), "value_from_span_options");
-  });
-
-  it("is robust when spanOptions is undefined", async () => {
-    const mockTraceId = "11111111111111111111111111111111";
-    const mockSpanId = "2222222222222222";
-    const mockTracer = new MockTracer(mockTraceId, mockSpanId, TraceFlags.SAMPLED);
-    mockTracerProvider.setTracer(mockTracer);
-
-    const request = createPipelineRequest({
-      url: "https://bing.com",
-      tracingOptions: {
-        tracingContext: setSpan(context.active(), ROOT_SPAN),
-      },
-    });
-    const response: PipelineResponse = {
-      headers: createHttpHeaders(),
-      request: request,
-      status: 200,
-    };
-    const policy = tracingPolicy();
-    const next = sinon.stub<Parameters<SendRequest>, ReturnType<SendRequest>>();
-    next.resolves(response);
-    await policy.sendRequest(request, next);
-
-    assert.isTrue(mockTracer.startSpanCalled());
-    assert.lengthOf(mockTracer.getStartedSpans(), 1);
-    const span = mockTracer.getStartedSpans()[0];
-    assert.notExists(span.getAttribute("az.namespace"));
   });
 });

sdk/core/core-tracing/CHANGELOG.md

@@ -10,6 +10,8 @@
 
 ### Other Changes
 
+- Minor improvements to the typing of `updatedOptions` when calling startSpan.
+
 ## 1.0.0 (2022-03-31)
 
 This release marks the GA release of our @azure/core-tracing libraries and is unchanged from preview.14

sdk/core/core-tracing/review/core-tracing.api.md

@@ -30,6 +30,13 @@ export interface OperationTracingOptions {
     tracingContext?: TracingContext;
 }
 
+// @public
+export type OptionsWithTracingContext<Options> = Options & {
+    tracingOptions: {
+        tracingContext: TracingContext;
+    };
+};
+
 // @public
 export type Resolved<T> = T extends {
     then(onfulfilled: infer F): any;
@@ -57,7 +64,7 @@ export interface TracingClient {
         tracingOptions?: OperationTracingOptions;
     }>(name: string, operationOptions?: Options, spanOptions?: TracingSpanOptions): {
         span: TracingSpan;
-        updatedOptions: Options;
+        updatedOptions: OptionsWithTracingContext<Options>;
     };
     withContext<CallbackArgs extends unknown[], Callback extends (...args: CallbackArgs) => ReturnType<Callback>>(context: TracingContext, callback: Callback, ...callbackArgs: CallbackArgs): ReturnType<Callback>;
     withSpan<Options extends {

sdk/core/core-tracing/src/index.ts

@@ -5,6 +5,7 @@ export {
   Instrumenter,
   InstrumenterSpanOptions,
   OperationTracingOptions,
+  OptionsWithTracingContext,
   Resolved,
   SpanStatus,
   SpanStatusError,

sdk/core/core-tracing/src/interfaces.ts

@@ -62,7 +62,10 @@ export interface TracingClient {
     name: string,
     operationOptions?: Options,
     spanOptions?: TracingSpanOptions
-  ): { span: TracingSpan; updatedOptions: Options };
+  ): {
+    span: TracingSpan;
+    updatedOptions: OptionsWithTracingContext<Options>;
+  };
   /**
    * Wraps a callback with an active context and calls the callback.
    * Depending on the implementation, this may set the globally available active context.
@@ -278,3 +281,13 @@ export interface OperationTracingOptions {
   /** The context to use for created Tracing Spans. */
   tracingContext?: TracingContext;
 }
+
+/**
+ * A utility type for when we know a TracingContext has been set
+ * as part of an operation's options.
+ */
+export type OptionsWithTracingContext<Options> = Options & {
+  tracingOptions: {
+    tracingContext: TracingContext;
+  };
+};

sdk/core/core-tracing/src/tracingClient.ts

@@ -3,6 +3,7 @@
 
 import {
   OperationTracingOptions,
+  OptionsWithTracingContext,
   Resolved,
   TracingClient,
   TracingClientOptions,
@@ -28,7 +29,7 @@ export function createTracingClient(options: TracingClientOptions): TracingClien
     spanOptions?: TracingSpanOptions
   ): {
     span: TracingSpan;
-    updatedOptions: Options;
+    updatedOptions: OptionsWithTracingContext<Options>;
   } {
     const startSpanResult = getInstrumenter().startSpan(name, {
       ...spanOptions,
@@ -42,12 +43,10 @@ export function createTracingClient(options: TracingClientOptions): TracingClien
       tracingContext = tracingContext.setValue(knownContextKeys.namespace, namespace);
     }
     span.setAttribute("az.namespace", tracingContext.getValue(knownContextKeys.namespace));
-    const updatedOptions = {
-      ...operationOptions,
-      tracingOptions: {
-        tracingContext: tracingContext,
-      },
-    } as Options;
+    const updatedOptions: OptionsWithTracingContext<Options> = Object.assign({}, operationOptions, {
+      tracingOptions: { ...operationOptions?.tracingOptions, tracingContext },
+    });
+
     return {
       span,
       updatedOptions,
@@ -68,7 +67,7 @@ export function createTracingClient(options: TracingClientOptions): TracingClien
   ): Promise<Resolved<ReturnType<Callback>>> {
     const { span, updatedOptions } = startSpan(name, operationOptions, spanOptions);
     try {
-      const result = await withContext(updatedOptions.tracingOptions!.tracingContext!, () =>
+      const result = await withContext(updatedOptions.tracingOptions.tracingContext, () =>
         Promise.resolve(callback(updatedOptions, span))
       );
       span.setStatus({ status: "success" });

sdk/core/core-tracing/src/tracingContext.ts

@@ -1,14 +1,12 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { TracingClient, TracingContext, TracingSpan } from "./interfaces";
+import { TracingContext, TracingSpan } from "./interfaces";
 
 /** @internal */
 export const knownContextKeys = {
   span: Symbol.for("@azure/core-tracing span"),
   namespace: Symbol.for("@azure/core-tracing namespace"),
-  client: Symbol.for("@azure/core-tracing client"),
-  parentContext: Symbol.for("@azure/core-tracing parent context"),
 };
 
 /**
@@ -23,9 +21,6 @@ export function createTracingContext(options: CreateTracingContextOptions = {}):
   if (options.span) {
     context = context.setValue(knownContextKeys.span, options.span);
   }
-  if (options.client) {
-    context = context.setValue(knownContextKeys.client, options.client);
-  }
   if (options.namespace) {
     context = context.setValue(knownContextKeys.namespace, options.namespace);
   }
@@ -63,12 +58,10 @@ export class TracingContextImpl implements TracingContext {
  * Represents a set of items that can be set when creating a new {@link TracingContext}.
  */
 export interface CreateTracingContextOptions {
-  /** The {@link parentContext} - the newly created context will contain all the values of the parent context unless overriden. */
+  /** The {@link parentContext} - the newly created context will contain all the values of the parent context unless overridden. */
   parentContext?: TracingContext;
   /** An initial span to set on the context. */
   span?: TracingSpan;
-  /** The tracing client used to create this context. */
-  client?: TracingClient;
   /** The namespace to set on any child spans. */
   namespace?: string;
 }

sdk/core/core-tracing/test/tracingClient.spec.ts

@@ -85,10 +85,18 @@ describe("TracingClient", () => {
     });
 
     it("Returns tracingContext in updatedOptions", () => {
-      let { updatedOptions } = client.startSpan("test");
-      assert.exists(updatedOptions.tracingOptions?.tracingContext);
+      let { updatedOptions } = client.startSpan<{}>("test");
+      assert.exists(updatedOptions.tracingOptions.tracingContext);
       updatedOptions = client.startSpan("test", updatedOptions).updatedOptions;
-      assert.exists(updatedOptions.tracingOptions?.tracingContext);
+      assert.exists(updatedOptions.tracingOptions.tracingContext);
+    });
+
+    it("Does not erase unknown tracingOptions", () => {
+      // this test is to future-proof any tracingOptions we might add
+      let { updatedOptions } = client.startSpan<{}>("test", {
+        tracingOptions: { unknownProp: true } as any,
+      });
+      assert.exists((updatedOptions.tracingOptions as any).unknownProp);
     });
   });
 

sdk/core/core-tracing/test/tracingContext.spec.ts

@@ -4,7 +4,6 @@
 import { TracingContextImpl, createTracingContext, knownContextKeys } from "../src/tracingContext";
 import { assert } from "chai";
 import { createDefaultTracingSpan } from "../src/instrumenter";
-import { createTracingClient } from "../src/tracingClient";
 
 describe("TracingContext", () => {
   describe("TracingContextImpl", () => {
@@ -96,15 +95,12 @@ describe("TracingContext", () => {
     });
 
     it("can add known attributes", () => {
-      const client = createTracingClient({ namespace: "test", packageName: "test" });
       const span = createDefaultTracingSpan();
       const namespace = "test-namespace";
       const newContext = createTracingContext({
-        client,
         span,
         namespace,
       });
-      assert.strictEqual(newContext.getValue(knownContextKeys.client), client);
       assert.strictEqual(newContext.getValue(knownContextKeys.namespace), namespace);
       assert.strictEqual(newContext.getValue(knownContextKeys.span), span);
     });

sdk/instrumentation/opentelemetry-instrumentation-azure-sdk/src/instrumentation.browser.ts

@@ -46,7 +46,7 @@ class AzureSdkInstrumentation extends InstrumentationBase {
 /**
  * Enables Azure SDK Instrumentation using OpenTelemetry for Azure SDK client libraries.
  *
- * When registerd, any Azure data plane package will begin emitting tracing spans for internal calls
+ * When registered, any Azure data plane package will begin emitting tracing spans for internal calls
  * as well as network calls
  *
  * Example usage:

sdk/instrumentation/opentelemetry-instrumentation-azure-sdk/src/instrumentation.ts

@@ -58,7 +58,7 @@ class AzureSdkInstrumentation extends InstrumentationBase {
 /**
  * Enables Azure SDK Instrumentation using OpenTelemetry for Azure SDK client libraries.
  *
- * When registerd, any Azure data plane package will begin emitting tracing spans for internal calls
+ * When registered, any Azure data plane package will begin emitting tracing spans for internal calls
  * as well as network calls
  *
  * Example usage:

sdk/instrumentation/opentelemetry-instrumentation-azure-sdk/src/instrumenter.ts

@@ -47,14 +47,6 @@ export class OpenTelemetryInstrumenter implements Instrumenter {
       }
     }
 
-    // COMPAT: remove when core-rest-pipeline has upgraded to core-tracing 1.0
-    // https://github.com/Azure/azure-sdk-for-js/issues/20567
-    const newNamespaceKey = Symbol.for("@azure/core-tracing namespace");
-    const oldNamespaceKey = Symbol.for("az.namespace");
-    if (!ctx.getValue(oldNamespaceKey) && ctx.getValue(newNamespaceKey)) {
-      ctx = ctx.setValue(oldNamespaceKey, ctx.getValue(newNamespaceKey));
-    }
-
     return {
       span: new OpenTelemetrySpanWrapper(span),
       tracingContext: trace.setSpan(ctx, span),

sdk/instrumentation/opentelemetry-instrumentation-azure-sdk/test/public/instrumenter.spec.ts

@@ -88,19 +88,6 @@ describe("OpenTelemetryInstrumenter", () => {
 
         assert.equal(trace.getSpan(tracingContext), unwrap(span));
       });
-
-      it("adds az.namespace as a context attribute for compatibility", async () => {
-        const currentContext = context
-          .active()
-          .setValue(Symbol.for("@azure/core-tracing namespace"), "test-namespace");
-
-        const { tracingContext } = instrumenter.startSpan("test", {
-          tracingContext: currentContext,
-          packageName,
-        });
-
-        assert.equal(tracingContext.getValue(Symbol.for("az.namespace")), "test-namespace");
-      });
     });
 
     describe("when a context is not provided", () => {