-
Notifications
You must be signed in to change notification settings - Fork 872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jetty-9 httpclient instrumentation, comments please #3079
Changes from all commits
ca251fa
56d6b64
fd16bad
a516efa
5c31232
dfc7fba
1239182
93da464
7ae9587
0f98296
a1d74b2
4eafb3e
0e94971
e261762
1f91406
0c856de
6a52659
b50d84b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
plugins { | ||
id("otel.javaagent-instrumentation") | ||
} | ||
|
||
muzzle { | ||
pass { | ||
group = "org.eclipse.jetty" | ||
module = 'jetty-client' | ||
versions = "[9.2,9.4.+)" | ||
} | ||
} | ||
|
||
|
||
//Jetty client 9.2 is the best starting point, HttpClient.send() is stable there | ||
def jettyVers_base9 = '9.2.0.v20140526' | ||
|
||
|
||
dependencies { | ||
implementation project(':instrumentation:jetty-httpclient:jetty-httpclient-9.2:library') | ||
|
||
library "org.eclipse.jetty:jetty-client:${jettyVers_base9}" | ||
latestDepTestLibrary "org.eclipse.jetty:jetty-client:9.+" | ||
|
||
testImplementation project(':instrumentation:jetty-httpclient:jetty-httpclient-9.2:testing') | ||
testImplementation("org.eclipse.jetty:jetty-server:${jettyVers_base9}") { | ||
exclude group: 'org.eclipse.jetty', module: 'jetty-client' | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.jetty.httpclient.v9_2; | ||
|
||
import static io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyClientWrapUtil.wrapResponseListeners; | ||
import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext; | ||
import static io.opentelemetry.javaagent.instrumentation.jetty.httpclient.v9_2.JettyHttpClientSingletons.instrumenter; | ||
import static net.bytebuddy.matcher.ElementMatchers.isMethod; | ||
import static net.bytebuddy.matcher.ElementMatchers.named; | ||
import static net.bytebuddy.matcher.ElementMatchers.takesArgument; | ||
|
||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.context.Scope; | ||
import io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyHttpClient9TracingInterceptor; | ||
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; | ||
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; | ||
import java.util.List; | ||
import net.bytebuddy.asm.Advice; | ||
import net.bytebuddy.description.type.TypeDescription; | ||
import net.bytebuddy.matcher.ElementMatcher; | ||
import org.eclipse.jetty.client.HttpRequest; | ||
import org.eclipse.jetty.client.api.Response; | ||
|
||
public class JettyHttpClient9Instrumentation implements TypeInstrumentation { | ||
@Override | ||
public ElementMatcher<TypeDescription> typeMatcher() { | ||
return named("org.eclipse.jetty.client.HttpClient"); | ||
} | ||
|
||
@Override | ||
public void transform(TypeTransformer transformer) { | ||
transformer.applyAdviceToMethod( | ||
isMethod() | ||
.and(named("send")) | ||
.and(takesArgument(0, named("org.eclipse.jetty.client.HttpRequest"))) | ||
.and(takesArgument(1, List.class)), | ||
JettyHttpClient9Instrumentation.class.getName() + "$JettyHttpClient9Advice"); | ||
} | ||
|
||
@SuppressWarnings("unused") | ||
public static class JettyHttpClient9Advice { | ||
|
||
@Advice.OnMethodEnter(suppress = Throwable.class) | ||
public static void addTracingEnter( | ||
@Advice.Argument(value = 0) HttpRequest httpRequest, | ||
@Advice.Argument(value = 1, readOnly = false) List<Response.ResponseListener> listeners, | ||
@Advice.Local("otelContext") Context context, | ||
@Advice.Local("otelScope") Scope scope) { | ||
Context parentContext = currentContext(); | ||
if (!instrumenter().shouldStart(parentContext, httpRequest)) { | ||
return; | ||
} | ||
|
||
// First step is to attach the tracer to the Jetty request. Request listeners are wrapped here | ||
JettyHttpClient9TracingInterceptor requestInterceptor = | ||
new JettyHttpClient9TracingInterceptor(parentContext, instrumenter()); | ||
requestInterceptor.attachToRequest(httpRequest); | ||
|
||
// Second step is to wrap all the important result callback | ||
listeners = wrapResponseListeners(parentContext, listeners); | ||
|
||
context = requestInterceptor.getContext(); | ||
scope = context.makeCurrent(); | ||
} | ||
|
||
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class) | ||
public static void exitTracingInterceptor( | ||
@Advice.Argument(value = 0) HttpRequest httpRequest, | ||
@Advice.Thrown Throwable throwable, | ||
@Advice.Local("otelContext") Context context, | ||
@Advice.Local("otelScope") Scope scope) { | ||
|
||
if (scope == null) { | ||
return; | ||
} | ||
|
||
// not ending span here unless error, span ended in the interceptor | ||
scope.close(); | ||
if (throwable != null) { | ||
instrumenter().end(context, httpRequest, null, throwable); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.jetty.httpclient.v9_2; | ||
|
||
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed; | ||
import static java.util.Collections.singletonList; | ||
|
||
import com.google.auto.service.AutoService; | ||
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; | ||
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; | ||
import java.util.List; | ||
import net.bytebuddy.matcher.ElementMatcher; | ||
|
||
@AutoService(InstrumentationModule.class) | ||
public class JettyHttpClient9InstrumentationModule extends InstrumentationModule { | ||
|
||
public JettyHttpClient9InstrumentationModule() { | ||
super("jetty-httpclient", "jetty-httpclient-9.2"); | ||
} | ||
|
||
@Override | ||
public List<TypeInstrumentation> typeInstrumentations() { | ||
return singletonList(new JettyHttpClient9Instrumentation()); | ||
} | ||
|
||
@Override | ||
public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() { | ||
// AbstractTypedContentProvider showed up in version Jetty Client 9.2 on to 10.x | ||
return hasClassesNamed("org.eclipse.jetty.client.util.AbstractTypedContentProvider"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.jetty.httpclient.v9_2; | ||
|
||
import io.opentelemetry.api.GlobalOpenTelemetry; | ||
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; | ||
import io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyClientInstrumenterBuilder; | ||
import io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyHttpClientNetAttributesExtractor; | ||
import io.opentelemetry.javaagent.instrumentation.api.instrumenter.PeerServiceAttributesExtractor; | ||
import org.eclipse.jetty.client.api.Request; | ||
import org.eclipse.jetty.client.api.Response; | ||
|
||
public class JettyHttpClientSingletons { | ||
|
||
private static final Instrumenter<Request, Response> INSTRUMENTER; | ||
|
||
private JettyHttpClientSingletons() {} | ||
|
||
static { | ||
JettyClientInstrumenterBuilder builder = | ||
new JettyClientInstrumenterBuilder(GlobalOpenTelemetry.get()); | ||
|
||
PeerServiceAttributesExtractor<Request, Response> peerServiceAttributesExtractor = | ||
PeerServiceAttributesExtractor.create(new JettyHttpClientNetAttributesExtractor()); | ||
INSTRUMENTER = builder.addAttributeExtractor(peerServiceAttributesExtractor).build(); | ||
} | ||
|
||
public static Instrumenter<Request, Response> instrumenter() { | ||
return INSTRUMENTER; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.javaagent.instrumentation.jetty.httpclient.v9_2 | ||
|
||
import io.opentelemetry.instrumentation.jetty.httpclient.v9_2.AbstractJettyClient9Test | ||
import io.opentelemetry.instrumentation.test.AgentTestTrait | ||
import org.eclipse.jetty.client.HttpClient | ||
import org.eclipse.jetty.util.ssl.SslContextFactory | ||
|
||
class JettyHttpClient9AgentTest extends AbstractJettyClient9Test implements AgentTestTrait { | ||
|
||
@Override | ||
HttpClient createStandardClient() { | ||
return new HttpClient() | ||
} | ||
|
||
@Override | ||
HttpClient createHttpsClient(SslContextFactory sslContextFactory) { | ||
return new HttpClient(sslContextFactory) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
plugins { | ||
id("otel.library-instrumentation") | ||
id("net.ltgt.errorprone") | ||
} | ||
|
||
|
||
//Jetty client 9.2 is the best starting point, HttpClient.send() is stable there | ||
def jettyVers_base9 = '9.2.0.v20140526' | ||
|
||
dependencies { | ||
library "org.eclipse.jetty:jetty-client:${jettyVers_base9}" | ||
latestDepTestLibrary "org.eclipse.jetty:jetty-client:9.+" | ||
testImplementation project(':instrumentation:jetty-httpclient::jetty-httpclient-9.2:testing') | ||
|
||
implementation "org.slf4j:slf4j-api" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.jetty.httpclient.v9_2; | ||
|
||
import io.opentelemetry.api.OpenTelemetry; | ||
import org.eclipse.jetty.client.HttpClient; | ||
|
||
/** JettyClientTracing, the Entrypoint for tracing Jetty client. */ | ||
public final class JettyClientTracing { | ||
|
||
private final HttpClient httpClient; | ||
|
||
public static JettyClientTracing create(OpenTelemetry openTelemetry) { | ||
JettyClientTracingBuilder builder = newBuilder(openTelemetry); | ||
return builder.build(); | ||
} | ||
|
||
public static JettyClientTracingBuilder newBuilder(OpenTelemetry openTelemetry) { | ||
return new JettyClientTracingBuilder(openTelemetry); | ||
} | ||
|
||
public HttpClient getHttpClient() { | ||
return httpClient; | ||
} | ||
|
||
JettyClientTracing(HttpClient httpClient) { | ||
this.httpClient = httpClient; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.jetty.httpclient.v9_2; | ||
|
||
import io.opentelemetry.api.OpenTelemetry; | ||
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; | ||
import io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyClientInstrumenterBuilder; | ||
import org.eclipse.jetty.client.HttpClientTransport; | ||
import org.eclipse.jetty.client.api.Request; | ||
import org.eclipse.jetty.client.api.Response; | ||
import org.eclipse.jetty.util.ssl.SslContextFactory; | ||
|
||
public final class JettyClientTracingBuilder { | ||
|
||
private final OpenTelemetry openTelemetry; | ||
private HttpClientTransport httpClientTransport; | ||
private SslContextFactory sslContextFactory; | ||
|
||
public JettyClientTracingBuilder(OpenTelemetry openTelemetry) { | ||
this.openTelemetry = openTelemetry; | ||
} | ||
|
||
public JettyClientTracingBuilder setHttpClientTransport(HttpClientTransport httpClientTransport) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can fill in other javadoc later but do you think you can add to at least these two Jetty-specific ones being closest to the code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure - I can put in some good javadoc for these |
||
this.httpClientTransport = httpClientTransport; | ||
return this; | ||
} | ||
|
||
public JettyClientTracingBuilder setSslContextFactory(SslContextFactory sslContextFactory) { | ||
this.sslContextFactory = sslContextFactory; | ||
return this; | ||
} | ||
|
||
public JettyClientTracing build() { | ||
JettyClientInstrumenterBuilder instrumenterBuilder = | ||
new JettyClientInstrumenterBuilder(this.openTelemetry); | ||
Instrumenter<Request, Response> instrumenter = instrumenterBuilder.build(); | ||
|
||
TracingHttpClient tracingHttpClient = | ||
TracingHttpClient.buildNew(instrumenter, this.sslContextFactory, this.httpClientTransport); | ||
|
||
return new JettyClientTracing(tracingHttpClient); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.jetty.httpclient.v9_2; | ||
|
||
import static io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyClientWrapUtil.wrapResponseListeners; | ||
|
||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; | ||
import io.opentelemetry.instrumentation.jetty.httpclient.v9_2.internal.JettyHttpClient9TracingInterceptor; | ||
import java.util.List; | ||
import org.eclipse.jetty.client.HttpClient; | ||
import org.eclipse.jetty.client.HttpClientTransport; | ||
import org.eclipse.jetty.client.HttpRequest; | ||
import org.eclipse.jetty.client.api.Request; | ||
import org.eclipse.jetty.client.api.Response; | ||
import org.eclipse.jetty.util.ssl.SslContextFactory; | ||
|
||
class TracingHttpClient extends HttpClient { | ||
|
||
private final Instrumenter<Request, Response> instrumenter; | ||
|
||
TracingHttpClient(Instrumenter<Request, Response> instrumenter) { | ||
super(); | ||
this.instrumenter = instrumenter; | ||
} | ||
|
||
TracingHttpClient( | ||
Instrumenter<Request, Response> instrumenter, SslContextFactory sslContextFactory) { | ||
super(sslContextFactory); | ||
this.instrumenter = instrumenter; | ||
} | ||
|
||
TracingHttpClient( | ||
Instrumenter<Request, Response> instrumenter, | ||
HttpClientTransport transport, | ||
SslContextFactory sslContextFactory) { | ||
super(transport, sslContextFactory); | ||
this.instrumenter = instrumenter; | ||
} | ||
|
||
static TracingHttpClient buildNew( | ||
Instrumenter<Request, Response> instrumenter, | ||
SslContextFactory sslContextFactory, | ||
HttpClientTransport httpClientTransport) { | ||
TracingHttpClient tracingHttpClient = null; | ||
if (sslContextFactory != null && httpClientTransport != null) { | ||
tracingHttpClient = | ||
new TracingHttpClient(instrumenter, httpClientTransport, sslContextFactory); | ||
} else if (sslContextFactory != null) { | ||
tracingHttpClient = new TracingHttpClient(instrumenter, sslContextFactory); | ||
} else { | ||
tracingHttpClient = new TracingHttpClient(instrumenter); | ||
} | ||
return tracingHttpClient; | ||
} | ||
|
||
@Override | ||
protected void send(HttpRequest request, List<Response.ResponseListener> listeners) { | ||
Context parentContext = Context.current(); | ||
JettyHttpClient9TracingInterceptor requestInterceptor = | ||
new JettyHttpClient9TracingInterceptor(parentContext, this.instrumenter); | ||
requestInterceptor.attachToRequest(request); | ||
List<Response.ResponseListener> wrapped = wrapResponseListeners(parentContext, listeners); | ||
super.send(request, wrapped); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the test use jetty server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested without that jetter-server reference, tests passed. So, no it could be removed, want to do that in a PR?