[Event Hubs] add tracing support when sending events #5207

chradek · 2019-09-20T22:39:57Z

Relates to #5180

This PR is a WIP that adds support for tracing send requests. I need some clarification on setting the send span statuses, specifically when an error is encountered.

Also waiting on ~~#5182~~ ~~#5233~~ #5283 to be merged as there are some API changes made there that will impact this PR.

xirzec

I really like this PR, just a few different thoughts

xirzec · 2019-09-24T18:13:22Z

sdk/core/core-tracing/lib/utils/extractSpanContext.ts

+ * @param traceParent Serialized span context data as a `traceparent` header value.
+ * @returns The `SpanContext` generated from the `traceparent` value.
+ */
+export function extractSpanContextFromTraceParent(traceParent: string): SpanContext {


nit: I think spanContextFromTraceParentHeader might be a slightly better name

xirzec · 2019-09-24T18:14:27Z

sdk/core/core-tracing/lib/utils/extractSpanContext.ts

@@ -0,0 +1,26 @@
+import { SpanContext } from '../interfaces/span_context';


not to dump more work on you, but I feel like this function would be a nice/easy thing to add a couple unit tests for ;)

Oh, I plan to! This PR is still in draft so not what I would consider ship-able yet!

xirzec · 2019-09-24T18:14:51Z

sdk/core/core-tracing/lib/utils/getTraceParent.ts

+ * @param spanContext Contains context for a specific span.
+ * @returns The `spanContext` represented as a `traceparent` value.
+ */
+export function getTraceParent(spanContext: SpanContext): string {


getTraceParentHeader?

xirzec · 2019-09-24T18:15:46Z

sdk/core/core-tracing/lib/utils/getTraceParent.ts

+ */
+export function getTraceParent(spanContext: SpanContext): string {
+  if (!spanContext.traceId || !spanContext.spanId) {
+    throw new Error(`Missing required fields "traceId" or "spanId" from spanContext.`);


I know it's tedious, but can you break this into two separate warnings? That way if this fails in production someone can look at the logs and know which field was missing

Related to another comment, what do you think about returning null if these checks fail, and logging the errors instead of throwing? That fits better with the overall philosophy that turning on tracing shouldn't break user's applications.

xirzec · 2019-09-24T18:16:17Z

sdk/core/core-tracing/lib/utils/getTraceParent.ts

@@ -0,0 +1,21 @@
+import { SpanContext } from "../interfaces/span_context";


should we combine these two utility functions into one ts file? I'm not sure how the other libs organize themselves

otherwise it feels like these functions should be the default export...

I think I'd prefer to combine them. I became wary of default exports as I went back and forth between commonjs and es2015 imports, but I think our packages handle those differences today without friction...

It's also a personal habit of mine to give exports their own file (rollup flattens them in the build artifact so no performance penalty) but I can adjust if others find that annoying!

I'm not totally opposed to the practice, it just felt like overkill, but maybe I go too much the other way (put everything in one module and break that out later)

xirzec · 2019-09-24T22:06:58Z

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

+
+export const TRACEPARENT_PROPERTY = "Diagnostic_Id";
+
+export function instrumentEventData(eventData: EventData, span: Span): boolean {


I'm curious, what is the success boolean used for?

xirzec · 2019-09-24T22:08:14Z

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

+  }
+
+  try {
+    const traceParent = getTraceParent(span.context());


I wonder if we should consider a way of parsing that doesn't throw, like returning null or some other sentinel value, since exceptions-as-control-flow is a little more expensive.

I agree, and think null or undefined would be good choices. I think instead of throwing an error, the getTraceParent method could log instead (I don't think logging has been added to core-tracing yet but it should eventually).

Anders would say to use undefined

xirzec · 2019-09-24T22:15:02Z

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

+      const messageSpan = createMessageSpan(options.parentSpan);
+      // Create a shallow copy of eventData and eventData.properties in case we add the diagnostic id to the properties.
+      eventData = {...eventData, properties: {...eventData.properties}};
+      isInstrumented = instrumentEventData(eventData, messageSpan);


If we didn't want instrumentEventData to mutate the object, we could just treat its input as readonly and do the object copy trick inside it and return the new version, right?

It just feels odd to have a method that mutates a passed-in argument if the only user of that method doesn't want that behavior

Yeah, I originally had that but then came across a situation where I also needed to know if the event was instrumented by the method. I didn't want to make the return type return the boolean and the event data, but maybe I can tackle the isInstrumented check another way.

Ah, I remember now. I wanted to know if an event being added to a batch via tryAdd was instrumented by instrumentEventData, or if it was already instrumented before being added. I can be smarter about handling this in tryAdd and have instrumentEventData go back to returning a new copy of EventData.

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

chradek · 2019-09-27T23:47:06Z

I think I've addressed all the feedback. Waiting on #5283 so I can use the TestTracer to write tests for the event-hubs changes.

xirzec

Looking real good! Happy to get this landed soon, sprinkling a few more comments for now.

xirzec · 2019-09-28T00:14:05Z

sdk/core/core-tracing/lib/utils/traceParentHeader.ts

+}
+
+
+const VERSION = "00";


can this be at the top of the file?

xirzec · 2019-09-28T00:15:16Z

sdk/core/core-tracing/lib/utils/traceParentHeader.ts

+  }
+
+  const flags = spanContext.traceFlags || TraceFlags.UNSAMPLED;
+  const traceFlags = (flags < 10) ? `0${flags.toString(16)}` : flags.toString(16);


maybe encode the flags at hex in the previous step to save some .toString(16) in this line?

sdk/core/core-tracing/lib/wrappers/opencensus/openCensusSpanWrapper.ts

sdk/core/core-tracing/package.json

xirzec · 2019-09-28T00:19:45Z

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

+
+  const traceParent = getTraceParentHeader(span.context());
+  if (traceParent) {
+    eventData.properties![TRACEPARENT_PROPERTY] = traceParent;


feels like good minor feedback to the TS team that we've implicitly initialized this above... 😉

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

chradek · 2019-10-01T16:24:14Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2019-10-01T16:24:23Z

Azure Pipelines successfully started running 1 pipeline(s).

xirzec

Looks good! 👍

I really like that you used chai for your test assertions, I think I'd like to see us standardize around that in other packages as well. ☕️

Left a few last comments for the road. ☄

xirzec · 2019-10-01T18:18:59Z

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

+ * The set of options to configure the behavior of `tryAdd`.
+ * - `parentSpan` : The `Span` or `SpanContext` to use as the `parent` of the span created while calling this operation.
+ */
+export interface TryAddOptions {


this same interface is defined in messageSpan.ts

It does not appear to be so.

xirzec · 2019-10-01T18:19:38Z

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

+ * The set of options to manually propagate `Span` context for distributed tracing.
+ * - `parentSpan` : The `Span` or `SpanContext` for the operation to use as a `parent` when creating its own span.
+ */
+export interface ParentSpanOptions {


this also looks familiar... maybe there are some common interfaces that need to be pulled out?

Yeah, maybe this can be pulled into core-tracing. I think core-http has something similar on RequestOptions, but this library doesn't depend on that so I duplicated it here.

xirzec · 2019-10-01T18:23:08Z

sdk/eventhub/event-hubs/test/hubruntime.spec.ts

+
+      tracer.getSpanGraph(rootSpan.context().traceId).should.eql(expectedGraph);
+      tracer.getActiveSpans().length.should.equal(0, "All spans should have had end called.");
+    });
  });
 }).timeout(60000);


kinda curious why the timeout is so big. I feel like we have a few tests that need to be sped up through mocking (though don't worry about this for this PR)

Yes, this is because currently all the tests are live tests. I'll be taking a look at mocking in the near future :)

chradek force-pushed the event-hubs-tracing branch from e0ee841 to d39c136 Compare September 24, 2019 16:44

xirzec reviewed Sep 24, 2019

View reviewed changes

chradek force-pushed the event-hubs-tracing branch from 0a0a871 to e6fd203 Compare September 27, 2019 23:45

chradek marked this pull request as ready for review September 27, 2019 23:47

chradek requested review from ramya-rao-a and ShivangiReja as code owners September 27, 2019 23:47

xirzec reviewed Sep 28, 2019

View reviewed changes

chradek added 9 commits September 30, 2019 14:50

[Event Hubs] add tracing support when sending events

1b27f2a

refactor after rebase

b6118eb

adds tracing support to eventHubClient operations

3def75a

npm format

1f52869

adds tests and logging for core-tracing

04e0bc1

fix core-tracing build to work with tsts

ffe2535

updates based on feedback

675455d

adds method to extract SpanContext from EventData

f1bd81d

adds tests for event hubs tracing

789dab6

chradek force-pushed the event-hubs-tracing branch from f3cf6a8 to 789dab6 Compare October 1, 2019 04:15

xirzec approved these changes Oct 1, 2019

View reviewed changes

chradek merged commit c28a1a5 into Azure:master Oct 1, 2019

daviwil mentioned this pull request Oct 3, 2019

[core-http] Fix broken browser bundle due to Rollup config #5383

Merged

HarshaNalluru mentioned this pull request Oct 6, 2020

[Service Bus] Tracing for send API #11651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Event Hubs] add tracing support when sending events #5207

[Event Hubs] add tracing support when sending events #5207

chradek commented Sep 20, 2019 •

edited

Loading

xirzec left a comment

xirzec Sep 24, 2019

xirzec Sep 24, 2019

chradek Sep 24, 2019

xirzec Sep 24, 2019

xirzec Sep 24, 2019

chradek Sep 24, 2019

xirzec Sep 25, 2019

xirzec Sep 24, 2019

xirzec Sep 24, 2019

chradek Sep 24, 2019

xirzec Sep 25, 2019

xirzec Sep 24, 2019

xirzec Sep 24, 2019

chradek Sep 24, 2019

xirzec Sep 25, 2019

xirzec Sep 24, 2019

chradek Sep 24, 2019

chradek Sep 24, 2019

chradek commented Sep 27, 2019

xirzec left a comment

xirzec Sep 28, 2019

xirzec Sep 28, 2019

xirzec Sep 28, 2019

chradek commented Oct 1, 2019

azure-pipelines bot commented Oct 1, 2019

xirzec left a comment

xirzec Oct 1, 2019

chradek Oct 1, 2019

xirzec Oct 1, 2019

chradek Oct 1, 2019

xirzec Oct 1, 2019

chradek Oct 1, 2019

		@@ -0,0 +1,26 @@
		import { SpanContext } from '../interfaces/span_context';

		@@ -0,0 +1,21 @@
		import { SpanContext } from "../interfaces/span_context";


		export const TRACEPARENT_PROPERTY = "Diagnostic_Id";

		export function instrumentEventData(eventData: EventData, span: Span): boolean {

[Event Hubs] add tracing support when sending events #5207

[Event Hubs] add tracing support when sending events #5207

Conversation

chradek commented Sep 20, 2019 • edited Loading

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek commented Sep 27, 2019

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek commented Oct 1, 2019

azure-pipelines bot commented Oct 1, 2019

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek commented Sep 20, 2019 •

edited

Loading