Move client span creation to decorator and automatically suppress creation of neste… #460

anuraaga · 2020-05-31T06:06:44Z

…d client spans.

This is a PoC for #440. It is common for a client request, such as an RPC, to be composed of multiple parts that are all instrumented, such as the AWS SDK using Apache HTTP client. Currently, we mark the RPC span as internal and the transport span as client, but that breaks features like service graphs which are built around the concept of server/client spans being services, not transport.

Here, I keep track of client spans and make sure another one is not created as the child of a client span. This check would have been trivial if Span presented an accessor for its kind, if this idea seems sane maybe we can ask about that. In the meantime, a weak map works.

Currently I'm suppressing the span but I also considered returning a wrapping Span which delegates everything except for end. This would allow transports to add information to the client span, for example events for sending / receiving data from the wire. It could happen in the future though since it's just enriching the data, not changing the modeling. It seems even more important if applying this similarly to server-side as it will be common for the "nested span" to have the RPC and similar important information.

Note, as the tests show, we lose the feature of modeling retries as spans for the AWS SDK. This seems OK to me, the current code actually doesn't even propagate these spans, the only span that is propagated is the SDK span (found this out while writing this code, propagating an INTERNAL span seems like it goes against the rules of tracing!). I have filed an issue a while ago about the SDK allowing header mutation per retry and it's probably better to wait for that and mark retries as events in the meantime (not implemented yet will do so if this approach is ok to proceed with).

Let me know if this approach seems reasonable at all!

…d client spans.

iNikem · 2020-05-31T09:00:46Z

Ignoring all but the first client span for a given trace seems like too radical for me. Even if we go this way, we have to provide a configuration option to disable this suppression and the corresponding documentation.

anuraaga · 2020-05-31T09:17:17Z

Agree it's a big change, and happy to have some configuration if it's still an ok direction. Also still have the idea of returning the current span itself (with a wrapper to prevent closing) if that's a better direction.

FWIW, I thought of only adding these checks to transport instrumentation (Apache, Netty, okhttp etc) but thinking about it couldn't come up with a reason why it would ever be better to have multiple client spans. If you could share some use cases, that would help me understand how to make this more general, or not we can just make it a setting, either global or per instrumentation :)

iNikem · 2020-06-01T10:35:40Z

After more careful reading of the specification I have concluded that the original proposal of @anuraaga to "keep track of client spans and make sure another one is not created as the child of a client span" is the correct one. And this aspect should be addressed uniformly. I have create #465 with my proposal to use Context as a storage mechanism for Server/Client spans. If that proposal gets approval, then only storage should be changed in this PR.

"returning a wrapping Span which delegates everything except for end", as proposed in this PR, can then be used for span augmentation as proposed in #465

trask · 2020-06-01T18:26:12Z

@iNikem @anuraaga I like the idea of using the context to store the CLIENT span in this PR 👍.

iNikem · 2020-06-01T18:39:40Z

@anuraaga What do you think about introducing Context driven storage in this PR? I then can proceed in changing all other instrumentations to use the same mechanism.

Or I just can go forward and do it myself, if you are short on time right now :)

anuraaga · 2020-06-02T05:55:22Z

Thanks - yeah let me take a stab at it, it's a good opportunity to learn even more about the codebase!

anuraaga

Took a stab, this is still PoC quality but let me know if it's the direction you were thinking. One observation is it seems fragile to make sure all code calls our context-mounting methods instead of the default ones.

anuraaga · 2020-06-02T08:13:08Z

...pentelemetry/auto/instrumentation/apachehttpclient/v4_0/ApacheHttpClientInstrumentation.java

-      // AWS calls are often signed, so we can't add headers without breaking the signature.
-      if (!awsClientCall) {
+      final Context context = ClientDecorator.withSpan(span, Context.current());
+      // TODO(anuraaga): Seems like a bug that invalid context still gets injected by the injector.


Ah noticed this is fixed in master of the SDK :)

iNikem · 2020-06-02T10:18:03Z

Took a stab, this is still PoC quality but let me know if it's the direction you were thinking. One observation is it seems fragile to make sure all code calls our context-mounting methods instead of the default ones.

Future work of "semantic tracers" should take care of that by extracting much more cross-cutting concerns such as this into 1 place

iNikem · 2020-06-02T10:20:08Z

Seems good to a quick look. I will review this more thoughtfully later today.

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

...ain/java8/io/opentelemetry/auto/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java

iNikem · 2020-06-02T11:34:16Z

I hoped that with client span available in current context, we don't need io.opentelemetry.instrumentation.awssdk.v2_2.TracingExecutionInterceptor#SPAN_ATTRIBUTE anymore. Is this the case?

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

trask · 2020-06-02T19:48:14Z

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

+      return TracingContextUtils.withSpan(clientSpan, context);
+    }
+    return TracingContextUtils.withSpan(
+        clientSpan, Context.current().withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan));


i think?

Suggested change

clientSpan, Context.current().withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan));

clientSpan, context.withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan));

trask · 2020-06-02T20:43:43Z

...pentelemetry/auto/instrumentation/apachehttpclient/v4_0/ApacheHttpClientInstrumentation.java

-      // AWS calls are often signed, so we can't add headers without breaking the signature.
-      if (!awsClientCall) {


I'm guessing you know something that we don't about this not being needed anymore? 😄

Now that there's only the SDK span and no Apache span, there's nothing to propagate here. That being said, it does seem safer to keep the check just in case something changes, at the same time, it seems pretty hacky to have the AWS-specific code here so it feels nice to remove it.

Oh, I understand now, right.

Do the aws-sdk tests protect us here? If so I'm good removing it.

Yup the tests cover it. It's how I found this line of code :)

trask · 2020-06-02T21:25:31Z

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

+            clientSpan, Context.current().withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan)));
+  }
+
+  public static Context withSpan(final Span clientSpan, final Context context) {


It looks like this is only called with Context.current(), so maybe this method isn't needed?

I noticed the same lack of symmetry in TracingContextUtils and followed the pattern. Let me know if it's worth diverging.

io.opentelemetry.trace.TracingContextUtils#withSpan can potentially be called with contexts other than current during context extraction via io.opentelemetry.trace.propagation.HttpTraceContext#extract method. This client decorator is not general purpose util, but more specific one. Thus I think we indeed can merge these two methods together.

anuraaga

Left a high level comment about how to store this client span into context, let me know if we're on the same page and then I'll proceed to cleanup / testss after that.

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

anuraaga · 2020-06-03T01:14:52Z

...pentelemetry/auto/instrumentation/apachehttpclient/v4_0/ApacheHttpClientInstrumentation.java

-      // AWS calls are often signed, so we can't add headers without breaking the signature.
-      if (!awsClientCall) {


Now that there's only the SDK span and no Apache span, there's nothing to propagate here. That being said, it does seem safer to keep the check just in case something changes, at the same time, it seems pretty hacky to have the AWS-specific code here so it feels nice to remove it.

...ain/java8/io/opentelemetry/auto/instrumentation/awssdk/v2_2/TracingExecutionInterceptor.java

trask · 2020-06-03T17:18:57Z

@anuraaga @iNikem I'm good with this approach. There were already some problems and hacky stuff around how we deal with the aws-sdk / netty interaction. Hopefully we can improve that in the future, but not an issue for this PR.

…-instr-java into suppress-nested-client-spans

iNikem · 2020-06-05T07:53:04Z

@anuraaga do you plan to convert this into proper PR?

anuraaga · 2020-06-05T07:57:10Z

Hi @nikem yeah sorry for the delay but I'm working on cleaning it up to switch it to a PR

iNikem · 2020-06-05T07:58:34Z

No problem, just checking the status :)

…adoc

anuraaga · 2020-06-05T08:42:00Z

Fixed the failing test and did some cleanups. Still need to add unit tests for the new methods in ClientDecorator, hope to have it by Monday :)

anuraaga

Thanks! Think this is more or less ready

anuraaga · 2020-06-08T04:36:37Z

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

+            clientSpan, Context.current().withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan)));
+  }
+
+  public static Context withSpan(final Span clientSpan, final Context context) {


I noticed the same lack of symmetry in TracingContextUtils and followed the pattern. Let me know if it's worth diverging.

anuraaga · 2020-06-08T06:40:45Z

Believe the test failure is #480

iNikem · 2020-06-08T06:43:51Z

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

+            clientSpan, Context.current().withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan)));
+  }
+
+  public static Context withSpan(final Span clientSpan, final Context context) {


io.opentelemetry.trace.TracingContextUtils#withSpan can potentially be called with contexts other than current during context extraction via io.opentelemetry.trace.propagation.HttpTraceContext#extract method. This client decorator is not general purpose util, but more specific one. Thus I think we indeed can merge these two methods together.

iNikem · 2020-06-08T06:44:53Z

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

+    if (!clientSpan.getContext().isValid()) {
+      return TracingContextUtils.withSpan(clientSpan, context);
+    }
+    return TracingContextUtils.withSpan(


I would extract common call to TracingContextUtils.withSpan from the if above.

...src/main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/ClientDecorator.java

...main/java/io/opentelemetry/auto/bootstrap/instrumentation/decorator/HttpClientDecorator.java

Anuraag Agrawal added 2 commits May 31, 2020 15:01

Move client span creation to decorator and suppress creation of neste…

9db9927

…d client spans.

Retry TODO

c2ee043

anuraaga mentioned this pull request Jun 1, 2020

Allow overriding Span kind for AWS SDK v2. #464

Closed

Store subtree client span in context.

d574447

anuraaga commented Jun 2, 2020

View reviewed changes

iNikem suggested changes Jun 2, 2020

View reviewed changes

trask reviewed Jun 2, 2020

View reviewed changes

anuraaga commented Jun 3, 2020

View reviewed changes

Merge branch 'master' of github.com:open-telemetry/opentelemetry-auto…

84ac809

…-instr-java into suppress-nested-client-spans

Apply new pattern to AWS V1 SDK instrumentation too, cleanup, and jav…

76b3221

…adoc

Update test

95d8115

anuraaga marked this pull request as ready for review June 8, 2020 04:38

anuraaga requested review from jkwatson and tylerbenson as code owners June 8, 2020 04:38

anuraaga commented Jun 8, 2020

View reviewed changes

Unit tests

9051e50

anuraaga force-pushed the suppress-nested-client-spans branch from c42a37e to 9051e50 Compare June 8, 2020 05:05

iNikem reviewed Jun 8, 2020

View reviewed changes

Cleanups

6f07eb9

iNikem approved these changes Jun 8, 2020

View reviewed changes

Merge branch 'master' into suppress-nested-client-spans

0e18e08

trask merged commit f13a9c4 into open-telemetry:master Jun 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move client span creation to decorator and automatically suppress creation of neste… #460

Move client span creation to decorator and automatically suppress creation of neste… #460

anuraaga commented May 31, 2020 •

edited

Loading

iNikem commented May 31, 2020

anuraaga commented May 31, 2020

iNikem commented Jun 1, 2020

trask commented Jun 1, 2020

iNikem commented Jun 1, 2020 •

edited

Loading

anuraaga commented Jun 2, 2020

anuraaga left a comment

anuraaga Jun 2, 2020

iNikem commented Jun 2, 2020

iNikem commented Jun 2, 2020

iNikem commented Jun 2, 2020

trask Jun 2, 2020

trask Jun 2, 2020

anuraaga Jun 3, 2020

trask Jun 3, 2020

anuraaga Jun 8, 2020

trask Jun 2, 2020

anuraaga Jun 8, 2020

iNikem Jun 8, 2020

anuraaga left a comment

anuraaga Jun 3, 2020

trask commented Jun 3, 2020

iNikem commented Jun 5, 2020

anuraaga commented Jun 5, 2020

iNikem commented Jun 5, 2020

anuraaga commented Jun 5, 2020

anuraaga left a comment

anuraaga Jun 8, 2020

anuraaga commented Jun 8, 2020

iNikem Jun 8, 2020

iNikem Jun 8, 2020

	clientSpan, Context.current().withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan));
	clientSpan, context.withValue(CONTEXT_CLIENT_SPAN_KEY, clientSpan));

		// AWS calls are often signed, so we can't add headers without breaking the signature.
		if (!awsClientCall) {

Move client span creation to decorator and automatically suppress creation of neste… #460

Move client span creation to decorator and automatically suppress creation of neste… #460

Conversation

anuraaga commented May 31, 2020 • edited Loading

iNikem commented May 31, 2020

anuraaga commented May 31, 2020

iNikem commented Jun 1, 2020

trask commented Jun 1, 2020

iNikem commented Jun 1, 2020 • edited Loading

anuraaga commented Jun 2, 2020

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem commented Jun 2, 2020

iNikem commented Jun 2, 2020

iNikem commented Jun 2, 2020

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

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented Jun 3, 2020

iNikem commented Jun 5, 2020

anuraaga commented Jun 5, 2020

iNikem commented Jun 5, 2020

anuraaga commented Jun 5, 2020

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented May 31, 2020 •

edited

Loading

iNikem commented Jun 1, 2020 •

edited

Loading