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

Jump to bottom

feat: support instrumentation for jsonrpc4j #13008

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

feat: support instrumentation for jsonrpc4j #13008

wants to merge 17 commits into from

Conversation

chenlujjj
Copy link

@chenlujjj chenlujjj commented Jan 7, 2025 •
edited
Loading

To resolve: #6020

The instrumentation is for both server and client:

  • server: set JsonRpcServer's invocationListener as an instance of OpenTelemetryJsonRpcInvocationListener
  • client: transform the invoke method of both JsonRpcHttpClient and JsonRpcRestClient

I'm new to this repository, so I borrowed the implementation of grpc and http client instrumentation

@chenlujjj chenlujjj requested a review from a team as a code owner January 7, 2025 10:45
@chenlujjj chenlujjj marked this pull request as draft January 7, 2025 10:45
@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch 4 times, most recently from 55628cf to b605e52 Compare January 7, 2025 15:43
@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from b605e52 to 0190c13 Compare January 7, 2025 15:59
@chenlujjj
Copy link
Author

Hello @trask , could you please take a look when you have time?
I'm not quite sure the implementation is in the right way, so I guess an early review can be beneficial before everything is ready.

I'm also working on the testing.

@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from ee68060 to 6f50140 Compare January 8, 2025 12:19
steverao
steverao reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@steverao steverao left a comment

Choose a reason for hiding this comment

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

You forget to add related information in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md#libraries--frameworks

@adam-awx
Copy link

Can you add some readme for the standalone instrumentation since we already have it?

@adam-awx
Copy link

adam-awx commented Jan 13, 2025 •
edited
Loading

@chenlujjj Only AnnotationsErrorResolver and DefaultErrorResolver have changed from class to enum in version 1.3.0(no maven artifact). In theory, your instrumentation code should support jsonrpc4j 1.3.3+(the first available version >1.3.0 on maven central) instead of 1.6.0+.

This has been proved in the muzzle task.

@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from a54e28c to c8cd109 Compare January 13, 2025 08:29
@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from 861450e to 974c315 Compare January 15, 2025 08:50
@chenlujjj chenlujjj marked this pull request as ready for review January 15, 2025 10:47
@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch 5 times, most recently from d6fc515 to 8644438 Compare January 16, 2025 03:28
@chenlujjj
Copy link
Author

Hi @steverao I updated the code, could you take another look ?

@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from 8644438 to 06f0869 Compare January 16, 2025 11:58
laurit
laurit reviewed Jan 21, 2025
View reviewed changes
docs/supported-libraries.md Outdated Show resolved Hide resolved
instrumentation/jsonrpc4j-1.3/javaagent/build.gradle.kts Outdated Show resolved Hide resolved
instrumentation/jsonrpc4j-1.3/javaagent/build.gradle.kts Outdated Show resolved Hide resolved
.../io/opentelemetry/javaagent/instrumentation/jsonrpc4j/v1_3/JsonRpcClientInstrumentation.java Outdated Show resolved Hide resolved
.../io/opentelemetry/javaagent/instrumentation/jsonrpc4j/v1_3/JsonRpcClientInstrumentation.java Outdated Show resolved Hide resolved
instrumentation/jsonrpc4j-1.3/testing/build.gradle.kts Outdated Show resolved Hide resolved
instrumentation/jsonrpc4j-1.3/testing/build.gradle.kts Outdated Show resolved Hide resolved
...sting/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/AbstractJsonRpcTest.java Outdated Show resolved Hide resolved
...sting/src/main/java/io/opentelemetry/instrumentation/jsonrpc4j/v1_3/AbstractJsonRpcTest.java
"{\"jsonrpc\":\"2.0\",\"method\":\"add\",\"params\":[1,2],\"id\":1}"
.getBytes(UTF_8));
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
server.handleRequest(inputStream, outputStream);
Copy link
Contributor

Choose a reason for hiding this comment

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

this test does not verify that context is propagated, perhaps should use a real http request?

.../io/opentelemetry/instrumentation/jsonrpc4j/v1_3/OpenTelemetryJsonRpcInvocationListener.java Outdated Show resolved Hide resolved
@chenlujjj
Copy link
Author

@laurit Thank you for the review, I resolved some comments, will work on the remaining ones tomorrow

@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from 145e58d to c92c0ef Compare January 21, 2025 16:53
@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch 4 times, most recently from f781d8c to b40f27d Compare January 22, 2025 07:40
@chenlujjj chenlujjj force-pushed the feat/jsonrpc-instrumentation branch from b40f27d to 7972e68 Compare January 22, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adam-awx adam-awx adam-awx left review comments

@steverao steverao steverao left review comments

@laurit laurit laurit left review comments

@theletterf theletterf Awaiting requested review from theletterf

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for jsonrpc4j
4 participants
@chenlujjj @adam-awx @laurit @steverao