New Instrumenter API #2596

anuraaga · 2021-03-18T10:11:57Z

Still draft quality but I think this is ready for some initial feedback

The biggest difference from the current APIs is avoiding inheritance in favor of stitching together parts. The notion is that there are only two big logic behavioral differences between instrumenters - server instrumenters extract context and client ones inject it. Other than that, it's just customization - HTTP semantic conventions drives the customization of span attributes, etc, but there is no big difference in logic it's sort of configuration. So instead of using inheritance, I encapsulate attribute extraction, span name extraction, and status extraction into composable parts. This allows two things that I think an inheritance version has trouble with

Resolve different conflicting semantic conventions. For example, AWS spans are notorious for having different functionality that needs to be reconciled per-method for determining span name from options of RPC, Messaging, Database, or Armeria also can be used both for RPC or just plain HTTP. So these different types of conventions don't fit neatly into an inheritance tree, we need to be able to compose them. I haven't added an example of this yet in this PR though
Allow user customization of points they need with builder pattern. I think this style allows keeping instrumentation classes private without losing customization, because customization can be defined in terms of the instrumentation-api. With inheritance, we'd probably need to expose instrumentation classes to allow methods to be overridden.

More than happy to hear thoughts, there are probably some terrible aspects of this too. Though most issues won't be ironed out until trying it on more instrumentation in future PRs.

/cc @as-polyakov

…nstrumentation into instrumentation-extractor

mateuszrzeszutek

Overall I really like this proposition - the composition is so much nicer (and flexible!) than the inheritance 👍 👍 👍

Various observations & ideas:

For some instrumentations (like DB instrumentations) we will probably have to create some DTOs that collect several objects into a REQUEST - I don't think that's bad though, and some instrumentations already do this because of various reasons.
Those instrumentations that have more lifecycle phases than just start & end will be a bit problematic: we'll have to work around all those loose onRequest(), onException(), onSomething() calls.

mateuszrzeszutek · 2021-03-18T15:58:09Z

...library/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/OpenTelemetryClient.java


 Span span = Span.fromContext(context);
 if (span.isRecording()) {
 ctx.log()
 .whenComplete()
 .thenAccept(
 log -> {
- clientTracer.getNetPeerAttributes().setNetPeer(span, ctx.remoteAddress());
-
 long requestEndTimeNanos = requestStartTimeNanos + log.responseDurationNanos();


I think that the Armeria instrumentation was the only one that passed the start/end timestamps manually - can we remove that safely? I think it'd be great to simplify that bit.

Decided at least for now to remove it and might rethink later

mateuszrzeszutek · 2021-03-18T16:03:53Z

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

+
+ AttributesBuilder attributesBuilder = Attributes.builder();
+ for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : extractors) {
+ extractor.onStart(attributesBuilder, request);


That's just a minor thing, but using the AttributeSetter interface could help us avoid creating additional Attributes instance and copying to the span/spanBuilder.

@anuraaga @jkwatson what do you think about moving AttributeSetter interface to the API to unify setting attributes on Span and SpanBuilder?

Another option we have is to just use SpanBuilder here, and come up with a separate solution for the 2 instrumentations that don't fit this usage.

Yeah the thought has come to mind but also with the caveat that indeed, I think it's very rare to share the same code for both SpanBuilder and Span which sounds like it's the case.

I think it's very rare to share the same code for both SpanBuilder and Span which sounds like it's the case.

That interface was added to reuse NetPeerUtils methods, which was called in various situations 😄
NetAttributesExtractor fixes that issue, so the only use case left that I can imagine is onRequest() situation - which we have to solve or find a workaround for anyway.

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

mateuszrzeszutek · 2021-03-18T17:09:06Z

...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpAttributesExtractor.java

+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+public abstract class HttpAttributesExtractor<REQUEST, RESPONSE>


Should we put HTTP-related classes in a ...instrumenter.http subpackage? Same for DB/messaging/RPC attribute/span name extractors (in the future). The base package would contain just the base classes/interfaces

Hmm - I think it means we wouldn't have SpanStatusExtractor.http() (not in same package anymore) and just use HttpStatusExtractor.create() or something. I'm wondering about this vs extracting an attributes package with HttpAttributesExtractor, NetAttributesExtractor, etc, that does keep the public API together with some separation.

Or HttpSemanticConventions.spanName()/HttpSemanticConventions.status() in the http package

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

...-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java

mateuszrzeszutek · 2021-03-18T17:44:19Z

...-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java

+import io.opentelemetry.context.propagation.ContextPropagators;
+import io.opentelemetry.context.propagation.TextMapGetter;
+
+public abstract class ServerInstrumenter<REQUEST, RESPONSE>


There's one interesting thing that I've noticed when comparing this class to HttpServerTracer: the tracer class has the STORAGE type parameter and attachServerContext() and getServerContext() methods that interact with it.
This can actually be removed from the tracer/instrumenter, it's only used for manual context propagation and I think it actually SHOULDN'T be here - that's a completely separate concern.
(Also, not all server instrumentations implement it)

...brary/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTracingBuilder.java

mateuszrzeszutek · 2021-03-19T10:59:37Z

@anuraaga I've implemented several of my comments in this PR: anuraaga#20
I'd be happy to hear some feedback & comments.
In short, I've completely removed inheritance and added a instrumenter builder that allows the API user to choose which behavior (server or client propagation, internal span) they want in their instrumenter.

trask · 2021-03-22T05:23:59Z

thx @anuraaga and @mateuszrzeszutek for giving so much thought to this!

I agree with all the big ideas 👍

once you're ready, I suggest we go for it: merge and start migrating instrumentation one-by-one, uncovering edge cases and working out the smaller stuff as we go

* New Instrumenter API - InstrumenterBuilder * New Instrumenter API - InstrumenterBuilder - code review comments

…nstrumentation into instrumentation-extractor

anuraaga · 2021-03-29T06:31:19Z

Sorry for the delay, this should be ready for review

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

bogdandrutu · 2021-03-29T16:30:37Z

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

+ Instrumenter(
+ Tracer tracer,
+ SpanNameExtractor<? super REQUEST> spanNameExtractor,
+ SpanKindExtractor<? super REQUEST> spanKindExtractor,
+ StatusExtractor<? super REQUEST, ? super RESPONSE> statusExtractor,
+ List<? extends AttributesExtractor<? super REQUEST, ? super RESPONSE>> extractors,
+ ErrorCauseExtractor errorCauseExtractor) {
+ this.tracer = tracer;
+ this.spanNameExtractor = spanNameExtractor;
+ this.spanKindExtractor = spanKindExtractor;
+ this.statusExtractor = statusExtractor;
+ this.extractors = extractors;
+ this.errorCauseExtractor = errorCauseExtractor;
+ }


As long as this is package protected passing all the arguments is fine, but if we plan to make this public would suggest to have an Option pojo to allow us to add more arguments.

This wouldn't become public, that's why we have the builder

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

mateuszrzeszutek

@anuraaga please let me know if you need any help with implementing suggestions from comments, I'd like to help with this PR.

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StatusExtractor.java

mateuszrzeszutek · 2021-03-30T09:35:26Z

...-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java

+import io.opentelemetry.context.propagation.TextMapSetter;
+import java.util.List;
+
+final class ClientInstrumenter<REQUEST, RESPONSE> extends Instrumenter<REQUEST, RESPONSE> {


We should probably rename ClientInstrumenter/ServerInstrumenter to DownstreamPropagatingInstrumenter/UpstreamPropagatingInstrumenter (also see anuraaga#20 (comment))

Ah yeah - I understood the comment, I am still on the fence on it. Considering how much more common server/client is than the other forms I wonder if it's not so bad to have server / client words even when it's actually consumer / producer. Was thinking of addressing this in a follow-up when using from messaging.

Just to clarify: we can still have newServerInstrumenter(TextMapGetter), new(Upstream|Downstream)Instrumenter() would be just for the situation when you have to pass a custom SpanKindExtractor

Yeah it's a tradeoff between introducing new concepts vs having precise terminology. If server and producer are close enough to be two ducks (i.e, if it quacks it's a duck) then consolidating might be ok. I'm not too sure on it yet.

...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

...src/test/java/io/opentelemetry/instrumentation/api/instrumenter/HttpStatusExtractorTest.java

...ntelemetry/instrumentation/api/instrumenter/InetSocketAddressNetAttributesExtractorTest.java

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy

…n/test/base/HttpClientTest.groovy Co-authored-by: Mateusz Rzeszutek <[email protected]>

anuraaga · 2021-03-30T14:41:49Z

Thanks a lot @mateuszrzeszutek - I've left some thumbs on ones that seem trivial for this PR and comments on ones I lean towards following up. If you'd like to send a pr for them sounds great or else will address tomorrow

mateuszrzeszutek · 2021-03-30T15:46:58Z

anuraaga#21

…nstrumentation into instrumentation-extractor

…elemetry-java-instrumentation into instrumentation-extractor

…nstrumentation into instrumentation-extractor

anuraaga · 2021-04-01T05:18:00Z

This should be ready for another review now, and I think it's probaby a reasonable first try to iterate on in future PRs

iNikem

I am currently rather concerned about the simplicity of building an instrumenter for a given instrumentation. E.g. when using builder it is not clear what pieces are actually mandatory: do I have to provide httpAttributesExtractor? If not how can I understand if I should? Maybe a proper example will reduce that confusion.

iNikem · 2021-04-01T05:40:36Z

...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/HttpAttributesExtractor.java

+ protected final void onStart(AttributesBuilder attributes, REQUEST request) {
+ set(attributes, SemanticAttributes.HTTP_METHOD, method(request));
+ set(attributes, SemanticAttributes.HTTP_URL, url(request));
+ set(attributes, SemanticAttributes.HTTP_TARGET, target(request));
+ set(attributes, SemanticAttributes.HTTP_HOST, host(request));
+ set(attributes, SemanticAttributes.HTTP_ROUTE, route(request));
+ set(attributes, SemanticAttributes.HTTP_SCHEME, scheme(request));
+ set(attributes, SemanticAttributes.HTTP_USER_AGENT, userAgent(request));


From http semantic convention:

more than the required attributes can be supplied, but this is recommended only if they cannot be inferred from the sent ones.

For this API, we just set what was provided indeed, I don't think we can model something like that at this layer since we don't know what can be inferred, or if we do then it needs to be more explicit in the spec I guess, that spec language is way too vague to be actually implementable I think.

WDYM, vague? Spec gives several options for required attribute combinations and then warns agains duplication.

Well these attributes are defined by the spec so it could say exactly which attributes should be filled. If it's the three sets of attributes and says attributes SHOULD NOT be filled from one set if another is filled, it would be clearer. I don't know either if this is a ordered list by priority. So vague ;)

E.g, in that text server name is mentioned descriptively but it could just be said it should always be filled when available.

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

trask

I think it's probaby a reasonable first try to iterate on in future PRs

👍

...brary/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTracingBuilder.java

trask · 2021-04-01T06:40:15Z

instrumentation/armeria-1.3/library/armeria-1.3-library.gradle

@@ -1,5 +1,5 @@
 apply from: "$rootDir/gradle/instrumentation-library.gradle"
-apply plugin: "net.ltgt.errorprone"
+// apply plugin: "net.ltgt.errorprone"


add a TODO here?

…nstrumentation into instrumentation-extractor

Anuraag Agrawal added 8 commits March 10, 2021 15:05

Instrumenter

8027c55

Spotless

b7e66a6

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

6af20de

…nstrumentation into instrumentation-extractor

More instrumenter

c1e19af

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

46a9e1c

…nstrumentation into instrumentation-extractor

More

2e6db8f

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

be06cdb

…nstrumentation into instrumentation-extractor

Instrumenter API

a91ad8e

anuraaga requested review from trask, iNikem and mateuszrzeszutek March 18, 2021 10:11

mateuszrzeszutek reviewed Mar 18, 2021

View reviewed changes

trask mentioned this pull request Mar 22, 2021

add Instrumenters support #1871

Closed

Mateusz Rzeszutek and others added 8 commits March 23, 2021 09:21

New Instrumenter API - InstrumenterBuilder (#20)

6c5e576

* New Instrumenter API - InstrumenterBuilder * New Instrumenter API - InstrumenterBuilder - code review comments

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

297f336

…nstrumentation into instrumentation-extractor

Start doccing

fbee4b0

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

2cd5e68

…nstrumentation into instrumentation-extractor

Docs

fb3038a

Most tests

fbbe2d5

Last test

cdd8078

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

fb33d75

…nstrumentation into instrumentation-extractor

anuraaga marked this pull request as ready for review March 29, 2021 06:31

anuraaga requested review from jkwatson, pavolloffay and tylerbenson as code owners March 29, 2021 06:31

Stick with current hostName lookup for now

54c3d76

bogdandrutu reviewed Mar 29, 2021

View reviewed changes

mateuszrzeszutek reviewed Mar 30, 2021

View reviewed changes

Update testing-common/src/main/groovy/io/opentelemetry/instrumentatio…

243ba5b

…n/test/base/HttpClientTest.groovy Co-authored-by: Mateusz Rzeszutek <[email protected]>

Mateusz Rzeszutek and others added 6 commits March 31, 2021 09:04

New Instrumenter API - code review comments (#21)

5babac7

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

cdf7bca

…nstrumentation into instrumentation-extractor

Doc

b32c9a0

Merge branch 'instrumentation-extractor' of github.com:anuraaga/opent…

36f5fa6

…elemetry-java-instrumentation into instrumentation-extractor

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

0d5ec54

…nstrumentation into instrumentation-extractor

Checkstyle

0bdd63f

iNikem reviewed Apr 1, 2021

View reviewed changes

trask approved these changes Apr 1, 2021

View reviewed changes

Cleanups

28c7d6c

mateuszrzeszutek approved these changes Apr 1, 2021

View reviewed changes

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…

cd185f6

…nstrumentation into instrumentation-extractor

anuraaga merged commit cc47da5 into open-telemetry:main Apr 5, 2021

mateuszrzeszutek mentioned this pull request Apr 29, 2021

Record HTTP server metrics in instrumenter API. #2877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Instrumenter API #2596

New Instrumenter API #2596

anuraaga commented Mar 18, 2021

mateuszrzeszutek left a comment

mateuszrzeszutek Mar 18, 2021

anuraaga Apr 1, 2021

mateuszrzeszutek Mar 18, 2021

trask Mar 22, 2021

anuraaga Mar 22, 2021

mateuszrzeszutek Mar 22, 2021

mateuszrzeszutek Mar 18, 2021

anuraaga Apr 1, 2021

mateuszrzeszutek Apr 1, 2021

mateuszrzeszutek Mar 18, 2021

mateuszrzeszutek commented Mar 19, 2021

trask commented Mar 22, 2021

anuraaga commented Mar 29, 2021

bogdandrutu Mar 29, 2021

anuraaga Mar 29, 2021

mateuszrzeszutek left a comment

mateuszrzeszutek Mar 30, 2021

anuraaga Mar 30, 2021

mateuszrzeszutek Mar 30, 2021

anuraaga Mar 30, 2021

anuraaga commented Mar 30, 2021

mateuszrzeszutek commented Mar 30, 2021

anuraaga commented Apr 1, 2021

iNikem left a comment

iNikem Apr 1, 2021

anuraaga Apr 1, 2021

iNikem Apr 1, 2021

anuraaga Apr 1, 2021

anuraaga Apr 1, 2021

trask left a comment

trask Apr 1, 2021

anuraaga Apr 1, 2021

New Instrumenter API #2596

New Instrumenter API #2596

Conversation

anuraaga commented Mar 18, 2021

mateuszrzeszutek 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

mateuszrzeszutek commented Mar 19, 2021

trask commented Mar 22, 2021

anuraaga commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszrzeszutek 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

anuraaga commented Mar 30, 2021

mateuszrzeszutek commented Mar 30, 2021

anuraaga commented Apr 1, 2021

iNikem 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

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment