Apache httpasyncclient 5.x #5697

anuragagarwal561994 · 2022-03-26T17:36:13Z

Closes: #5631

linux-foundation-easycla · 2022-03-26T17:36:17Z

The committers listed above are authorized under a signed CLA.

✅ login: anuragagarwal561994 / name: Anurag Agarwal (6ce2705, 0ef6cbd, dd8ac9f, f7d6c77, 7c2c4cf, cdea4a6, 9617509)

anuragagarwal561994 · 2022-03-26T17:57:23Z

@ok2c @trask this is the first draft, let us just continue our discussion if any on this MR.

I am open to any suggestion or changes that are required, all the test cases are passing currently and I have also incorporated the test case changes that were suggested to me.

mateuszrzeszutek

Left a couple of comments, but overall it looks really good 👍
Thanks!

docs/supported-libraries.md

mateuszrzeszutek · 2022-03-28T09:15:00Z

...vaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientHttpAttributesGetter.java

+  @Override
+  @Nullable
+  public Integer statusCode(ApacheHttpClientRequest request, HttpResponse response) {
+    return response != null ? response.getCode() : null;


response will always be non-null here

mateuszrzeszutek · 2022-03-28T09:18:17Z

...o/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

+    return uri != null ? uri.toString() : null;
+  }
+
+  public String getFlavor(HttpResponse response) {


Nit

Suggested change

public String getFlavor(HttpResponse response) {

public String getFlavor(@Nullable HttpResponse response) {

This was actually done for interim purpose, actually if we see this is kind of a static method or more related to response and not the request, what do you suggest I should do here. This should ideally not be part of the AsyncHttpClientRequest now as it is now not a request property.

Hmm, that's true. You can inline this method into ApacheHttpAsyncClientHttpAttributesGetter

I will need to add a logger separately in the AttributesGetter as well that should be fine right?

Yeah, perfectly fine.

mateuszrzeszutek · 2022-03-28T09:20:40Z

...o/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

+
+public final class ApacheHttpClientRequest {
+
+  private static final Logger logger = LoggerFactory.getLogger(ApacheHttpClientRequest.class);


Recently we've removed all slf4j usage from most of the agent code and switched to JUL -- can you rebase against a fresh main and replace slf4j with java.util.logging logger?

...opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientTest.java

anuragagarwal561994 · 2022-03-28T22:00:57Z

Thanks @mateuszrzeszutek there are couple of things I would like to highlight with respect to the MR.
In HttpClient 4, we know while the instrumentation is initiated the flavor of the request.
In HttpClient 5, we know this after we have made the connection, which is after the instrumentation has started.

Thus in test cases where we are having issues connecting with the server, my tests were failing and thus for the test cases I have removed the check for flavor, although it still comes in for the successful request cases. Suggest me if I can do something here, as far as I saw the library I couldn't think of anything else.

anuragagarwal561994 · 2022-03-28T22:04:57Z

Support for method

public final <T> Future<T> execute(
            final HttpHost target,
            final AsyncRequestProducer requestProducer,
            final AsyncResponseConsumer<T> responseConsumer,
            final HandlerFactory<AsyncPushConsumer> pushHandlerFactory,
            final HttpContext context,
            final FutureCallback<T> callback) {
    }

in CloseableHttpAsyncClient is not added and the test cases were not written for the same although a user might be using the same, because I thought instrumenting an interface would be right than instrumenting an abstract class implementing that interface.

So the instrumentation kind of doesn't cover all the cases but most of the cases for now.

anuragagarwal561994 · 2022-03-28T22:29:47Z

Third thing that I would want to highlight here is that HTTP client 5 by default in Negotiate mode which means if http endpoint is given it will do http 1.1 but if https is given then it will prefer http 2.

Most of the tests were passed but io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest#httpsRequest was failing as I think the server was returning http 1.1 response while the client was trying to parse it as http 2 or something happened here. If you help me with that case that would be great.

For now I enforced the http client 5 to use HttpVersionPolicy.FORCE_HTTP_1 in tests to overcome this scenario, but if there is something we can do here, do let me know.

mateuszrzeszutek · 2022-03-29T10:41:15Z

...o/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

+    return uri != null ? uri.toString() : null;
+  }
+
+  public String getFlavor(HttpResponse response) {


Hmm, that's true. You can inline this method into ApacheHttpAsyncClientHttpAttributesGetter

...o/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

mateuszrzeszutek · 2022-03-29T10:42:58Z

...o/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

+      case "https":
+        return 443;
+      default:
+        logger.log(FINE, "no default port mapping for scheme: {}", uri.getScheme());


Suggested change

logger.log(FINE, "no default port mapping for scheme: {}", uri.getScheme());

logger.log(FINE, "no default port mapping for scheme: {0}", uri.getScheme());

mateuszrzeszutek · 2022-03-29T11:09:49Z

Thus in test cases where we are having issues connecting with the server, my tests were failing and thus for the test cases I have removed the check for flavor, although it still comes in for the successful request cases. Suggest me if I can do something here, as far as I saw the library I couldn't think of anything else.

The HttpClientTestOptions#setHttpAttributes() method accepts a lambda that gets the URI called as a parameter -- depending on which endpoint was called (e.g. containing /success) you can decide whether to expect http.flavor or not.

So the instrumentation kind of doesn't cover all the cases but most of the cases for now.

It can always be added in a separate PR, I believe it is fine for now (as long as we don't lose that information).

For now I enforced the http client 5 to use HttpVersionPolicy.FORCE_HTTP_1 in tests to overcome this scenario, but if there is something we can do here, do let me know.

We don't have any HTTP 2 test coverage yet (see #3227) -- for now you don't have to worry about, forcing HTTP/1.1 if perfectly fine.

anuragagarwal561994 · 2022-03-29T13:54:28Z

I have addressed all the comments, @mateuszrzeszutek you can check the updated code

anuragagarwal561994 · 2022-03-29T17:53:13Z

The HttpClientTestOptions#setHttpAttributes() method accepts a lambda that gets the URI called as a parameter -- depending on which endpoint was called (e.g. containing /success) you can decide whether to expect http.flavor or not.

This is done as well, any other changes required?

trask

thx @anuragagarwal561994!

instrumentation/apache-httpasyncclient-5.0/javaagent/build.gradle.kts

...aagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentationModule.java

anuragagarwal561994 · 2022-03-30T07:51:52Z

Sure @trask let me try doing the same and check if some other issues come up. How about I keep the package name separate and merge the modules?

anuragagarwal561994 · 2022-03-30T09:08:57Z

@trask I tried merging the modules, there is just one issue httpclient-5.0 is having groovy test cases as of now and I am not able to get it running on my local, may be things are done correctly just that I was not able to test in my IDE.

If you can suggest something it would be of help, it just either reports some imports missing or so.

On running gradle build in terminal I get

Execution failed for task ':instrumentation:grpc-1.6:testing:generateProto'.
> Could not resolve all files for configuration ':instrumentation:grpc-1.6:testing:protobufToolsLocator_protoc'.
   > Could not find protoc-3.3.0-osx-aarch_64.exe (com.google.protobuf:protoc:3.3.0).
     Searched in the following locations:
         https://repo.maven.apache.org/maven2/com/google/protobuf/protoc/3.3.0/protoc-3.3.0-osx-aarch_64.exe

this is irrespective of this change, just that I couldn't test it on my local.

In IDE while building I keep getting javax.annotation.Nullable not found

...lemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientSingletons.java

...y/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpClientInstrumentationModule.java

trask · 2022-03-30T20:13:59Z

@trask I tried merging the modules, there is just one issue httpclient-5.0 is having groovy test cases as of now and I am not able to get it running on my local, may be things are done correctly just that I was not able to test in my IDE.

checked out your PR locally, pushed one small change back to your PR, and tests are passing locally for me

trask · 2022-03-30T20:15:00Z

docs/supported-libraries.md

-| [Apache HttpAsyncClient](https://hc.apache.org/index.html)                                                                        | 4.1+                           |
+| [Apache HttpAsyncClient](https://hc.apache.org/index.html)                                                                        | 4.1+ and 5.0+                  |


I think remove this change (5.0+ is covered by 4.1+ anyways)

trask · 2022-03-30T20:15:49Z

instrumentation/apache-httpasyncclient-5.0/javaagent/build.gradle.kts

@@ -0,0 +1,15 @@
+plugins {


can delete this file now

trask · 2022-03-30T20:19:31Z

...aagent/instrumentation/apachehttpclient/async/ApacheHttpAsyncClientHttpAttributesGetter.java

+
+  @Override
+  @Nullable
+  public String flavor(ApacheHttpClientRequest request, @Nullable HttpResponse response) {


if this is exactly the same as method in classic instrumentation, how about extracting common helper somewhere?

More or less it is same but the request type is different, I guess we will have to keep 2 classes.

trask · 2022-03-30T20:20:27Z

.../opentelemetry/javaagent/instrumentation/apachehttpclient/async/ApacheHttpClientRequest.java

+  }
+
+  // minimize memory overhead by not using streams
+  static List<String> headersToList(Header[] headers) {


same with this method, if exactly the same as method in classic

The suggestions are good, but I guess it will involve more refactoring in both parts of the client support.
Like when I started this MR, I thought of just using HttpRequest only which is http client specific, but then I got a use-case of keeping the extra class AsyncHttpClientRequest which was to store the entity.

Both the async and the http client are kind of using different strategies. In async we are decorating, in classic we are replacing the request instance. So better way would be to have something in common and another way could be to have something like Request / Response adapters which operates on HttpRequest / HttpResponse. I see other methods which repeat like getFlavor

anuragagarwal561994 · 2022-04-03T00:37:44Z

@trask @mateuszrzeszutek I have done some rework on my pull request and now I guess it should look awesome 😄

Let me summarise the ChangeLog:

Removed the ApacheHttpAsyncClientRequest - only reason left to keep it was to get the request content length, for starters we can do away with it.
Made everything generic to HttpRequest - this allowed reusing the same logic for async client and classic client
Moved all the logic from respective attribute getters to ApacheHttpClientUtils, generally by design I don't prefer utility classes (because this is Java not a scripting language) except in cases where we are dealing with third party entities
After 2nd and 3rd refactoring, only thing left in the async client package was just the instrumentation class, so merged both the packages, all of the code became reusable by this point
For the test cases I moved the current http client test cases to java (another pain point resolved 😄)
Moved the boilerplate code into AbstractApacheHttpClientTest class thus making both of them look similar.
Used the connectionTimeout and readTimeout method where possible
Where I couldn't use the timeout method, I used them as static constants.

anuragagarwal561994 · 2022-04-04T18:59:27Z

@mateuszrzeszutek @trask did you get time to review this MR?

trask

thx this is looking great! will do a more thorough review soon

...java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/ApacheHttpClientHelper.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

trask

thx @anuragagarwal561994 💯

* Copies code for httpasyncclient-4.1 and creates instrumentation for 5.0 * Makes import changes for http client 5 * Decorate request channel and changes type in tests * Corrects test cases * Corrects most of the test cases * Forces http1 protocol to pass the test cases * Merge supported libraries for async client Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> * Remove http.sceme and http.target attr from test Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> * Removes not needed null check for status code * Replaces slf4j loggers with JUL * Inlined flavor extraction in attributes getter * Uses parameter placeholders for logging * Uses success endpoint to test flavor * Merges httpasyncclient and httpclient modules * Merges http client 5 modules * Update java-8 compatible changes Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> * Change instrumentation name Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> * Adds missing import statement * Rename packages * Java 8 * Reverts adding 5.0+ support from supporting libraries * Deleted hanging module * Uses seconds instead of ms in http test * Merges both classic and async client implementations * Moves http client all test cases to java tests * Uses abstract apache test class and moves boilerplate * Uses connection and read timeouts from ApacheHttpClientTest * Refactors remaining classes, shifts logic to HttpUtils * Renames HttpUtils to ApacheHttpClientUtils * Corrects failing code style error * Corrects build errors * Renames package to have http client version * Corrects package name * Uses instrumenter as static import * Inline utility methods Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

anuragagarwal561994 requested a review from a team as a code owner March 26, 2022 17:36

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from 13caea9 to 9617509 Compare March 26, 2022 17:55

mateuszrzeszutek reviewed Mar 28, 2022

View reviewed changes

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from a2d70ff to 35a427c Compare March 28, 2022 22:24

mateuszrzeszutek reviewed Mar 29, 2022

View reviewed changes

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from 35a427c to 3f3e49c Compare March 29, 2022 13:53

trask reviewed Mar 30, 2022

View reviewed changes

instrumentation/apache-httpasyncclient-5.0/javaagent/build.gradle.kts Outdated Show resolved Hide resolved

...aagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentationModule.java Outdated Show resolved Hide resolved

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from bca4ddc to a049d8c Compare March 30, 2022 09:06

mateuszrzeszutek reviewed Mar 30, 2022

View reviewed changes

...lemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientSingletons.java Outdated Show resolved Hide resolved

...y/javaagent/instrumentation/apachehttpclient/v5_0/ApacheHttpClientInstrumentationModule.java Outdated Show resolved Hide resolved

trask reviewed Mar 30, 2022

View reviewed changes

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch 2 times, most recently from 029873f to c55b453 Compare April 2, 2022 14:52

anuragagarwal561994 closed this Apr 3, 2022

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from c55b453 to 56e3ed6 Compare April 3, 2022 00:20

anuragagarwal561994 reopened this Apr 3, 2022

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from cfba6cd to 5f6301d Compare April 4, 2022 18:57

trask reviewed Apr 4, 2022

View reviewed changes

...java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/ApacheHttpClientHelper.java Outdated Show resolved Hide resolved

anuragagarwal561994 and others added 26 commits April 7, 2022 10:27

Replaces slf4j loggers with JUL

f5219ed

Inlined flavor extraction in attributes getter

d18d60e

Uses parameter placeholders for logging

5fe5e66

Uses success endpoint to test flavor

83d5108

Merges httpasyncclient and httpclient modules

af3d8a8

Merges http client 5 modules

d4a2df3

Update java-8 compatible changes

35707eb

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

Change instrumentation name

5b4ab73

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

Adds missing import statement

f18ef1c

Rename packages

c95eb28

Java 8

45b2780

Reverts adding 5.0+ support from supporting libraries

18596f3

Deleted hanging module

aac4f8f

Uses seconds instead of ms in http test

e4e5a48

Merges both classic and async client implementations

288e930

Moves http client all test cases to java tests

9935c45

Uses abstract apache test class and moves boilerplate

c680f00

Uses connection and read timeouts from ApacheHttpClientTest

4dbd5bc

Refactors remaining classes, shifts logic to HttpUtils

d1d059a

Renames HttpUtils to ApacheHttpClientUtils

20bb2c8

Corrects failing code style error

e246b16

Corrects build errors

9d2cf7c

Renames package to have http client version

ff6f532

Corrects package name

9a5f0a7

Uses instrumenter as static import

8fdb626

Inline utility methods

5923e66

anuragagarwal561994 force-pushed the apache-httpasyncclient-5.x branch from 064fcbf to 5923e66 Compare April 7, 2022 05:02

trask approved these changes Apr 7, 2022

View reviewed changes

trask merged commit 13a851b into open-telemetry:main Apr 7, 2022

	public String getFlavor(HttpResponse response) {
	public String getFlavor(@Nullable HttpResponse response) {


		public final class ApacheHttpClientRequest {

		private static final Logger logger = LoggerFactory.getLogger(ApacheHttpClientRequest.class);

	logger.log(FINE, "no default port mapping for scheme: {}", uri.getScheme());
	logger.log(FINE, "no default port mapping for scheme: {0}", uri.getScheme());

		\| [Apache HttpAsyncClient](https://hc.apache.org/index.html) \| 4.1+ \|
		\| [Apache HttpAsyncClient](https://hc.apache.org/index.html) \| 4.1+ and 5.0+ \|

Apache httpasyncclient 5.x #5697

Apache httpasyncclient 5.x #5697

Conversation

anuragagarwal561994 commented Mar 26, 2022

linux-foundation-easycla bot commented Mar 26, 2022 • edited Loading

anuragagarwal561994 commented Mar 26, 2022

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

anuragagarwal561994 commented Mar 28, 2022

anuragagarwal561994 commented Mar 28, 2022

anuragagarwal561994 commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszrzeszutek commented Mar 29, 2022

anuragagarwal561994 commented Mar 29, 2022

anuragagarwal561994 commented Mar 29, 2022

trask left a comment

Choose a reason for hiding this comment

anuragagarwal561994 commented Mar 30, 2022

anuragagarwal561994 commented Mar 30, 2022

trask commented Mar 30, 2022

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

anuragagarwal561994 commented Apr 3, 2022

anuragagarwal561994 commented Apr 4, 2022

trask left a comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Mar 26, 2022 •

edited

Loading