Migrate vertx HTTP client to Instrumenter API #4510
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| @Nullable | ||
| NetClientAttributesExtractor<HttpClientRequest, HttpClientResponse> | ||
| netAttributesExtractor) { |
There was a problem hiding this comment.
weird formatting from spotless 🤷♂️
.../opentelemetry/javaagent/instrumentation/vertx/v4_0/client/Vertx4NetAttributesExtractor.java
Show resolved
Hide resolved
|
|
||
| private static final Instrumenter<HttpClientRequest, HttpClientResponse> INSTRUMENTER = | ||
| VertxClientInstrumenterFactory.create( | ||
| "io.opentelemetry.vertx-http-client-3.0", new Vertx3HttpAttributesExtractor(), null); |
There was a problem hiding this comment.
That would require parsing the URL (which is exposed as a string by the library), so I decided not to do that.
| import io.opentelemetry.context.propagation.TextMapSetter; | ||
| import io.vertx.core.http.HttpClientRequest; | ||
|
|
||
| public class HttpRequestHeaderSetter implements TextMapSetter<HttpClientRequest> { |
There was a problem hiding this comment.
Hmm, is there any point in doing so? Only one instance is created at a time, we don't have to worry about making it a singleton class.
There was a problem hiding this comment.
See #4522 and #995 (comment)
There was a problem hiding this comment.
Hmm, okay, I definitely agree that we should use enums (if possible) for implementing singletons. Is there any need to make this class a singleton in the first place?
| @Override | ||
| Set<AttributeKey<?>> httpAttributes(URI uri) { | ||
| def attributes = super.httpAttributes(uri) | ||
| attributes.remove(SemanticAttributes.HTTP_FLAVOR) |
There was a problem hiding this comment.
A lot of the instrumenters return SemanticAttributes.HttpFlavorValues.HTTP_1_1 because that is the only version that the library supports. Would it make sense to use the same approach here. Or should we change other places to not fill it also when we can't extract accurate info from the instrumented library?
There was a problem hiding this comment.
Or should we change other places to not fill it also when we can't extract accurate info from the instrumented library?
I think I prefer this option - if you can't extract something from the library, then just don't try to guess it.
In case of HTTP flavor, I guess that you could also use HTTP 1.0 and all those hard-coded 1.1 extractors would set false information.
I'll create an issue for cleaning this up in all instrumentations.
There was a problem hiding this comment.
Vert.x 3.0.0 seems to hardcode it to 1.1 https://github.com/eclipse-vertx/vert.x/blob/187083cc2572afe82b5bf9c743fa4df868926a56/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java#L91 Though this part has changed in later version. I believe it is the same with other frameworks that the 1.1 we guessed really is the only option.
There was a problem hiding this comment.
They changed it in 3.2, it does not have hard-coded version: https://github.com/eclipse-vertx/vert.x/blob/3.2.0/src/main/java/io/vertx/core/http/impl/HttpClientRequestImpl.java
This instrumentation also applies to >3.2, which makes me reluctant to set 1.1 for all possible cases.
| def attributes = super.httpAttributes(uri) | ||
| attributes.remove(SemanticAttributes.HTTP_FLAVOR) | ||
| attributes.remove(SemanticAttributes.NET_PEER_NAME) | ||
| attributes.remove(SemanticAttributes.NET_PEER_PORT) |
There was a problem hiding this comment.
I guess you chose not to fill peer port and name because it would require parsing them from the uri? An alternative would be to use reflection to get these.
One instrumentation strategy that I often used in a previous project in situations like this was to add an interface with methods like __getPort to implementation class and generate corresponding methods. Then when needed you could just instanceof + cast the request to get access to these. Unfortunately the only way to do this now would be to use asm which might be an overkill.
There was a problem hiding this comment.
Unfortunately the only way to do this now would be to use asm which might be an overkill.
Yep 😄
I think that we can leave it as it is until somebody creates a bug about it. Also, the newer 4.x Vertx version exposes host & port properly - so only users of an old Vertx version would not get net* attributes.
* Migrate vertx HTTP client to Instrumenter API * Fix vertx-reactive tests * Code review comments
Part of #2713