Skip to content

Commit b892efe

Browse files
richardpark-msftrichardpark-msft
authored
[eventhubs,service-bus]: re-add legacy tracing options (Azure#14113)
Adding in some compat code to handle converting from the older span structure to the newer OperationOption's based structure.
1 parent 3b8f79b commit b892efe

File tree

10 files changed

+577
-376
lines changed

10 files changed

+577
-376
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { MessagingError } from '@azure/core-amqp';
99
import { OperationTracingOptions } from '@azure/core-tracing';
1010
import { RetryMode } from '@azure/core-amqp';
1111
import { RetryOptions } from '@azure/core-amqp';
12+
import { Span } from '@opentelemetry/api';
1213
import { SpanContext } from '@opentelemetry/api';
1314
import { TokenCredential } from '@azure/core-auth';
1415
import { WebSocketImpl } from 'rhea-promise';
@@ -288,6 +289,8 @@ export { TokenCredential }
288289

289290
// @public
290291
export interface TryAddOptions {
292+
// @deprecated (undocumented)
293+
parentSpan?: Span | SpanContext;
291294
tracingOptions?: OperationTracingOptions;
292295
}
293296

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

Lines changed: 74 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT license.
33

4-
import { createSpanFunction, SpanOptions } from "@azure/core-tracing";
4+
import { createSpanFunction, SpanContext, SpanOptions } from "@azure/core-tracing";
55
import { Span, SpanKind } from "@opentelemetry/api";
6+
import { TryAddOptions } from "../eventDataBatch";
67
import { EventHubConnectionConfig } from "../eventhubConnectionConfig";
78
import { OperationOptions } from "../util/operationOptions";
89

@@ -52,3 +53,75 @@ export function createMessageSpan(
5253
kind: SpanKind.PRODUCER
5354
});
5455
}
56+
57+
/**
58+
* Converts TryAddOptions into the modern shape (OperationOptions) when needed.
59+
* (this is something we can eliminate at the next major release of EH _or_ when
60+
* we release with the GA version of opentelemetry).
61+
*
62+
* @internal
63+
*/
64+
export function convertTryAddOptionsForCompatibility(tryAddOptions: TryAddOptions): TryAddOptions {
65+
/* eslint-disable-next-line @typescript-eslint/ban-ts-comment */
66+
// @ts-ignore: parentSpan is deprecated and this is compat code to translate it until we can get rid of it.
67+
const possibleParentSpan = tryAddOptions.parentSpan;
68+
69+
/*
70+
Our goal here is to offer compatibility but there is a case where a user might accidentally pass
71+
_both_ sets of options. We'll assume they want the OperationTracingOptions code path in that case.
72+
73+
Example of accidental span passing:
74+
75+
const someOptionsPassedIntoTheirFunction = {
76+
parentSpan: span; // set somewhere else in their code
77+
}
78+
79+
function takeSomeOptionsFromSomewhere(someOptionsPassedIntoTheirFunction) {
80+
81+
batch.tryAddMessage(message, {
82+
// "runtime" blend of options from some other part of their app
83+
...someOptionsPassedIntoTheirFunction, // parentSpan comes along for the ride...
84+
85+
tracingOptions: {
86+
// thank goodness, I'm doing this right! (thinks the developer)
87+
spanOptions: {
88+
context: context
89+
}
90+
}
91+
});
92+
}
93+
94+
And now they've accidentally been opted into the legacy code path even though they think
95+
they're using the modern code path.
96+
97+
This does kick the can down the road a bit - at some point we will be putting them in this
98+
situation where things looked okay but their spans are becoming unparented but we can
99+
try to announce this (and other changes related to tracing) in our next big rev.
100+
*/
101+
102+
if (!possibleParentSpan || tryAddOptions.tracingOptions) {
103+
// assume that the options are already in the modern shape even if (possibly)
104+
// they were still specifying `parentSpan`
105+
return tryAddOptions;
106+
}
107+
108+
const convertedOptions: TryAddOptions = {
109+
...tryAddOptions,
110+
tracingOptions: {
111+
spanOptions: {
112+
parent: isSpan(possibleParentSpan) ? possibleParentSpan.context() : possibleParentSpan
113+
}
114+
}
115+
};
116+
117+
return convertedOptions;
118+
}
119+
120+
function isSpan(possibleSpan: Span | SpanContext | undefined): possibleSpan is Span {
121+
if (possibleSpan == null) {
122+
return false;
123+
}
124+
125+
const x = possibleSpan as Span;
126+
return typeof x.context === "function";
127+
}

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ import { EventData, toRheaMessage } from "./eventData";
55
import { ConnectionContext } from "./connectionContext";
66
import { MessageAnnotations, message, Message as RheaMessage } from "rhea-promise";
77
import { throwTypeErrorIfParameterMissing } from "./util/error";
8-
import { SpanContext } from "@opentelemetry/api";
8+
import { Span, SpanContext } from "@opentelemetry/api";
99
import { TRACEPARENT_PROPERTY, instrumentEventData } from "./diagnostics/instrumentEventData";
10-
import { createMessageSpan } from "./diagnostics/tracing";
10+
import { convertTryAddOptionsForCompatibility, createMessageSpan } from "./diagnostics/tracing";
1111
import { defaultDataTransformer } from "./dataTransformer";
1212
import { isDefined, isObjectWithProperties } from "./util/typeGuards";
1313
import { OperationTracingOptions } from "@azure/core-tracing";
@@ -47,6 +47,11 @@ export interface TryAddOptions {
4747
* The options to use when creating Spans for tracing.
4848
*/
4949
tracingOptions?: OperationTracingOptions;
50+
51+
/**
52+
* @deprecated Tracing options have been moved to the `tracingOptions` property.
53+
*/
54+
parentSpan?: Span | SpanContext;
5055
}
5156

5257
/**
@@ -281,6 +286,7 @@ export class EventDataBatchImpl implements EventDataBatch {
281286
*/
282287
public tryAdd(eventData: EventData, options: TryAddOptions = {}): boolean {
283288
throwTypeErrorIfParameterMissing(this._context.connectionId, "tryAdd", "eventData", eventData);
289+
options = convertTryAddOptionsForCompatibility(options);
284290

285291
// check if the event has already been instrumented
286292
const previouslyInstrumented = Boolean(

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

Lines changed: 101 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@ import {
1212
EventHubConsumerClient,
1313
EventHubProducerClient,
1414
EventPosition,
15+
OperationOptions,
1516
ReceivedEventData,
16-
SendBatchOptions
17+
SendBatchOptions,
18+
TryAddOptions
1719
} from "../../src";
1820
import {
1921
EnvVarKeys,
@@ -355,123 +357,134 @@ describe("EventHub Sender", function(): void {
355357
resetTracer();
356358
});
357359

358-
it("will not instrument already instrumented events", async function(): Promise<void> {
359-
const { tracer, resetTracer } = setTracerForTest();
360+
function legacyOptionsUsingSpanContext(rootSpan: TestSpan): Pick<TryAddOptions, "parentSpan"> {
361+
return {
362+
parentSpan: rootSpan.context()
363+
};
364+
}
360365

361-
const rootSpan = tracer.startSpan("test");
366+
function legacyOptionsUsingSpan(rootSpan: TestSpan): Pick<TryAddOptions, "parentSpan"> {
367+
return {
368+
parentSpan: rootSpan
369+
};
370+
}
362371

363-
const list = [
364-
{ name: "Albert" },
365-
{
366-
name: "Marie",
367-
properties: {
368-
[TRACEPARENT_PROPERTY]: "foo"
372+
function modernOptions(rootSpan: TestSpan): OperationOptions {
373+
return {
374+
tracingOptions: {
375+
spanOptions: {
376+
parent: rootSpan.context()
369377
}
370378
}
371-
];
379+
};
380+
}
372381

373-
const eventDataBatch = await producerClient.createBatch({
374-
partitionId: "0"
375-
});
382+
[legacyOptionsUsingSpan, legacyOptionsUsingSpanContext, modernOptions].forEach((optionsFn) => {
383+
describe(`tracing (${optionsFn.name})`, () => {
384+
it("will not instrument already instrumented events", async function(): Promise<void> {
385+
const { tracer, resetTracer } = setTracerForTest();
376386

377-
for (let i = 0; i < 2; i++) {
378-
eventDataBatch.tryAdd(
379-
{ body: `${list[i].name}`, properties: list[i].properties },
380-
{
381-
tracingOptions: {
382-
spanOptions: {
383-
parent: rootSpan.context()
387+
const rootSpan = tracer.startSpan("test");
388+
389+
const list = [
390+
{ name: "Albert" },
391+
{
392+
name: "Marie",
393+
properties: {
394+
[TRACEPARENT_PROPERTY]: "foo"
384395
}
385396
}
397+
];
398+
399+
const eventDataBatch = await producerClient.createBatch({
400+
partitionId: "0"
401+
});
402+
403+
for (let i = 0; i < 2; i++) {
404+
eventDataBatch.tryAdd(
405+
{ body: `${list[i].name}`, properties: list[i].properties },
406+
optionsFn(rootSpan)
407+
);
386408
}
387-
);
388-
}
389-
await producerClient.sendBatch(eventDataBatch);
390-
rootSpan.end();
409+
await producerClient.sendBatch(eventDataBatch);
410+
rootSpan.end();
391411

392-
const rootSpans = tracer.getRootSpans();
393-
rootSpans.length.should.equal(2, "Should only have two root spans.");
394-
rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
412+
const rootSpans = tracer.getRootSpans();
413+
rootSpans.length.should.equal(2, "Should only have two root spans.");
414+
rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
395415

396-
const expectedGraph: SpanGraph = {
397-
roots: [
398-
{
399-
name: rootSpan.name,
400-
children: [
416+
const expectedGraph: SpanGraph = {
417+
roots: [
401418
{
402-
name: "Azure.EventHubs.message",
403-
children: []
419+
name: rootSpan.name,
420+
children: [
421+
{
422+
name: "Azure.EventHubs.message",
423+
children: []
424+
}
425+
]
404426
}
405427
]
406-
}
407-
]
408-
};
428+
};
409429

410-
tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
411-
tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
412-
resetTracer();
413-
});
430+
tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
431+
tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
432+
resetTracer();
433+
});
414434

415-
it("will support tracing batch and send", async function(): Promise<void> {
416-
const { tracer, resetTracer } = setTracerForTest();
435+
it("will support tracing batch and send", async function(): Promise<void> {
436+
const { tracer, resetTracer } = setTracerForTest();
417437

418-
const rootSpan = tracer.startSpan("root");
438+
const rootSpan = tracer.startSpan("root");
419439

420-
const list = [{ name: "Albert" }, { name: "Marie" }];
440+
const list = [{ name: "Albert" }, { name: "Marie" }];
421441

422-
const eventDataBatch = await producerClient.createBatch({
423-
partitionId: "0"
424-
});
425-
for (let i = 0; i < 2; i++) {
426-
eventDataBatch.tryAdd(
427-
{ body: `${list[i].name}` },
428-
{
442+
const eventDataBatch = await producerClient.createBatch({
443+
partitionId: "0"
444+
});
445+
for (let i = 0; i < 2; i++) {
446+
eventDataBatch.tryAdd({ body: `${list[i].name}` }, optionsFn(rootSpan));
447+
}
448+
await producerClient.sendBatch(eventDataBatch, {
429449
tracingOptions: {
430450
spanOptions: {
431451
parent: rootSpan.context()
432452
}
433453
}
434-
}
435-
);
436-
}
437-
await producerClient.sendBatch(eventDataBatch, {
438-
tracingOptions: {
439-
spanOptions: {
440-
parent: rootSpan.context()
441-
}
442-
}
443-
});
444-
rootSpan.end();
454+
});
455+
rootSpan.end();
445456

446-
const rootSpans = tracer.getRootSpans();
447-
rootSpans.length.should.equal(1, "Should only have one root span.");
448-
rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
457+
const rootSpans = tracer.getRootSpans();
458+
rootSpans.length.should.equal(1, "Should only have one root span.");
459+
rootSpans[0].should.equal(rootSpan, "The root span should match what was passed in.");
449460

450-
const expectedGraph: SpanGraph = {
451-
roots: [
452-
{
453-
name: rootSpan.name,
454-
children: [
455-
{
456-
name: "Azure.EventHubs.message",
457-
children: []
458-
},
459-
{
460-
name: "Azure.EventHubs.message",
461-
children: []
462-
},
461+
const expectedGraph: SpanGraph = {
462+
roots: [
463463
{
464-
name: "Azure.EventHubs.send",
465-
children: []
464+
name: rootSpan.name,
465+
children: [
466+
{
467+
name: "Azure.EventHubs.message",
468+
children: []
469+
},
470+
{
471+
name: "Azure.EventHubs.message",
472+
children: []
473+
},
474+
{
475+
name: "Azure.EventHubs.send",
476+
children: []
477+
}
478+
]
466479
}
467480
]
468-
}
469-
]
470-
};
481+
};
471482

472-
tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
473-
tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
474-
resetTracer();
483+
tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
484+
tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
485+
resetTracer();
486+
});
487+
});
475488
});
476489

477490
it("with partition key should be sent successfully.", async function(): Promise<void> {

‎sdk/servicebus/service-bus/package.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
"extract-api": "tsc -p . && api-extractor run --local",
6363
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"samples/**/*.{ts,js}\" \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
6464
"integration-test:browser": "karma start --single-run",
65-
"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",
65+
"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\"",
6666
"integration-test": "npm run integration-test:node && npm run integration-test:browser",
6767
"lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]",
6868
"lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o service-bus-lintReport.html || exit 0",
@@ -177,4 +177,4 @@
177177
"events": "^3.0.0",
178178
"typedoc": "0.15.2"
179179
}
180-
}
180+
}

0 commit comments

Comments
 (0)