-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add EventGrid distributed tracing #15850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
6c08b4c
c7749ba
66070e9
e8f0686
6cf5d79
823f9e2
fb6b320
0b52b7b
bb47152
42be188
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,15 @@ | |
| import com.azure.core.http.rest.Response; | ||
| import com.azure.core.util.Context; | ||
| import com.azure.core.util.serializer.SerializerAdapter; | ||
| import com.azure.core.util.tracing.TracerProxy; | ||
| import com.azure.messaging.eventgrid.implementation.Constants; | ||
| import com.azure.messaging.eventgrid.implementation.EventGridPublisherClientImpl; | ||
| import com.azure.messaging.eventgrid.implementation.EventGridPublisherClientImplBuilder; | ||
| import reactor.core.publisher.Flux; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| import static com.azure.core.util.FluxUtil.withContext; | ||
| import static com.azure.core.util.tracing.Tracer.AZ_TRACING_NAMESPACE_KEY; | ||
|
|
||
| /** | ||
| * A service client that publishes events to an EventGrid topic or domain. Use {@link EventGridPublisherClientBuilder} | ||
|
|
@@ -33,6 +36,9 @@ public final class EventGridPublisherAsyncClient { | |
|
|
||
| private final EventGridServiceVersion serviceVersion; | ||
|
|
||
| // 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. | ||
|
|
||
|
samvaity marked this conversation as resolved.
Outdated
|
||
| EventGridPublisherAsyncClient(HttpPipeline pipeline, String hostname, SerializerAdapter serializerAdapter, | ||
| EventGridServiceVersion serviceVersion) { | ||
| this.impl = new EventGridPublisherClientImplBuilder() | ||
|
|
@@ -70,7 +76,8 @@ Mono<Void> sendEvents(Iterable<EventGridEvent> events, Context context) { | |
| return Flux.fromIterable(events) | ||
| .map(EventGridEvent::toImpl) | ||
| .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))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should have a null check for context before using it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This internal api is called by a public API, which calls
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @YijunXieMS - this method is also called from sync client and the user can pass a null context. User can call public Response<Void> sendEventsWithResponse(Iterable<EventGridEvent> events, Context context) {
return asyncClient.sendEventsWithResponse(events, context).block();
} |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -85,10 +92,12 @@ public Mono<Void> sendCloudEvents(Iterable<CloudEvent> events) { | |
| } | ||
|
|
||
| Mono<Void> sendCloudEvents(Iterable<CloudEvent> events, Context context) { | ||
| this.addCloudEventTracePlaceHolder(events); | ||
| return Flux.fromIterable(events) | ||
| .map(CloudEvent::toImpl) | ||
| .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))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure to check context for null on the calling function to avoid NPE when doing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In some cases, we end up calling this from the sync client and hence the |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -105,7 +114,8 @@ public Mono<Void> sendCustomEvents(Iterable<Object> events) { | |
| Mono<Void> sendCustomEvents(Iterable<Object> events, Context context) { | ||
| return Flux.fromIterable(events) | ||
| .collectList() | ||
| .flatMap(list -> this.impl.publishCustomEventEventsAsync(this.hostname, list, context)); | ||
| .flatMap(list -> this.impl.publishCustomEventEventsAsync(this.hostname, list, | ||
| context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE))); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -123,7 +133,8 @@ Mono<Response<Void>> sendEventsWithResponse(Iterable<EventGridEvent> events, Con | |
| return Flux.fromIterable(events) | ||
| .map(EventGridEvent::toImpl) | ||
| .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))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my comment about user passing null for context applies to this method. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -138,10 +149,12 @@ public Mono<Response<Void>> sendCloudEventsWithResponse(Iterable<CloudEvent> eve | |
| } | ||
|
|
||
| Mono<Response<Void>> sendCloudEventsWithResponse(Iterable<CloudEvent> events, Context context) { | ||
| this.addCloudEventTracePlaceHolder(events); | ||
| return Flux.fromIterable(events) | ||
| .map(CloudEvent::toImpl) | ||
| .collectList() | ||
| .flatMap(list -> this.impl.publishCloudEventEventsWithResponseAsync(this.hostname, list, context)); | ||
| .flatMap(list -> this.impl.publishCloudEventEventsWithResponseAsync(this.hostname, list, | ||
| context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE))); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -158,6 +171,21 @@ public Mono<Response<Void>> sendCustomEventsWithResponse(Iterable<Object> events | |
| Mono<Response<Void>> sendCustomEventsWithResponse(Iterable<Object> events, Context context) { | ||
| return Flux.fromIterable(events) | ||
| .collectList() | ||
| .flatMap(list -> this.impl.publishCustomEventEventsWithResponseAsync(this.hostname, list, context)); | ||
| .flatMap(list -> this.impl.publishCustomEventEventsWithResponseAsync(this.hostname, list, | ||
| context.addData(AZ_TRACING_NAMESPACE_KEY, Constants.EVENT_GRID_TRACING_NAMESPACE_VALUE))); | ||
| } | ||
|
|
||
| private void addCloudEventTracePlaceHolder(Iterable<CloudEvent> events) { | ||
| if (TracerProxy.isTracingEnabled()) { | ||
| for (CloudEvent event : events) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added null check like in EventHubs |
||
| if (event.getExtensionAttributes() == null || | ||
| (event.getExtensionAttributes().get(Constants.TRACE_PARENT) == null && | ||
| event.getExtensionAttributes().get(Constants.TRACE_STATE) == null)) { | ||
|
|
||
| event.addExtensionAttribute(Constants.TRACE_PARENT, Constants.TRACE_PARENT_PLACEHOLDER); | ||
| event.addExtensionAttribute(Constants.TRACE_STATE, Constants.TRACE_STATE_PLACEHOLDER); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need both?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package com.azure.messaging.eventgrid.implementation; | ||
|
|
||
| import com.azure.core.http.HttpHeader; | ||
| import com.azure.core.http.HttpPipelineCallContext; | ||
| import com.azure.core.http.HttpPipelineNextPolicy; | ||
| import com.azure.core.http.HttpRequest; | ||
| import com.azure.core.http.HttpResponse; | ||
| import com.azure.core.http.policy.HttpPipelinePolicy; | ||
| import com.azure.core.util.tracing.TracerProxy; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| import java.nio.ByteBuffer; | ||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
|
|
||
| public class CloudEventTracingPipelinePolicy implements HttpPipelinePolicy { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add javadoc
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||
| @Override | ||
| public Mono<HttpResponse> process(HttpPipelineCallContext context, HttpPipelineNextPolicy next) { | ||
| final HttpRequest request = context.getHttpRequest(); | ||
| final HttpHeader contentType = request.getHeaders().get(Constants.CONTENT_TYPE); | ||
| if (TracerProxy.isTracingEnabled() && contentType != null && | ||
| Constants.CLOUD_EVENT_CONTENT_TYPE.equals(contentType.getValue())) { | ||
| return request.getBody().map(byteBuffer -> | ||
| replaceTracingPlaceHolder(request, byteBuffer)).then(next.process()); | ||
| } | ||
| else { | ||
| return next.process(); | ||
| } | ||
| } | ||
|
|
||
| static String replaceTracingPlaceHolder(HttpRequest request, ByteBuffer byteBuffer) { | ||
| String bodyString = new String(byteBuffer.array(), StandardCharsets.UTF_8); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the Flux stream have strings that are split across multiple ByteBuffer boundaries?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to use a StringBuilder to take everything from the Flux. Good question. |
||
| 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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not very clear why we are replacing the trace names. It might be good to add some documentation for this method.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added Javadoc and some comments. |
||
| traceparentHeader != null | ||
| ? String.format(",\"%s\":\"%s\"", Constants.TRACE_PARENT, | ||
| traceparentHeader.getValue()) : ""); | ||
| bodyString = bodyString.replace(Constants.TRACE_STATE_REPLACE, | ||
| tracestateHeader != null | ||
| ? String.format(",\"%s\":\"%s\"", Constants.TRACE_STATE, tracestateHeader.getValue()) : ""); | ||
| request.setHeader(Constants.CONTENT_LENGTH, String.valueOf(bodyString.length())); | ||
| request.setBody(bodyString); | ||
| return bodyString; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package com.azure.messaging.eventgrid.implementation; | ||
|
|
||
| public class Constants { | ||
| public static final String CONTENT_TYPE = "Content-Type"; | ||
| public static final String CONTENT_LENGTH = "Content-Length"; | ||
| public static final String CLOUD_EVENT_CONTENT_TYPE = "application/cloudevents-batch+json; charset=utf-8"; | ||
| public static final String TRACE_PARENT = "traceparent"; | ||
| public static final String TRACE_STATE = "tracestate"; | ||
| public static final String TRACE_PARENT_PLACEHOLDER = "TP-14b6b15b-74b6-4178-847e-d142aa2727b2"; | ||
| public static final String TRACE_STATE_PLACEHOLDER = "TS-14b6b15b-74b6-4178-847e-d142aa2727b2"; | ||
| public static final String TRACE_PARENT_REPLACE = ",\"" + TRACE_PARENT + "\":\"TP-14b6b15b-74b6-4178-847e-d142aa2727b2\""; | ||
| public static final String TRACE_STATE_REPLACE = ",\"" + TRACE_STATE + "\":\"TS-14b6b15b-74b6-4178-847e-d142aa2727b2\""; | ||
| public static final String EVENT_GRID_TRACING_NAMESPACE_VALUE = "Microsoft.EventGrid"; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package com.azure.messaging.eventgrid.implementation; | ||
|
|
||
| import com.azure.core.http.HttpMethod; | ||
| import com.azure.core.http.HttpRequest; | ||
| import org.junit.jupiter.api.Assertions; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.net.MalformedURLException; | ||
| import java.net.URL; | ||
| import java.nio.ByteBuffer; | ||
|
|
||
| public class CloudEventTracingPipelinePolicyTests { | ||
| private HttpRequest httpRequest; | ||
|
|
||
| @BeforeEach | ||
| public void setup() throws MalformedURLException { | ||
| httpRequest = new HttpRequest(HttpMethod.POST, new URL("https://something.com")); | ||
| } | ||
|
|
||
| @Test | ||
| void processBodyWithNoHeader() { | ||
| String testBodyString = "[{\"id\":\"313ac785-2dca-467e-a6a7-623f1baa2890\",\"source\":\"source\"," + | ||
| "\"type\":\"json\",\"specversion\":\"1.0\",\"tracestate\":\"TS-14b6b15b-74b6-4178-847e-d142aa2727b2\"," + | ||
| "\"traceparent\":\"TP-14b6b15b-74b6-4178-847e-d142aa2727b2\"}]"; | ||
| String expectedNewBody = "[{\"id\":\"313ac785-2dca-467e-a6a7-623f1baa2890\",\"source\":\"source\"," + | ||
| "\"type\":\"json\",\"specversion\":\"1.0\"}]"; | ||
|
|
||
| httpRequest.setBody(testBodyString); | ||
| httpRequest.setHeader(Constants.CONTENT_LENGTH, testBodyString); | ||
| String newBody = CloudEventTracingPipelinePolicy.replaceTracingPlaceHolder( | ||
| httpRequest, ByteBuffer.wrap(testBodyString.getBytes())); | ||
| Assertions.assertEquals(expectedNewBody, newBody); | ||
| Assertions.assertEquals(httpRequest.getHeaders().get(Constants.CONTENT_LENGTH).getValue(), | ||
| String.valueOf(newBody.length())); | ||
| } | ||
|
|
||
| @Test | ||
| void processBodyWithTraceParentHeader() { | ||
| httpRequest.setHeader(Constants.TRACE_PARENT, "aTraceParent"); | ||
| String testBodyString = "[{\"id\":\"313ac785-2dca-467e-a6a7-623f1baa2890\",\"source\":\"source\"," + | ||
| "\"type\":\"json\",\"specversion\":\"1.0\",\"tracestate\":\"TS-14b6b15b-74b6-4178-847e-d142aa2727b2\"," + | ||
| "\"traceparent\":\"TP-14b6b15b-74b6-4178-847e-d142aa2727b2\"}]"; | ||
| String expectedNewBody = "[{\"id\":\"313ac785-2dca-467e-a6a7-623f1baa2890\",\"source\":\"source\"," + | ||
| "\"type\":\"json\",\"specversion\":\"1.0\"," + | ||
| "\"traceparent\":\"aTraceParent\"}]"; | ||
| httpRequest.setBody(testBodyString); | ||
| httpRequest.setHeader(Constants.CONTENT_LENGTH, testBodyString); | ||
| String newBody = CloudEventTracingPipelinePolicy.replaceTracingPlaceHolder( | ||
| httpRequest, ByteBuffer.wrap(testBodyString.getBytes())); | ||
| Assertions.assertEquals(expectedNewBody, newBody); | ||
| Assertions.assertEquals(httpRequest.getHeaders().get(Constants.CONTENT_LENGTH).getValue(), | ||
| String.valueOf(newBody.length())); | ||
| } | ||
|
|
||
| @Test | ||
| void processBodyWithTraceStateHeader() { | ||
| httpRequest.setHeader(Constants.TRACE_STATE, "aTraceState"); | ||
| String testBodyString = "[{\"id\":\"313ac785-2dca-467e-a6a7-623f1baa2890\",\"source\":\"source\"," + | ||
| "\"type\":\"json\",\"specversion\":\"1.0\",\"tracestate\":\"TS-14b6b15b-74b6-4178-847e-d142aa2727b2\"," + | ||
| "\"traceparent\":\"TP-14b6b15b-74b6-4178-847e-d142aa2727b2\"}]"; | ||
| String expectedNewBody = "[{\"id\":\"313ac785-2dca-467e-a6a7-623f1baa2890\",\"source\":\"source\"," + | ||
| "\"type\":\"json\",\"specversion\":\"1.0\"," + | ||
| "\"tracestate\":\"aTraceState\"}]"; | ||
|
|
||
| httpRequest.setBody(testBodyString); | ||
| httpRequest.setHeader(Constants.CONTENT_LENGTH, testBodyString); | ||
| String newBody = CloudEventTracingPipelinePolicy.replaceTracingPlaceHolder( | ||
| httpRequest, ByteBuffer.wrap(testBodyString.getBytes())); | ||
| Assertions.assertEquals(expectedNewBody, newBody); | ||
| Assertions.assertEquals(httpRequest.getHeaders().get(Constants.CONTENT_LENGTH).getValue(), | ||
| String.valueOf(newBody.length())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this resource provide namespace link is relevant to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for a constant, which is copied to Constants.java but I didn't copy this together. Good catch.