[eventhubs,service-bus]: re-add legacy tracing options (#14113)

Adding in some compat code to handle converting from the older span structure to the newer OperationOption's based structure.

sdk/eventhub/event-hubs/review/event-hubs.api.md

@@ -9,6 +9,7 @@ import { MessagingError } from '@azure/core-amqp';
 import { OperationTracingOptions } from '@azure/core-tracing';
 import { RetryMode } from '@azure/core-amqp';
 import { RetryOptions } from '@azure/core-amqp';
+import { Span } from '@opentelemetry/api';
 import { SpanContext } from '@opentelemetry/api';
 import { TokenCredential } from '@azure/core-auth';
 import { WebSocketImpl } from 'rhea-promise';
@@ -288,6 +289,8 @@ export { TokenCredential }
 
 // @public
 export interface TryAddOptions {
+    // @deprecated (undocumented)
+    parentSpan?: Span | SpanContext;
     tracingOptions?: OperationTracingOptions;
 }
 

sdk/eventhub/event-hubs/src/diagnostics/tracing.ts

@@ -1,8 +1,9 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { createSpanFunction, SpanOptions } from "@azure/core-tracing";
+import { createSpanFunction, SpanContext, SpanOptions } from "@azure/core-tracing";
 import { Span, SpanKind } from "@opentelemetry/api";
+import { TryAddOptions } from "../eventDataBatch";
 import { EventHubConnectionConfig } from "../eventhubConnectionConfig";
 import { OperationOptions } from "../util/operationOptions";
 
@@ -52,3 +53,75 @@ export function createMessageSpan(
     kind: SpanKind.PRODUCER
   });
 }
+
+/**
+ * Converts TryAddOptions into the modern shape (OperationOptions) when needed.
+ * (this is something we can eliminate at the next major release of EH _or_ when
+ * we release with the GA version of opentelemetry).
+ *
+ * @internal
+ */
+export function convertTryAddOptionsForCompatibility(tryAddOptions: TryAddOptions): TryAddOptions {
+  /* eslint-disable-next-line @typescript-eslint/ban-ts-comment */
+  // @ts-ignore: parentSpan is deprecated and this is compat code to translate it until we can get rid of it.
+  const possibleParentSpan = tryAddOptions.parentSpan;
+
+  /*
+    Our goal here is to offer compatibility but there is a case where a user might accidentally pass
+    _both_ sets of options. We'll assume they want the OperationTracingOptions code path in that case.
+
+    Example of accidental span passing:
+
+    const someOptionsPassedIntoTheirFunction = {
+       parentSpan: span;      // set somewhere else in their code
+    }
+
+    function takeSomeOptionsFromSomewhere(someOptionsPassedIntoTheirFunction) {
+      
+      batch.tryAddMessage(message, { 
+        // "runtime" blend of options from some other part of their app
+        ...someOptionsPassedIntoTheirFunction,      // parentSpan comes along for the ride...
+
+        tracingOptions: {
+          // thank goodness, I'm doing this right! (thinks the developer)
+          spanOptions: {
+            context: context
+          }
+        }
+      });
+    }
+
+    And now they've accidentally been opted into the legacy code path even though they think
+    they're using the modern code path.
+    
+    This does kick the can down the road a bit - at some point we will be putting them in this
+    situation where things looked okay but their spans are becoming unparented but we can 
+    try to announce this (and other changes related to tracing) in our next big rev.
+  */
+
+  if (!possibleParentSpan || tryAddOptions.tracingOptions) {
+    // assume that the options are already in the modern shape even if (possibly)
+    // they were still specifying `parentSpan`
+    return tryAddOptions;
+  }
+
+  const convertedOptions: TryAddOptions = {
+    ...tryAddOptions,
+    tracingOptions: {
+      spanOptions: {
+        parent: isSpan(possibleParentSpan) ? possibleParentSpan.context() : possibleParentSpan
+      }
+    }
+  };
+
+  return convertedOptions;
+}
+
+function isSpan(possibleSpan: Span | SpanContext | undefined): possibleSpan is Span {
+  if (possibleSpan == null) {
+    return false;
+  }
+
+  const x = possibleSpan as Span;
+  return typeof x.context === "function";
+}

sdk/eventhub/event-hubs/src/eventDataBatch.ts

@@ -5,9 +5,9 @@ import { EventData, toRheaMessage } from "./eventData";
 import { ConnectionContext } from "./connectionContext";
 import { MessageAnnotations, message, Message as RheaMessage } from "rhea-promise";
 import { throwTypeErrorIfParameterMissing } from "./util/error";
-import { SpanContext } from "@opentelemetry/api";
+import { Span, SpanContext } from "@opentelemetry/api";
 import { TRACEPARENT_PROPERTY, instrumentEventData } from "./diagnostics/instrumentEventData";
-import { createMessageSpan } from "./diagnostics/tracing";
+import { convertTryAddOptionsForCompatibility, createMessageSpan } from "./diagnostics/tracing";
 import { defaultDataTransformer } from "./dataTransformer";
 import { isDefined, isObjectWithProperties } from "./util/typeGuards";
 import { OperationTracingOptions } from "@azure/core-tracing";
@@ -47,6 +47,11 @@ export interface TryAddOptions {
    * The options to use when creating Spans for tracing.
    */
   tracingOptions?: OperationTracingOptions;
+
+  /**
+   * @deprecated Tracing options have been moved to the `tracingOptions` property.
+   */
+  parentSpan?: Span | SpanContext;
 }
 
 /**
@@ -281,6 +286,7 @@ export class EventDataBatchImpl implements EventDataBatch {
    */
   public tryAdd(eventData: EventData, options: TryAddOptions = {}): boolean {
     throwTypeErrorIfParameterMissing(this._context.connectionId, "tryAdd", "eventData", eventData);
+    options = convertTryAddOptionsForCompatibility(options);
 
     // check if the event has already been instrumented
     const previouslyInstrumented = Boolean(

sdk/eventhub/event-hubs/test/internal/sender.spec.ts

@@ -12,8 +12,10 @@ import {
   EventHubConsumerClient,
   EventHubProducerClient,
   EventPosition,
+  OperationOptions,
   ReceivedEventData,
-  SendBatchOptions
+  SendBatchOptions,
+  TryAddOptions
 } from "../../src";
 import {
   EnvVarKeys,
@@ -355,123 +357,134 @@ describe("EventHub Sender", function(): void {
       resetTracer();
     });
 
-    it("will not instrument already instrumented events", async function(): Promise<void> {
-      const { tracer, resetTracer } = setTracerForTest();
+    function legacyOptionsUsingSpanContext(rootSpan: TestSpan): Pick<TryAddOptions, "parentSpan"> {
+      return {
+        parentSpan: rootSpan.context()
+      };
+    }
 
-      const rootSpan = tracer.startSpan("test");
+    function legacyOptionsUsingSpan(rootSpan: TestSpan): Pick<TryAddOptions, "parentSpan"> {
+      return {
+        parentSpan: rootSpan
+      };
+    }
 
-      const list = [
-        { name: "Albert" },
-        {
-          name: "Marie",
-          properties: {
-            [TRACEPARENT_PROPERTY]: "foo"
+    function modernOptions(rootSpan: TestSpan): OperationOptions {
+      return {
+        tracingOptions: {
+          spanOptions: {
+            parent: rootSpan.context()
           }
         }
-      ];
+      };
+    }
 
-      const eventDataBatch = await producerClient.createBatch({
-        partitionId: "0"
-      });
+    [legacyOptionsUsingSpan, legacyOptionsUsingSpanContext, modernOptions].forEach((optionsFn) => {
+      describe(`tracing (${optionsFn.name})`, () => {
+        it("will not instrument already instrumented events", async function(): Promise<void> {
+          const { tracer, resetTracer } = setTracerForTest();
 
-      for (let i = 0; i < 2; i++) {
-        eventDataBatch.tryAdd(
-          { body: `${list[i].name}`, properties: list[i].properties },
-          {
-            tracingOptions: {
-              spanOptions: {
-                parent: rootSpan.context()
+          const rootSpan = tracer.startSpan("test");
+
+          const list = [
+            { name: "Albert" },
+            {
+              name: "Marie",
+              properties: {
+                [TRACEPARENT_PROPERTY]: "foo"
               }
             }
+          ];
+
+          const eventDataBatch = await producerClient.createBatch({
+            partitionId: "0"
+          });
+
+          for (let i = 0; i < 2; i++) {
+            eventDataBatch.tryAdd(
+              { body: `${list[i].name}`, properties: list[i].properties },
+              optionsFn(rootSpan)
+            );
           }
-        );
-      }
-      await producerClient.sendBatch(eventDataBatch);
-      rootSpan.end();
+          await producerClient.sendBatch(eventDataBatch);
+          rootSpan.end();
 
-      const rootSpans = tracer.getRootSpans();
-      rootSpans.length.should.equal(2, "Should only have two root spans.");
-      rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
+          const rootSpans = tracer.getRootSpans();
+          rootSpans.length.should.equal(2, "Should only have two root spans.");
+          rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
 
-      const expectedGraph: SpanGraph = {
-        roots: [
-          {
-            name: rootSpan.name,
-            children: [
+          const expectedGraph: SpanGraph = {
+            roots: [
               {
-                name: "Azure.EventHubs.message",
-                children: []
+                name: rootSpan.name,
+                children: [
+                  {
+                    name: "Azure.EventHubs.message",
+                    children: []
+                  }
+                ]
               }
             ]
-          }
-        ]
-      };
+          };
 
-      tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
-      tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
-      resetTracer();
-    });
+          tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
+          tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
+          resetTracer();
+        });
 
-    it("will support tracing batch and send", async function(): Promise<void> {
-      const { tracer, resetTracer } = setTracerForTest();
+        it("will support tracing batch and send", async function(): Promise<void> {
+          const { tracer, resetTracer } = setTracerForTest();
 
-      const rootSpan = tracer.startSpan("root");
+          const rootSpan = tracer.startSpan("root");
 
-      const list = [{ name: "Albert" }, { name: "Marie" }];
+          const list = [{ name: "Albert" }, { name: "Marie" }];
 
-      const eventDataBatch = await producerClient.createBatch({
-        partitionId: "0"
-      });
-      for (let i = 0; i < 2; i++) {
-        eventDataBatch.tryAdd(
-          { body: `${list[i].name}` },
-          {
+          const eventDataBatch = await producerClient.createBatch({
+            partitionId: "0"
+          });
+          for (let i = 0; i < 2; i++) {
+            eventDataBatch.tryAdd({ body: `${list[i].name}` }, optionsFn(rootSpan));
+          }
+          await producerClient.sendBatch(eventDataBatch, {
             tracingOptions: {
               spanOptions: {
                 parent: rootSpan.context()
               }
             }
-          }
-        );
-      }
-      await producerClient.sendBatch(eventDataBatch, {
-        tracingOptions: {
-          spanOptions: {
-            parent: rootSpan.context()
-          }
-        }
-      });
-      rootSpan.end();
+          });
+          rootSpan.end();
 
-      const rootSpans = tracer.getRootSpans();
-      rootSpans.length.should.equal(1, "Should only have one root span.");
-      rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
+          const rootSpans = tracer.getRootSpans();
+          rootSpans.length.should.equal(1, "Should only have one root span.");
+          rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
 
-      const expectedGraph: SpanGraph = {
-        roots: [
-          {
-            name: rootSpan.name,
-            children: [
-              {
-                name: "Azure.EventHubs.message",
-                children: []
-              },
-              {
-                name: "Azure.EventHubs.message",
-                children: []
-              },
+          const expectedGraph: SpanGraph = {
+            roots: [
               {
-                name: "Azure.EventHubs.send",
-                children: []
+                name: rootSpan.name,
+                children: [
+                  {
+                    name: "Azure.EventHubs.message",
+                    children: []
+                  },
+                  {
+                    name: "Azure.EventHubs.message",
+                    children: []
+                  },
+                  {
+                    name: "Azure.EventHubs.send",
+                    children: []
+                  }
+                ]
               }
             ]
-          }
-        ]
-      };
+          };
 
-      tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
-      tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
-      resetTracer();
+          tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
+          tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
+          resetTracer();
+        });
+      });
     });
 
     it("with partition key should be sent successfully.", async function(): Promise<void> {

sdk/servicebus/service-bus/package.json

@@ -62,7 +62,7 @@
     "extract-api": "tsc -p . && api-extractor run --local",
     "format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"samples/**/*.{ts,js}\" \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
     "integration-test:browser": "karma start --single-run",
-    "integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 120000 --full-trace dist-esm/test/internal/**/*.spec.js dist-esm/test/public/**/*.spec.js",
+    "integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 120000 --full-trace \"dist-esm/test/internal/**/*.spec.js\" \"dist-esm/test/public/**/*.spec.js\"",
     "integration-test": "npm run integration-test:node && npm run integration-test:browser",
     "lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]",
     "lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o service-bus-lintReport.html || exit 0",
@@ -177,4 +177,4 @@
     "events": "^3.0.0",
     "typedoc": "0.15.2"
   }
-}
+}