Jetty-9 httpclient instrumentation, comments please

[robododge]

This is my first PR attempt here, I am focusing on Jetty-9 instrumentation only. I am pushing this PR only for help resolving my questions, then I can build the Jetty 8 client.

Test status: 16 passed tests , 2 fails and 5 ignores

1. Need feedback on the 2 failing tests, they are for callback style http client.
2. Jetty 9.0 client had much API drift going into 9.1 and some breaking changes, so I starting the Liberary version at 9.1.x. How to compensate for the hole in Jetty 9.0?
3. Jetty 9.1 onward to Jetty 10 should be usable, but what features do the tests have to push through testing the various versions from 9.1 through 10?

I am using a different variation of this Jetty9 client in a live scenario, so I know it mostly works. I appreciate your feedback.

resolves: #2837

EDIT: (6/2/21: All tests pass now, callbacks wrapped properly)

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Steve Dodge (ea3274d)

[anuraaga]

Thanks for the contribution @robododge - would you be able to add library instrumentation too for non-agent users? For example, we have it for Armeria

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/armeria-1.3/library

I think we just need an HttpClient subclass, something like TracingHttpClient extends HttpClient and add the logic to send as I described below, add tracing, wrap listeners, and delegate to super.

We could either extract an internal static util to use the same send logic in the library instrumentation and agent send interception or possibly instrument the constructors to return TracingHttpClient.

Is this something you can help with?

[agoallikmaa]

If this is known to be incompatible with 9.0, maybe the whole subproject should use 9.1 in its name rather than 9.0?

If the instrumentation does not work correctly on 9.0, then it would be best if the type instrumentation is not applied there. This could be done by restricting it to 9.1+ by implementing InstrumentationModule#classLoaderMatcher. For example it seems that the class org.eclipse.jetty.client.api.ProxyConfiguration was removed in 9.1 and was not added back in later versions, so the classLoaderMatcher could make sure that class does not exist.

[robododge]

@agoallikmaa Thanks!

If the instrumentation does not work correctly on 9.0, then it would be best if the type instrumentation is not applied there. This could be done by restricting it to 9.1+ by implementing InstrumentationModule#classLoaderMatcher. For example it seems that the class org.eclipse.jetty.client.api.ProxyConfiguration was removed in 9.1 and was not added back in later versions, so the classLoaderMatcher could make sure that class does not exist.

I agree, with the 9.1 focus. Good find on the ProxyConfiguration class. Out of curiosity, is there a good approach or a tool that you could recommend to discover the class deltas from version to version? Or is it just stare-and-compare?

[agoallikmaa]

I agree, with the 9.1 focus. Good find on the ProxyConfiguration class. Out of curiosity, is there a good approach or a tool that you could recommend to discover the class deltas from version to version? Or is it just stare-and-compare?

I'm not aware of any tool for this, I have just been comparing versions manually.

[robododge]

@anuraaga - Regarding the non-agent usecase, I think I understand the ask, but need to clarify, is non-agent used when someone prefers to simply integrate the tracer code directly with their functional code? Please give me an example of how someone would use it on a non-functional basis. I think my code is close to enough that add a non-agent option wouldn't be much trouble as you explained, just want to make sure I have the usecase right.

[anuraaga]

@robododge The use case for library instrumentation is more about wanting to avoid the agent due to some extra overhead, (startup time overhead is unavoidable) and risk by such an invasive process. It's not to integrate tracer code directly into user business logic though, we provide something (interceptor, subclass, hooks, etc) that works with that library so the user can initialize it in their code, but still not really deal with tracing.

The unit test for Armeria library vs agent might be informative

Library:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/armeria-1.3/library/src/test/groovy/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy#L14

Agent:
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/armeria-1.3/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.groovy#L15

All the unit tests are the same, the user just required an extra line to initialize using library instrumentation. So in this case, we would probably structure the test to let the library version return the tracing subclass.

[robododge]

Another request for help on this PR. I've been going through implementing all the changes as mentioned. All the comments here have steered me toward HttpClient.send() as the place to attach the tracer and also wrap callback listeners. But looks like we have another problem with API drift in the 9.x series. Jetty client 9.2 started using a different method signature for the send() method. But, from 9.2 on into 10.0, it seems to stay the same. So far I have cut out v9.0, now this would probably mean cutting out v9.2 as well. Any concerns?

• Send() - v9.1 protected void send(Request request, List<Response.ResponseListener> listeners)😟

• Send() - v9.2 protected void send(final HttpRequest request, List<Response.ResponseListener> listeners)

• Send() - v10.0.2 protected void send(final HttpRequest request, List<Response.ResponseListener> listeners)

My new technique is like this:

public class JettyClientTracing extends HttpClient {
 @Override
  protected void send(HttpRequest request, List<Response.ResponseListener> listeners) {
   //Setup the tracer, start span context and wrap all listeners
  }
}

[trask]

from 9.2 on into 10.0, it seems to stay the same. So far I have cut out v9.0, now this would probably mean cutting out v9.2 as well. Any concerns?

hey @robododge! just confirming, you mean cutting out v9.1 as well? and keeping 9.2+?

[anuraaga]

@robododge Hmm that's an unfortunate change to an exposed method. Do you know if it's possible to implement both to support both versions? Something like

// 9.1
protected void send(Request request, List<Response.ResponseListener> listeners) {
  Context context = instrumenter.onStart(Context.current, request);
  listeners = decorateListeners(listeners, context);
  super.send(request, listeners);
}

// 9.2+
protected void send(HttpRequest request, List<Response.ResponseListener> listeners) {
  // Looks like our instrumentation can just handle Request
  Context context = instrumenter.onStart(Context.current, request);
  listeners = decorateListeners(listeners, context);
  super.send(request, listeners);
}

Not too much duplication

[trask]

v9.1 is very old, I would vote to drop it if there's any added complexity to supporting it

(EDIT: v9.2.0 was released May 2014)

[robododge]

hey @robododge! just confirming, you mean cutting out v9.1 as well? and keeping 9.2+?

@trask - yes proposal right now is drop v9.0, v9.1

[robododge]

@robododge Hmm that's an unfortunate change to an exposed method. Do you know if it's possible to implement both to support both versions?

@anuraaga - the technique I'm using is to actually create a wrapper class that overrides the HttpClient.send(). The code won't compile for example when you you have an v9.2 style HttpRequest object and you try to call the v9.1 style send(Request request, List<Response.ResponseListener> listeners

[trask]

@trask - yes proposal right now is drop v9.0, v9.1

@anuraaga thumbs'd up my comment above, so go for it, drop 'em 👍

[trask]

thanks @robododge!

if you are up for it, check out the new Instrumenter API in #2713, a few http client instrumentations have already been converted which you could use as a guide for this one

[mateuszrzeszutek]

LGTM 👍

Thanks @robododge !

[robododge]

Hi guys, are we good here? I want to get Jetty Http client 10 started after this

[robododge]

Hi guys, are we good here? I want to get Jetty Http client 10 started after this

I guess we're waiting for PR builds to come back

[anuraaga]

Found one minor point that can be addressed (if needed) in a followup. Thanks @robododge

[anuraaga]

Does the test use jetty server?

[robododge]

Just tested without that jetter-server reference, tests passed. So, no it could be removed, want to do that in a PR?