Skip to content
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.

Sign up for GitHub

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

Fix integration with jersey-client 2.30 #2071

Merged
merged 3 commits into from
Jan 19, 2021
Merged

Fix integration with jersey-client 2.30 #2071

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5f232ef
Fix integration with jetty-client 2.30
laurit Jan 18, 2021
d349a05
make class final and add private constructor
laurit Jan 18, 2021
602c408
Update instrumentation/jaxrs-client/jaxrs-client-2.0/jaxrs-client-2.0…
laurit Jan 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...xrs-client-2.0/jaxrs-client-2.0-common/javaagent/jaxrs-client-2.0-common-javaagent.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ dependencies {

testInstrumentation project(':instrumentation:apache-httpclient:apache-httpclient-4.0:javaagent')

latestDepTestLibrary group: 'org.glassfish.jersey.inject', name: 'jersey-hk2', version: '2.27'
latestDepTestLibrary group: 'org.glassfish.jersey.core', name: 'jersey-client', version: '2.27'
latestDepTestLibrary group: 'org.glassfish.jersey.inject', name: 'jersey-hk2', version: '[2+, 3)'
latestDepTestLibrary group: 'org.glassfish.jersey.core', name: 'jersey-client', version: '[2+, 3)'
laurit marked this conversation as resolved.
Show resolved Hide resolved
latestDepTestLibrary group: 'org.apache.cxf', name: 'cxf-rt-rs-client', version: '3.2.6'
latestDepTestLibrary group: 'org.jboss.resteasy', name: 'resteasy-client', version: '3.0.26.Final'
}
11 changes: 2 additions & 9 deletions ...lemetry/javaagent/instrumentation/jaxrsclient/v2_0/JerseyClientInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@

package io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0;

import static io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0.JaxRsClientTracer.tracer;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.returns;

import com.google.auto.service.AutoService;
import io.opentelemetry.context.Context;
import io.opentelemetry.javaagent.tooling.InstrumentationModule;
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
import java.util.Collections;
Expand Down Expand Up @@ -71,10 +69,7 @@ public static void handleError(
@Advice.FieldValue("requestContext") ClientRequest context,
@Advice.Thrown Throwable throwable) {
if (throwable != null) {
Object prop = context.getProperty(ClientTracingFilter.CONTEXT_PROPERTY_NAME);
if (prop instanceof Context) {
tracer().endExceptionally((Context) prop, throwable);
}
JerseyClientUtil.handleException(context, throwable);
}
}
}
Expand All @@ -85,9 +80,7 @@ public static class SubmitAdvice {
public static void handleError(
@Advice.FieldValue("requestContext") ClientRequest context,
@Advice.Return(readOnly = false) Future<?> future) {
if (!(future instanceof WrappedFuture)) {
future = new WrappedFuture<>(future, context);
}
future = JerseyClientUtil.addErrorReporting(context, future);
}
}
}
45 changes: 45 additions & 0 deletions ...in/java/io/opentelemetry/javaagent/instrumentation/jaxrsclient/v2_0/JerseyClientUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0;

import static io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0.JaxRsClientTracer.tracer;

import io.opentelemetry.context.Context;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Future;
import org.glassfish.jersey.client.ClientRequest;

public class JerseyClientUtil {
laurit marked this conversation as resolved.
Show resolved Hide resolved

public static Future<?> addErrorReporting(ClientRequest context, Future<?> future) {
// since jersey 2.30 jersey internally uses CompletableFuture
// we can't wrap it with WrappedFuture as it causes ClassCastException when casting
// to CompletableFuture
if (future instanceof CompletableFuture) {
future =
((CompletableFuture<?>) future)
.whenComplete(
(result, exception) -> {
if (exception != null) {
handleException(context, exception);
}
});
} else {
if (!(future instanceof WrappedFuture)) {
future = new WrappedFuture<>(future, context);
}
}

return future;
}

public static void handleException(ClientRequest context, Throwable exception) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally add instrumentation-specific handling to the *Tracer classes themselves instead of separating utils, would that work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracer class is in a different module jaxrs-client-2.0-common adding these methods there would also require moving WrappedFuture there. Adding a tracer to jaxrs-client-2.0-jersey-2.0 also doesn't seem like a good option because this module doesn't really have any specialised tracing code.

Object prop = context.getProperty(ClientTracingFilter.CONTEXT_PROPERTY_NAME);
if (prop instanceof Context) {
tracer().endExceptionally((Context) prop, exception);
}
}
}
13 changes: 3 additions & 10 deletions .../main/java/io/opentelemetry/javaagent/instrumentation/jaxrsclient/v2_0/WrappedFuture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@

package io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0;

import static io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0.JaxRsClientTracer.tracer;
import static io.opentelemetry.javaagent.instrumentation.jaxrsclient.v2_0.JerseyClientUtil.handleException;

import io.opentelemetry.context.Context;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -44,10 +43,7 @@ public T get() throws InterruptedException, ExecutionException {
try {
return wrapped.get();
} catch (ExecutionException e) {
Object prop = context.getProperty(ClientTracingFilter.CONTEXT_PROPERTY_NAME);
if (prop instanceof Context) {
tracer().endExceptionally((Context) prop, e.getCause());
}
handleException(context, e.getCause());
throw e;
}
}
Expand All @@ -58,10 +54,7 @@ public T get(long timeout, TimeUnit unit)
try {
return wrapped.get(timeout, unit);
} catch (ExecutionException e) {
Object prop = context.getProperty(ClientTracingFilter.CONTEXT_PROPERTY_NAME);
if (prop instanceof Context) {
tracer().endExceptionally((Context) prop, e.getCause());
}
handleException(context, e.getCause());
throw e;
}
}
Expand Down