Update Apache HttpClient 4.0 to Instrumenter API #3042
Conversation
trask
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay and
tylerbenson
as code owners
| URI uri = request.getURI(); | ||
| int port = uri.getPort(); | ||
| if (port != -1) { | ||
| return (long) port; | ||
| } | ||
| switch (uri.getScheme()) { | ||
| case "http": | ||
| return 80L; | ||
| case "https": | ||
| return 443L; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
do you think I should extract UriNetAttributesExtractor similar to InetSocketAddressNetAttributesExtractor?
There was a problem hiding this comment.
Is it correct to say URI will be more common in automatic instrumentation vs library instrumentation since it's a result of not having access to the internals? If so, possibly not, to encourage library authors adding OTel support to expose the more useful type.
There was a problem hiding this comment.
I'm not sure how common yet, will know soon 😁 (won't extract anything common for now and we can revisit later)
There was a problem hiding this comment.
There's one advantage to using URI in the instrumenter API: if we used it everywhere then we could have encapsulate the userInfo sanitization logic in the attribute extractor (well, we still can do it right now, but with a String url() method we'd have to have one more type conversion)
...io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientHelper.java
Outdated
Show resolved
Hide resolved
...javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientHttpAttributesExtractor.java
Outdated
Show resolved
Hide resolved
| enum HttpHeaderSetter implements TextMapSetter<HttpUriRequest> { | ||
| INSTANCE; |
There was a problem hiding this comment.
Nit: we don't really need to have setters/getters be enums, we can just create a new instance and pass to the instrumenter.
There was a problem hiding this comment.
will send a follow-up PR to change in all (few) existing places so @anuraaga can weigh in
…ent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientHttpAttributesExtractor.java Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
* Fix flaky spring batch test * Update instrumentation/spring/spring-batch-3.0/javaagent/src/test/groovy/ItemLevelSpanTest.groovy Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
* split out inner classes into separate files * and since they're public API I made them a little bit better: added javadocs, renamed classes/methods Getting the `...extension.muzzle` package stable is the first step to extracting the muzzle compile time plugin - the plugin may have a bit different release lifecycle than the main project and getting those classes stable will help a lot with eliminating breaking changes.
Another small part of #2713