Add EventGrid distributed tracing by YijunXieMS · Pull Request #15850 · Azure/azure-sdk-for-java

YijunXieMS · 2020-10-01T00:40:42Z

Add distributed tracing to EventGridEvent, CloudEvent and system events.
For CloudEvent, If the tracestate/traceparent are not populated in the event, then we want to copy the traceparent/tracestate into the CloudEvent extensions. Closes Distributed Tracing in EventGrid #14851
Fixed NullPointerException bug for CloudEvent.getExtensionAttributes() and addExtensionAttributes().
Update version number data type from Integer to Long for two classes AcsChatMessageEventBaseProperties and AcsChatThreadEventBaseProperties because the swagger file is changed. Closes Make EventGrid ACS chat events "version" property as Long #15848

samvaity · 2020-10-01T00:51:45Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

            .collectList()
-            .flatMap(list -> this.impl.publishCloudEventEventsAsync(this.hostname, list, context));
+            .flatMap(list -> this.impl.publishCloudEventEventsAsync(this.hostname, list,
+                context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE)));


Make sure to check context for null on the calling function to avoid NPE when doing addData

Does withContext guarantee context to be not null?

In some cases, we end up calling this from the sync client and hence the context could be null. Need a check there when passing.

samvaity · 2020-10-01T00:52:39Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+                    event.addExtensionAttribute(Constants.TRACE_PARENT, Constants.TRACE_PARENT_PLACEHOLDER);
+                    event.addExtensionAttribute(Constants.TRACE_STATE, Constants.TRACE_STATE_PLACEHOLDER);


Do we need both?

I put the two placeholder separately. After placeholder replacement, whatever in the request headers will be put into the body eventually. There won't be two in the header has just one.

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

.../main/java/com/azure/messaging/eventgrid/systemevents/AcsChatMessageEventBaseProperties.java

srnagar · 2020-10-01T00:53:17Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+    // Please see <a href=https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/azure-services-resource-providers>here</a>
+    // for more information on Azure resource provider namespaces.


Not sure how this resource provide namespace link is relevant to the constructor.

It's for a constant, which is copied to Constants.java but I didn't copy this together. Good catch.

srnagar · 2020-10-01T00:54:58Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

            .collectList()
-            .flatMap(list -> this.impl.publishEventsAsync(this.hostname, list, context));
+            .flatMap(list -> this.impl.publishEventsAsync(this.hostname,
+                list, context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE)));


Should have a null check for context before using it.

This internal api is called by a public API, which calls FluxUtil.withContext to create a Context. So I assume the context won't be null.
In debugging, I see it's an empty Context instance instead of null.

@YijunXieMS - this method is also called from sync client and the user can pass a null context.

User can call sendEventsWithResponse(events, null)

public Response<Void> sendEventsWithResponse(Iterable<EventGridEvent> events, Context context) { return asyncClient.sendEventsWithResponse(events, context).block(); }

srnagar · 2020-10-01T00:58:47Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

+
+    private void addCloudEventTracePlaceHolder(Iterable<CloudEvent> events) {
+        if (TracerProxy.isTracingEnabled()) {
+            for (CloudEvent event : events) {


events can be null since the public APIs don't seem to check. It might be better to have the null check and include an error message to indicate that events cannot be null.

Added null check like in EventHubs

srnagar · 2020-10-01T00:59:45Z

.../main/java/com/azure/messaging/eventgrid/implementation/CloudEventTracingPipelinePolicy.java

+import java.nio.charset.StandardCharsets;
+
+
+public class CloudEventTracingPipelinePolicy implements HttpPipelinePolicy {


Add javadoc

srnagar · 2020-10-01T01:08:36Z

.../main/java/com/azure/messaging/eventgrid/implementation/CloudEventTracingPipelinePolicy.java

+    }
+
+    static String replaceTracingPlaceHolder(HttpRequest request, ByteBuffer byteBuffer) {
+        String bodyString = new String(byteBuffer.array(), StandardCharsets.UTF_8);


Can the Flux stream have strings that are split across multiple ByteBuffer boundaries?

Changed to use a StringBuilder to take everything from the Flux. Good question.

srnagar · 2020-10-01T01:14:21Z

.../main/java/com/azure/messaging/eventgrid/implementation/CloudEventTracingPipelinePolicy.java

+        String bodyString = new String(byteBuffer.array(), StandardCharsets.UTF_8);
+        final HttpHeader traceparentHeader = request.getHeaders().get(Constants.TRACE_PARENT);
+        final HttpHeader tracestateHeader = request.getHeaders().get(Constants.TRACE_STATE);
+        bodyString = bodyString.replace(Constants.TRACE_PARENT_REPLACE,


It is not very clear why we are replacing the trace names. It might be good to add some documentation for this method.

Added Javadoc and some comments.

srnagar · 2020-10-01T22:07:40Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

            .collectList()
-            .flatMap(list -> this.impl.publishEventsAsync(this.hostname, list, context));
+            .flatMap(list -> this.impl.publishEventsAsync(this.hostname,
+                list, context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE)));


@YijunXieMS - this method is also called from sync client and the user can pass a null context.

User can call sendEventsWithResponse(events, null)

public Response<Void> sendEventsWithResponse(Iterable<EventGridEvent> events, Context context) { return asyncClient.sendEventsWithResponse(events, context).block(); }

srnagar · 2020-10-01T22:08:19Z

...ing-eventgrid/src/main/java/com/azure/messaging/eventgrid/EventGridPublisherAsyncClient.java

            .collectList()
-            .flatMap(list -> this.impl.publishEventsWithResponseAsync(this.hostname, list, context));
+            .flatMap(list -> this.impl.publishEventsWithResponseAsync(this.hostname, list,
+                context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE)));


I think my comment about user passing null for context applies to this method.

.../main/java/com/azure/messaging/eventgrid/implementation/CloudEventTracingPipelinePolicy.java

samvaity

👍

YijunXieMS · 2020-10-02T02:03:12Z

/azp run java - eventgrid - tests

azure-pipelines · 2020-10-02T02:03:22Z

Azure Pipelines successfully started running 1 pipeline(s).

YijunXieMS added 3 commits September 30, 2020 16:25

Add EventGrid distributed tracing

6c08b4c

Change version number from Integer to Long

c7749ba

Test different headers

66070e9

YijunXieMS added Event Grid Client This issue points to a problem in the data-plane of the library. labels Oct 1, 2020

YijunXieMS added this to the [2020] October milestone Oct 1, 2020

YijunXieMS requested review from JoshLove-msft, ellismg, lmazuel, rakshith91 and srnagar October 1, 2020 00:40

YijunXieMS self-assigned this Oct 1, 2020

YijunXieMS requested a review from samvaity October 1, 2020 00:47

samvaity reviewed Oct 1, 2020

View reviewed changes

srnagar reviewed Oct 1, 2020

View reviewed changes

YijunXieMS added 2 commits October 1, 2020 13:58

Update for code review comments

e8f0686

Add profiles

6cf5d79

srnagar approved these changes Oct 1, 2020

View reviewed changes

samvaity reviewed Oct 1, 2020

View reviewed changes

.../main/java/com/azure/messaging/eventgrid/implementation/CloudEventTracingPipelinePolicy.java Show resolved Hide resolved

YijunXieMS added 2 commits October 1, 2020 16:16

Correct package name

823f9e2

Add null check for context

fb6b320

samvaity approved these changes Oct 1, 2020

View reviewed changes

YijunXieMS added 3 commits October 1, 2020 16:39

Disable maven-surefire-plugin parallel

0b52b7b

Disable maven-surefire-plugin parallel

bb47152

opens com.azure.messaging.eventgrid.implementation in module-info.java

42be188

YijunXieMS merged commit eaf0db9 into Azure:master Oct 2, 2020

YijunXieMS deleted the eg_tracing branch October 2, 2020 03:43

		event.addExtensionAttribute(Constants.TRACE_PARENT, Constants.TRACE_PARENT_PLACEHOLDER);
		event.addExtensionAttribute(Constants.TRACE_STATE, Constants.TRACE_STATE_PLACEHOLDER);

		// Please see <a href=https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/azure-services-resource-providers>here</a>
		// for more information on Azure resource provider namespaces.

		import java.nio.charset.StandardCharsets;


		public class CloudEventTracingPipelinePolicy implements HttpPipelinePolicy {

Conversation

YijunXieMS commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srnagar Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samvaity left a comment

Choose a reason for hiding this comment

Uh oh!

YijunXieMS commented Oct 2, 2020

Uh oh!

azure-pipelines bot commented Oct 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srnagar Oct 1, 2020 •

edited

Loading