Skip to content

Commit c5f1862

Browse files
chenlujjjchenlujjj
committed
resolve some comments
1 parent 06f0869 commit c5f1862

20 files changed

+61
-114
lines changed

‎docs/supported-libraries.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ These are the supported libraries and frameworks:
9090
| [Jedis](https://github.com/xetorthio/jedis) | 1.4+ | N/A | [Database Client Spans] |
9191
| [JMS](https://javaee.github.io/javaee-spec/javadocs/javax/jms/package-summary.html) | 1.1+ | N/A | [Messaging Spans] |
9292
| [Jodd Http](https://http.jodd.org/) | 4.2+ | N/A | [HTTP Client Spans], [HTTP Client Metrics] |
93-
| [JSON-RPC](https://github.com/briandilley/jsonrpc4j) | 1.3.3+ | N/A | [RPC Client Spans], [RPC Client Metrics], [RPC Server Spans], [RPC Server Metrics] |
93+
| [JSON-RPC for Java](https://github.com/briandilley/jsonrpc4j) | 1.3.3+ | N/A | [RPC Client Spans], [RPC Client Metrics], [RPC Server Spans], [RPC Server Metrics] |
9494
| [JSP](https://javaee.github.io/javaee-spec/javadocs/javax/servlet/jsp/package-summary.html) | 2.3.x only | N/A | Controller Spans [3] |
9595
| [Kotlin Coroutines](https://kotlinlang.org/docs/coroutines-overview.html) | 1.0+ | N/A | Context propagation |
9696
| [Ktor](https://github.com/ktorio/ktor) | 1.0+ | [opentelemetry-ktor-1.0](../instrumentation/ktor/ktor-1.0/library),<br>[opentelemetry-ktor-2.0](../instrumentation/ktor/ktor-2.0/library),<br>[opentelemetry-ktor-3.0](../instrumentation/ktor/ktor-3.0/library) | [HTTP Client Spans], [HTTP Client Metrics], [HTTP Server Spans], [HTTP Server Metrics] |

‎instrumentation/jsonrpc4j-1.3/javaagent/build.gradle.kts

-10
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,8 @@ dependencies {
1717
library("com.github.briandilley.jsonrpc4j:jsonrpc4j:1.3.3")
1818

1919
testImplementation(project(":instrumentation:jsonrpc4j-1.3:testing"))
20-
2120
testImplementation("com.fasterxml.jackson.core:jackson-databind:2.13.3")
22-
2321
testImplementation("org.eclipse.jetty:jetty-server:9.4.49.v20220914")
24-
2522
testImplementation("org.eclipse.jetty:jetty-servlet:9.4.49.v20220914")
26-
2723
testImplementation("javax.portlet:portlet-api:2.0")
2824
}
29-
30-
tasks {
31-
test {
32-
jvmArgs("-Dotel.javaagent.experimental.thread-propagation-debugger.enabled=false")
33-
}
34-
}

‎instrumentation/jsonrpc4j-1.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsonrpc4j/v1_3/JsonRpcClientInstrumentation.java

+7-8
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import io.opentelemetry.instrumentation.jsonrpc4j.v1_3.JsonRpcClientResponse;
2121
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
2222
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
23+
import java.lang.reflect.Type;
2324
import java.util.Map;
2425
import net.bytebuddy.asm.Advice;
2526
import net.bytebuddy.description.type.TypeDescription;
@@ -46,8 +47,8 @@ public void transform(TypeTransformer transformer) {
4647
.and(takesArguments(4))
4748
.and(takesArgument(0, String.class))
4849
.and(takesArgument(1, Object.class))
49-
.and(takesArgument(2, named("java.lang.reflect.Type")))
50-
.and(takesArgument(3, named("java.util.Map")))
50+
.and(takesArgument(2, Type.class))
51+
.and(takesArgument(3, Map.class))
5152
.and(returns(Object.class)),
5253
this.getClass().getName() + "$InvokeAdvice");
5354
}
@@ -60,10 +61,11 @@ public static void onEnter(
6061
@Advice.Argument(0) String methodName,
6162
@Advice.Argument(1) Object argument,
6263
@Advice.Argument(3) Map<String, String> extraHeaders,
64+
@Advice.Local("otelRequest") JsonRpcClientRequest request,
6365
@Advice.Local("otelContext") Context context,
6466
@Advice.Local("otelScope") Scope scope) {
6567
Context parentContext = Context.current();
66-
JsonRpcClientRequest request = new JsonRpcClientRequest(methodName, argument);
68+
request = new JsonRpcClientRequest(methodName, argument);
6769
if (!CLIENT_INSTRUMENTER.shouldStart(parentContext, request)) {
6870
return;
6971
}
@@ -79,18 +81,15 @@ public static void onExit(
7981
@Advice.Argument(3) Map<String, String> extraHeaders,
8082
@Advice.Return Object result,
8183
@Advice.Thrown Throwable throwable,
84+
@Advice.Local("otelRequest") JsonRpcClientRequest request,
8285
@Advice.Local("otelContext") Context context,
8386
@Advice.Local("otelScope") Scope scope) {
8487
if (scope == null) {
8588
return;
8689
}
8790

8891
scope.close();
89-
CLIENT_INSTRUMENTER.end(
90-
context,
91-
new JsonRpcClientRequest(methodName, argument),
92-
new JsonRpcClientResponse(result),
93-
throwable);
92+
CLIENT_INSTRUMENTER.end(context, request, new JsonRpcClientResponse(result), throwable);
9493
}
9594
}
9695
}

‎instrumentation/jsonrpc4j-1.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsonrpc4j/v1_3/JsonRpcProxyInstrumentation.java

+15-29
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
2121
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
2222
import java.lang.reflect.InvocationHandler;
23+
import java.lang.reflect.InvocationTargetException;
2324
import java.lang.reflect.Method;
2425
import java.lang.reflect.Proxy;
2526
import java.util.Map;
@@ -60,21 +61,6 @@ public static <T> void onExit(
6061
proxy = instrumentCreateClientProxy(classLoader, proxyInterface, client, extraHeaders, proxy);
6162
}
6263

63-
private static Object proxyObjectMethods(Method method, Object proxyObject, Object[] args) {
64-
String name = method.getName();
65-
switch (name) {
66-
case "toString":
67-
return proxyObject.getClass().getName() + "@" + System.identityHashCode(proxyObject);
68-
case "hashCode":
69-
return System.identityHashCode(proxyObject);
70-
case "equals":
71-
return proxyObject == args[0];
72-
default:
73-
throw new IllegalArgumentException(
74-
method.getName() + " is not a member of java.lang.Object");
75-
}
76-
}
77-
7864
@SuppressWarnings({"unchecked"})
7965
public static <T> T instrumentCreateClientProxy(
8066
ClassLoader classLoader,
@@ -94,28 +80,28 @@ public Object invoke(Object proxy1, Method method, Object[] args) throws Throwab
9480
Context parentContext = Context.current();
9581
JsonRpcClientRequest request = new JsonRpcClientRequest(method, args);
9682
if (!CLIENT_INSTRUMENTER.shouldStart(parentContext, request)) {
97-
return method.invoke(proxy, args);
83+
try {
84+
return method.invoke(proxy, args);
85+
} catch (InvocationTargetException exception) {
86+
throw exception.getCause();
87+
}
9888
}
9989

10090
Context context = CLIENT_INSTRUMENTER.start(parentContext, request);
101-
Scope scope = context.makeCurrent();
102-
try {
103-
Object result = method.invoke(proxy, args);
104-
// after invoke
105-
scope.close();
106-
CLIENT_INSTRUMENTER.end(
107-
context,
108-
new JsonRpcClientRequest(method, args),
109-
new JsonRpcClientResponse(result),
110-
null);
111-
return result;
112-
91+
Object result;
92+
try (Scope scope = context.makeCurrent()) {
93+
result = method.invoke(proxy, args);
11394
} catch (Throwable t) {
11495
// after invoke
115-
scope.close();
11696
CLIENT_INSTRUMENTER.end(context, request, null, t);
11797
throw t;
11898
}
99+
CLIENT_INSTRUMENTER.end(
100+
context,
101+
new JsonRpcClientRequest(method, args),
102+
new JsonRpcClientResponse(result),
103+
null);
104+
return result;
119105
}
120106
});
121107
}

‎instrumentation/jsonrpc4j-1.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jsonrpc4j/v1_3/JsonRpcServerInstrumentation.java

+9-24
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
package io.opentelemetry.javaagent.instrumentation.jsonrpc4j.v1_3;
77

8-
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
98
import static io.opentelemetry.javaagent.instrumentation.jsonrpc4j.v1_3.JsonRpcSingletons.SERVER_INVOCATION_LISTENER;
109
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
1110
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
@@ -14,21 +13,14 @@
1413
import com.googlecode.jsonrpc4j.InvocationListener;
1514
import com.googlecode.jsonrpc4j.JsonRpcBasicServer;
1615
import com.googlecode.jsonrpc4j.MultipleInvocationListener;
17-
import io.opentelemetry.instrumentation.api.util.VirtualField;
1816
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
1917
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
2018
import net.bytebuddy.asm.Advice;
2119
import net.bytebuddy.description.type.TypeDescription;
22-
import net.bytebuddy.implementation.bytecode.assign.Assigner;
2320
import net.bytebuddy.matcher.ElementMatcher;
2421

2522
public class JsonRpcServerInstrumentation implements TypeInstrumentation {
2623

27-
@Override
28-
public ElementMatcher<ClassLoader> classLoaderOptimization() {
29-
return hasClassesNamed("com.googlecode.jsonrpc4j.JsonRpcBasicServer");
30-
}
31-
3224
@Override
3325
public ElementMatcher<TypeDescription> typeMatcher() {
3426
return named("com.googlecode.jsonrpc4j.JsonRpcBasicServer");
@@ -62,22 +54,15 @@ public static class SetInvocationListenerAdvice {
6254
@Advice.OnMethodEnter(suppress = Throwable.class)
6355
public static void setInvocationListener(
6456
@Advice.This JsonRpcBasicServer jsonRpcServer,
65-
@Advice.Argument(value = 0, readOnly = false, typing = Assigner.Typing.DYNAMIC)
66-
InvocationListener invocationListener) {
67-
VirtualField<JsonRpcBasicServer, Boolean> instrumented =
68-
VirtualField.find(JsonRpcBasicServer.class, Boolean.class);
69-
if (!Boolean.TRUE.equals(instrumented.get(jsonRpcServer))) {
70-
if (invocationListener == null) {
71-
invocationListener = SERVER_INVOCATION_LISTENER;
72-
} else if (invocationListener instanceof MultipleInvocationListener) {
73-
((MultipleInvocationListener) invocationListener)
74-
.addInvocationListener(SERVER_INVOCATION_LISTENER);
75-
} else {
76-
invocationListener =
77-
new MultipleInvocationListener(invocationListener, SERVER_INVOCATION_LISTENER);
78-
}
79-
80-
instrumented.set(jsonRpcServer, true);
57+
@Advice.Argument(value = 0, readOnly = false) InvocationListener invocationListener) {
58+
if (invocationListener == null) {
59+
invocationListener = SERVER_INVOCATION_LISTENER;
60+
} else if (invocationListener instanceof MultipleInvocationListener) {
61+
((MultipleInvocationListener) invocationListener)
62+
.addInvocationListener(SERVER_INVOCATION_LISTENER);
63+
} else if (invocationListener != SERVER_INVOCATION_LISTENER) {
64+
invocationListener =
65+
new MultipleInvocationListener(invocationListener, SERVER_INVOCATION_LISTENER);
8166
}
8267
}
8368
}

‎instrumentation/jsonrpc4j-1.3/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/jsonrpc4j/v1_3/JettyServer.java

+3-4
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,15 @@
99
import com.googlecode.jsonrpc4j.JsonRpcServer;
1010
import java.io.IOException;
1111
import java.lang.reflect.InvocationTargetException;
12-
import java.util.Random;
1312
import javax.servlet.ServletException;
1413
import javax.servlet.http.HttpServlet;
1514
import javax.servlet.http.HttpServletRequest;
1615
import javax.servlet.http.HttpServletResponse;
1716
import org.eclipse.jetty.server.Server;
17+
import org.eclipse.jetty.server.ServerConnector;
1818
import org.eclipse.jetty.servlet.ServletContextHandler;
1919
import org.eclipse.jetty.servlet.ServletHolder;
2020

21-
@SuppressWarnings("WeakerAccess")
2221
public class JettyServer implements AutoCloseable {
2322

2423
public static final String DEFAULT_LOCAL_HOSTNAME = "127.0.0.1";
@@ -40,14 +39,14 @@ public String getCustomServerUrlString(String servletName) {
4039
}
4140

4241
public void startup() throws Exception {
43-
port = 10000 + new Random().nextInt(30000);
44-
jetty = new Server(port);
42+
jetty = new Server(0);
4543
ServletContextHandler context = new ServletContextHandler(ServletContextHandler.NO_SESSIONS);
4644
context.setContextPath("/");
4745
jetty.setHandler(context);
4846
ServletHolder servlet = context.addServlet(JsonRpcTestServlet.class, "/" + SERVLET);
4947
servlet.setInitParameter("class", service.getCanonicalName());
5048
jetty.start();
49+
port = ((ServerConnector) jetty.getConnectors()[0]).getLocalPort();
5150
}
5251

5352
@Override

‎instrumentation/jsonrpc4j-1.3/library/build.gradle.kts

+1-4
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,9 @@ plugins {
22
id("otel.library-instrumentation")
33
}
44

5-
val jacksonVersion = "2.13.3"
6-
75
dependencies {
86
library("com.github.briandilley.jsonrpc4j:jsonrpc4j:1.3.3")
9-
10-
library("com.fasterxml.jackson.core:jackson-databind:$jacksonVersion")
7+
library("com.fasterxml.jackson.core:jackson-databind")
118

129
testImplementation(project(":instrumentation:jsonrpc4j-1.3:testing"))
1310
}

‎instrumentation/jsonrpc4j-1.3/library/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/JsonRpcClientAttributesExtractor.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package io.opentelemetry.instrumentation.jsonrpc4j.v1_3;
77

8+
import io.opentelemetry.api.common.AttributeKey;
89
import io.opentelemetry.api.common.AttributesBuilder;
910
import io.opentelemetry.context.Context;
1011
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
@@ -14,10 +15,14 @@
1415
final class JsonRpcClientAttributesExtractor
1516
implements AttributesExtractor<JsonRpcClientRequest, JsonRpcClientResponse> {
1617

18+
// copied from RpcIncubatingAttributes
19+
private static final AttributeKey<String> RPC_JSONRPC_VERSION =
20+
AttributeKey.stringKey("rpc.jsonrpc.version");
21+
1722
@Override
1823
public void onStart(
1924
AttributesBuilder attributes, Context parentContext, JsonRpcClientRequest jsonRpcRequest) {
20-
attributes.put("rpc.jsonrpc.version", "2.0");
25+
attributes.put(RPC_JSONRPC_VERSION, "2.0");
2126
}
2227

2328
@Override

‎instrumentation/jsonrpc4j-1.3/library/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/JsonRpcClientAttributesGetter.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
// Check
1111
// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-metrics.md#attributes
1212
// Check https://opentelemetry.io/docs/specs/semconv/rpc/json-rpc/
13-
public enum JsonRpcClientAttributesGetter implements RpcAttributesGetter<JsonRpcClientRequest> {
13+
enum JsonRpcClientAttributesGetter implements RpcAttributesGetter<JsonRpcClientRequest> {
1414
INSTANCE;
1515

1616
@Override

‎instrumentation/jsonrpc4j-1.3/library/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/JsonRpcClientSpanNameExtractor.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
99
import java.lang.reflect.Method;
1010

11-
public class JsonRpcClientSpanNameExtractor implements SpanNameExtractor<JsonRpcClientRequest> {
11+
final class JsonRpcClientSpanNameExtractor implements SpanNameExtractor<JsonRpcClientRequest> {
1212
@Override
1313
public String extract(JsonRpcClientRequest request) {
1414
if (request.getMethod() == null) {

‎instrumentation/jsonrpc4j-1.3/library/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/JsonRpcServerAttributesExtractor.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,20 @@
1919
final class JsonRpcServerAttributesExtractor
2020
implements AttributesExtractor<JsonRpcServerRequest, JsonRpcServerResponse> {
2121

22+
// copied from RpcIncubatingAttributes
2223
private static final AttributeKey<Long> RPC_JSONRPC_ERROR_CODE =
2324
AttributeKey.longKey("rpc.jsonrpc.error_code");
24-
2525
private static final AttributeKey<String> RPC_JSONRPC_ERROR_MESSAGE =
2626
AttributeKey.stringKey("rpc.jsonrpc.error_message");
27+
private static final AttributeKey<String> RPC_JSONRPC_VERSION =
28+
AttributeKey.stringKey("rpc.jsonrpc.version");
2729

2830
@Override
2931
public void onStart(
3032
AttributesBuilder attributes,
3133
Context parentContext,
3234
JsonRpcServerRequest jsonRpcServerRequest) {
33-
attributes.put("rpc.jsonrpc.version", "2.0");
35+
attributes.put(RPC_JSONRPC_VERSION, "2.0");
3436
}
3537

3638
@Override
@@ -50,8 +52,6 @@ public void onEnd(
5052
error, jsonRpcServerRequest.getMethod(), jsonRpcServerRequest.getArguments());
5153
attributes.put(RPC_JSONRPC_ERROR_CODE, jsonError.code);
5254
attributes.put(RPC_JSONRPC_ERROR_MESSAGE, jsonError.message);
53-
} else {
54-
attributes.put(RPC_JSONRPC_ERROR_CODE, ErrorResolver.JsonError.OK.code);
5555
}
5656
}
5757
}

‎instrumentation/jsonrpc4j-1.3/library/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/JsonRpcServerAttributesGetter.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
// Check
1111
// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/rpc/rpc-metrics.md#attributes
1212
// Check https://opentelemetry.io/docs/specs/semconv/rpc/json-rpc/
13-
public enum JsonRpcServerAttributesGetter implements RpcAttributesGetter<JsonRpcServerRequest> {
13+
enum JsonRpcServerAttributesGetter implements RpcAttributesGetter<JsonRpcServerRequest> {
1414
INSTANCE;
1515

1616
@Override

‎instrumentation/jsonrpc4j-1.3/library/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/JsonRpcServerRequest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import java.lang.reflect.Method;
1010
import java.util.List;
1111

12-
public final class JsonRpcServerRequest {
12+
final class JsonRpcServerRequest {
1313

1414
private final Method method;
1515
private final List<JsonNode> arguments;

0 commit comments

Comments
 (0)