Client span keys: suppressing same instrumentation by lmolkova · Pull Request #3691 · open-telemetry/opentelemetry-java-instrumentation

lmolkova · 2021-07-27T22:31:07Z

This is a draft implementation of client span keys per instrumentations that suppresses spans with the same semantics from being created.
E.g.

DB (fine)
  HTTP (fine)
    HTTP (suppressed)
       user custom (fine)

Supports HTTP, RPC, DB and Messaging

Features:

when instrumentation type is set (affects client spans only), suppresses duplicate layers
nested server/consumer spans are suppressed - no change here
Generic instrumentation is never suppressed
if Instrumenter is built with no type (configurable with system property), preserves current behavior - suppresses any nested spans. Going forward we can remove current behavior and switch to suppression based on types
Instrumenter builder can figure out instrumentation type from attribute extractors (i.e. we can potentially make IntrumentationType package-private.

trask

InstrumentationType class is created to achieve extensibility and we may also use it to improve the registration of custom types. But I'm also happy to shrink it back to enum.

I sent a small PR to your PR, to simplify InstrumentationType a bit (short of making it an enum). I also like the idea of simplifying it all the way to an enum.

It would be good to have a configuration setting to suppress all nested CLIENT spans (preserving current behavior), since some backends don't handle nested CLIENT spans well yet.

trask · 2021-07-27T23:25:44Z

+  // TODO we should be able to figure it out from the schema_url?
+  // but then we'd have multiple versions with different urls
+  // and we want to suppress multiple versions of the same instrumentation
+  private final InstrumentationType instrumentationType;


I think schema_url is something like https://opentelemetry.io/schemas/1.5.0 and doesn't include the semantic convention

I think http and db schemas may be versioned differently and in the same app you may have instrumentations with different versions (e.g. auto and native or manual instrumentations).
Also schema_url is an arbitrary url that you get Tracer with, so ideally it may be somehow associated with just this instrumentation type (e.g. https://opentelemetry.io/schemas/http/1.5.0). Anyway, that's not how it works now.

trask · 2021-07-27T23:54:57Z

+      InstrumentationType instrumentationType,
      SpanNameExtractor<? super REQUEST> spanNameExtractor) {
-    return new InstrumenterBuilder<>(openTelemetry, instrumentationName, spanNameExtractor);
+    return new InstrumenterBuilder<>(openTelemetry, instrumentationName, instrumentationType, spanNameExtractor);


Since instrumentationType is only for client spans(?), it would be nice to scope it to just client instrumenters. Let's see if @anuraaga and @mateuszrzeszutek have any thoughts how to do this.

I recall we thought it would be useful to have a server span key as well, but as a way to enrich it rather than suppress other. Is there a need in java, e.g. in Spring to ever suppress server spans? I remember such cases in .NET world.
Anyway, agree that beyond client spans it's not that important

oh that makes sense, we do suppress nested SERVER spans in Java instrumentation today. i c now, since ClientSpan creates it's own context keys, those wouldn't overlap with context keys created by ServerSpan 👍

iNikem · 2021-07-28T13:02:40Z

It would be good to have a configuration setting to suppress all nested CLIENT spans (preserving current behavior), since some backends don't handle nested CLIENT spans well yet.

Also the opposite: not suppress anything.

trask · 2021-07-28T18:41:50Z

It would be good to have a configuration setting to suppress all nested CLIENT spans (preserving current behavior), since some backends don't handle nested CLIENT spans well yet.

Also the opposite: not suppress anything.

In SIG last week I thought your use case was capturing HTTP spans under Database spans (and network spans under HTTP spans in the future). And that you didn't need to capture "duplicate" HTTP client spans under another HTTP client span. Though the logic seems nicely centralized with this approach and so adding such an option seems reasonable if someone really wants that capability.

mateuszrzeszutek · 2021-07-30T11:33:03Z

+    instrumentationKeys.put(InstrumentationType.HTTP, ContextKey.named(TYPED_CONTEXT_KEY_PREFIX + InstrumentationType.HTTP));
+    instrumentationKeys.put(InstrumentationType.DB, ContextKey.named(TYPED_CONTEXT_KEY_PREFIX + InstrumentationType.DB));
+    instrumentationKeys.put(InstrumentationType.RPC, ContextKey.named(TYPED_CONTEXT_KEY_PREFIX + InstrumentationType.RPC));
+    instrumentationKeys.put(
+        InstrumentationType.MESSAGING, ContextKey.named(TYPED_CONTEXT_KEY_PREFIX + InstrumentationType.MESSAGING));


Hmm, using a map (instead of just static final fields) might make our bridging really complicated (see AgentContextStorage). Perhaps it'd be a bit simpler (from the javaagent perspective) if each InstrumentationTypea had its own ContextKey` field?

mateuszrzeszutek · 2021-07-30T13:05:28Z

Hey @lmolkova ,
I tried to implement something that solves the problems I've found: #3734
Please take a look at it, maybe you'll find some part that'll be useful in your PR.

lmolkova · 2021-08-03T06:22:21Z

Thanks everyone for comments and suggestions!

@mateuszrzeszutek, I followed your idea : #3734 in this version, please take a look

lmolkova · 2021-08-07T19:11:28Z

@trask @mateuszrzeszutek this one is ready for review, can you please take a look.

It needs #3771 to distinguish server/consumer and pass tests (thanks @mateuszrzeszutek for creating it).

trask

I sent a PR to your PR, with an idea to rename/restructure InstrumentationType/SuppressingSpanWrapper into SpanKey and SpanSuppressionStrategy.

I think the explicit ContextKey constants in the SpanKey class will make bridging easier (between user/library instrumentation and javaagent instrumentation).

mateuszrzeszutek · 2021-08-11T14:25:55Z

+import org.checkerframework.checker.nullness.qual.Nullable;
+
+// TODO make class (and fields/methods below) package-private after tracers are gone and no need for
+//  bridging from ClientSpan/ServerSpan/ConsumerSpan


I think something like ServerSpan#fromContextOrNull() will always be needed - there are many instrumentations that use it to update the server span name.

this makes sense, we can remove the comment (it was mine)

mateuszrzeszutek · 2021-08-11T14:27:50Z

+
+  // this is used instead of above, depending on the configuration value for
+  // otel.instrumentation.experimental.span-suppression-by-type
+  // named outgoing because it covers both client and producer spans


Previously we didn't suppress PRODUCER spans at all - should we maintain that behavior? Having a PRODUCER -> CLIENT relationship happens a few times in our messaging instrumentations (a producer calls a http/rpc client that actually sends the message to the broker)

I guess we never suppressed them because there as never 2 producer spans so far and if there were, we'd want to suppress nested ones? With suppression-by-type we'd not suppress HTTP client under messaging producer because types are different. You're right that without it we'd start suppressing them.

BTW, it seems messaging producer can sometimes be client, not sure if we follow it though.

A producer of a message should set the span kind to PRODUCER unless it synchronously waits for a response: then it should use CLIENT.

Anyway, I've separated producer strategy from client and we'd suppress nested producers, but never suppress CLIENT under PRODUCER.

I guess we never suppressed them because there as never 2 producer spans so far and if there were, we'd want to suppress nested ones?

I that case we probably would want to suppress them.

BTW, it seems messaging producer can sometimes be client, not sure if we follow it though.

🤯
I don't think that we follow it; we always use PRODUCER for all outgoing messaging spans. (Anyway, that's another reason why not using the span kind when suppressing might be a good idea)

mateuszrzeszutek · 2021-08-11T14:35:33Z

+    boolean shouldSuppress(SpanKind spanKind, Context parentContext) {
+      switch (spanKind) {


I'd love to remove the association between span kind and span suppression at some point in the future - but I guess that's still impossible at this point. Maybe when we split our AttributesExtractor implementations into server/client? We've been thinking of doing that for HTTP, we could as well do the same thing for other conventions.

it seems we'd have to specialize instrumenters (so specific instumenter instance work for one span kind) to remove the association between kind/suppression. Or perhaps, if we move CompositeStrategy into the instrumenter and it would have 4 of them - one per kind, it would not remove the association but maybe a bit cleaner.
Either way, a combination of type + kind is important for suppression (i.e. server HTTP span should not suppress client HTTP span)

Another thought is that all strategies can be the same regardless of kind - suppress same type+kind (or suppress the same kind if type disabled) - it removes the association. I assume it is rare to have nested SERVERs of different types and we can easily allow that.

mateuszrzeszutek · 2021-08-11T15:01:38Z

Sorry for being a bit late, but between some additional surprise work and GH issues I didn't manage to look at it yesterday 🙇‍♂️

Can you rebase your branch with main? My spring-integration PR has been merged recently, so rebasing should make your PR a bit easier to review.

trask

thx @lmolkova!!

I have some vague thoughts around naming changes, but will keep thinking and possibly propose some naming changes in a follow-up PR, e.g.

calling the options "single"/"layered"/"all" (I'm hoping no one really wants the "all", but we'll see...)
the SpanSuppressionStrategy class is more than just about span suppression, it's also about populating span keys for later enrichment, but I'm not sure how to convey both of those purposes. one possible answer to the naming problem is to split SpanSuppressionStratety into two classes, one that handles suppression and one that handles span key management.

trask · 2021-08-12T04:42:21Z

+import org.checkerframework.checker.nullness.qual.Nullable;
+
+// TODO make class (and fields/methods below) package-private after tracers are gone and no need for
+//  bridging from ClientSpan/ServerSpan/ConsumerSpan


this makes sense, we can remove the comment (it was mine)

mateuszrzeszutek · 2021-08-12T09:29:51Z

the SpanSuppressionStrategy class is more than just about span suppression, it's also about populating span keys for later enrichment, but I'm not sure how to convey both of those purposes. one possible answer to the naming problem is to split SpanSuppressionStratety into two classes, one that handles suppression and one that handles span key management.

We pretty much only use ServerSpan and it's just for updating the span name. I don't have any strong opinion on this, but I keep wondering whether we should introduce the LocalRootSpan after all - not for suppression, just for keeping it in context so that it's easy to retrieve it (and rename it).

anuraaga

Just some small points, thanks!

anuraaga · 2021-08-13T03:28:03Z

+  private static final boolean ENABLE_SPAN_SUPPRESSION_BY_TYPE =
+      Config.get()
+          .getBooleanProperty(
+              "otel.instrumentation.experimental.outgoing-span-suppression-by-type", false);


Since this can be overridden through direct call to enableInstrumentationTypeSuppression I think we should use default- in the property name. Or maybe can remove that method

removing enableInstrumentationTypeSuppression method sounds good 👍

removing this method makes suppression untestable through instrumetner API. The method is package-private, though, so instrumentations won't be able to call it.

trask · 2021-08-14T20:44:53Z

🥳

trask reviewed Jul 27, 2021

View reviewed changes

trask mentioned this pull request Jul 29, 2021

Add option to create span on new netty connection #3707

Merged

mateuszrzeszutek reviewed Jul 30, 2021

View reviewed changes

mateuszrzeszutek mentioned this pull request Jul 30, 2021

An alternative idea of how to suppress client spans per instrumentati… #3734

Closed

lmolkova force-pushed the clientSpanKeys branch from e693cd4 to 59b9054 Compare August 3, 2021 06:13

lmolkova force-pushed the clientSpanKeys branch from 59b9054 to f2c8f05 Compare August 3, 2021 06:24

lmolkova commented Aug 3, 2021

View reviewed changes

Comment thread .../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/SuppressingSpanWrapper.java Outdated

lmolkova commented Aug 3, 2021

View reviewed changes

Comment thread ...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java Outdated

lmolkova commented Aug 3, 2021

View reviewed changes

Comment thread instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/BaseTracer.java Outdated

trask mentioned this pull request Aug 5, 2021

Port opentelemetry-annotations-1.0 to Instrumenter API #3738

Merged

lmolkova force-pushed the clientSpanKeys branch 3 times, most recently from 7486c13 to 691263f Compare August 7, 2021 17:54

lmolkova marked this pull request as ready for review August 7, 2021 19:09

lmolkova requested review from anuraaga, iNikem, jkwatson, laurit, pavolloffay and tylerbenson as code owners August 7, 2021 19:09

trask reviewed Aug 9, 2021

View reviewed changes

Comment thread ...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java Outdated

mateuszrzeszutek reviewed Aug 11, 2021

View reviewed changes

lmolkova force-pushed the clientSpanKeys branch from c056510 to bc7ef1f Compare August 11, 2021 18:46

trask approved these changes Aug 12, 2021

View reviewed changes

mateuszrzeszutek approved these changes Aug 12, 2021

View reviewed changes

Liudmila Molkova added 6 commits August 12, 2021 15:08

type-vased suppression for client spans

28f6d9a

review

680a930

review

b9a4cd4

style checks

d0c7bd6

up

9609f2f

minor review comments

186ca58

lmolkova force-pushed the clientSpanKeys branch from 5ad442f to 186ca58 Compare August 12, 2021 22:08

anuraaga approved these changes Aug 13, 2021

View reviewed changes

review

1397805

trask merged commit 080c85d into open-telemetry:main Aug 14, 2021

mateuszrzeszutek mentioned this pull request Apr 1, 2022

Enable span suppression by type (SpanKey) by default #5735

Closed

		boolean shouldSuppress(SpanKind spanKind, Context parentContext) {
		switch (spanKind) {

Uh oh!

Conversation

lmolkova commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask left a comment

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!

lmolkova Jul 28, 2021 • 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!

iNikem commented Jul 28, 2021

Uh oh!

trask commented Jul 28, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mateuszrzeszutek commented Jul 30, 2021

Uh oh!

lmolkova commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lmolkova commented Aug 7, 2021

Uh oh!

trask left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova Aug 13, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek commented Aug 11, 2021

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mateuszrzeszutek commented Aug 12, 2021

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

lmolkova commented Jul 27, 2021 •

edited

Loading

lmolkova Jul 28, 2021 •

edited

Loading

lmolkova Aug 13, 2021 •

edited

Loading