Add support for Nacos Client (#9961) #12849

wl2027 · 2024-12-08T05:22:08Z

Enhancement Methods

com.alibaba.nacos.common.remote.client.grpc.GrpcConnection#request
com.alibaba.nacos.common.remote.client.RpcClient#handleServerRequest

Enable Configuration

Configuration Items	Default Value
`otel.instrumentation.nacos-client.default-enabled`	`false`

Span Info Details

Request Child Class	SpanName	Additional Tags
InstanceRequest	Nacos/{$(lnstanceRequest.getType()}	nacos.namespace nacos.group nacos.service.name
ServiceQueryRequest	Nacos/queryService
SubscribeServiceRequest	Nacos/subscribeService,Nacos/unsubscribeService
ServicelistRequest	Nacos/getServicelist
ConfigQueryRequest	Nacos/queryConfig
ConfigPublishRequest	Nacos/publishConfig	nacos.data.id nacos.group nacos.tenant
ConfigRemoveRequest	Nacos/removeConfig
ConfigChangeNotifyRequest	Nacos/notifyConfigChange
NotifySubscriberRequest	Nacos/notifySubscribeChange	nacos.group nacos.service.name

linux-foundation-easycla · 2024-12-08T05:22:13Z

The committers listed above are authorized under a signed CLA.

✅ login: wl2027 / name: wl2027 (5af892d, 72e3a8e, e376621, ed8435b, afa0080, 016e9d6)

- Improved code formatting - Enhanced muzzle validation - Managed dependency versions

laurit · 2024-12-18T13:42:49Z

docs/supported-libraries.md

@@ -101,6 +101,7 @@ These are the supported libraries and frameworks:
 | [Micrometer](https://micrometer.io/)                                                                                                        | 1.5+                               | [opentelemetry-micrometer-1.5](../instrumentation/micrometer/micrometer-1.5/library)                                                                                                                                                                                                                                                                                                    | none                                                                                                                                    |
 | [MongoDB Driver](https://mongodb.github.io/mongo-java-driver/)                                                                              | 3.1+                               | [opentelemetry-mongo-3.1](../instrumentation/mongo/mongo-3.1/library)                                                                                                                                                                                                                                                                                                                   | [Database Client Spans]                                                                                                                 |
 | [MyBatis](https://mybatis.org/mybatis-3/)                                                                                                   | 3.2+                               | N/A                                                                                                                                                                                                                                                                                                                                                                                     | none                                                                                                                                    |
+| [Nacos](https://nacos.io/)                                                                                                                  | 2.0.3+                             | N/A                                                                                                                                                                                                                                                                                                                                                                                     | none                                                                                                                                    |


Perhaps use Nacos Client since the instrumentation is for the client

Okay, I will make changes here.

laurit · 2024-12-18T13:44:35Z

instrumentation/nacos-client-2.0.3/javaagent/build.gradle.kts

+}
+
+dependencies {
+  val nacosClientVersion = "2.0.3"


usually we don't use a separate variable for the instrumented library version, since this is used only in 1 place you could inline it
why are you suing 2.0.3, as far as I can tell tests pass with 2.0.0

The version 2.0.3 was chosen because I referred to the requirements described in issue #9961. However, after testing, I found that version 2.0.0 also works. I will make changes here.

laurit · 2024-12-18T13:48:49Z

instrumentation/nacos-client-2.0.3/javaagent/build.gradle.kts

+
+dependencies {
+  val nacosClientVersion = "2.0.3"
+  implementation("com.alibaba.nacos:nacos-client:$nacosClientVersion")


implementation may not be used here. implementation dependencies are bundled inside the agent jar which is not desired. Usually we use library that is our custom scope that means compileOnly + testImplementation. library also supports running tests with the latest version of the library with -PtestLatestDeps=true (currently tests don't compile with the latest version, you can use latestDepTestLibrary to limit the latest version or you could define tests suites)

Okay, I will make changes here.

laurit · 2024-12-18T13:48:59Z

instrumentation/nacos-client-2.0.3/javaagent/build.gradle.kts

+tasks.withType<Test>().configureEach {
+  jvmArgs("--add-opens=java.base/java.lang=ALL-UNNAMED")
+  jvmArgs("-XX:+IgnoreUnrecognizedVMOptions")
+}


is this really needed?

Okay, I will make changes here.

laurit · 2024-12-18T13:50:43Z

...elemetry/javaagent/instrumentation/nacos/client/v2_0_3/NacosClientInstrumentationModule.java

+
+  @Override
+  public List<TypeInstrumentation> typeInstrumentations() {
+    return Arrays.asList(new GrpcConnectionInstrumentation(), new RpcClientInstrumentation());


usually we static import Arrays.asList

Okay, I will make changes here.

laurit · 2024-12-18T13:58:20Z

...gent/instrumentation/nacos/client/v2_0_3/instrumentations/GrpcConnectionInstrumentation.java

+  @Override
+  public void transform(TypeTransformer transformer) {
+    transformer.applyAdviceToMethod(
+        isMethod().and(isPublic()).and(namedOneOf("request")).and(returns(Response.class)),


usually we use named instead of namedOneOf when there is only one name
don't use returns(Response.class), use returns(named("com.alibaba.nacos.api.remote.response.Response")). This way you don't need to bundle the Response class inside the agent.

Okay, I will make changes here.

laurit · 2024-12-18T14:00:10Z

...gent/instrumentation/nacos/client/v2_0_3/instrumentations/GrpcConnectionInstrumentation.java

+  public void transform(TypeTransformer transformer) {
+    transformer.applyAdviceToMethod(
+        isMethod().and(isPublic()).and(namedOneOf("request")).and(returns(Response.class)),
+        GrpcConnectionRequestAdvice.class.getName());


Usually we have advice classes as inner classes and here use this.getClass() + "$GrpcConnectionRequestAdvice". We avoid using the full class name to avoid loading the advice class when the advice is added, this way we can use the types from instrumented library inside the advice class without bundling the instrumented library inside the agent.

Okay, I will make changes here.

laurit · 2024-12-18T14:00:49Z

...ient-2.0.3/javaagent/src/test/java/com/alibaba/nacos/common/remote/client/RpcClientTest.java

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.alibaba.nacos.common.remote.client;


usually we place test classes in the same package as the instrumentation

I did this because com.alibaba.nacos.common.remote.client.RpcClient#handleServerRequest is a protected method. Is there a better way to handle this here?

laurit · 2024-12-18T14:02:59Z

...ient-2.0.3/javaagent/src/test/java/com/alibaba/nacos/common/remote/client/RpcClientTest.java

+  public void handleServerRequestSuccessResponse() {
+    when(serverRequestHandler.requestReply(any(Request.class)))
+        .thenReturn(NacosClientTestHelper.SUCCESS_RESPONSE);
+    for (Request request : nacosClientRequestList) {


when we need to run the same test with different arguments we use @ParameterizedTest

Okay, I will make changes here.

laurit · 2024-12-18T14:04:03Z

...gent/src/test/java/com/alibaba/nacos/shaded/com/google/errorprone/annotations/DoNotMock.java

+@Target(TYPE)
+@Retention(RUNTIME)
+@Documented
+public @interface DoNotMock {


is this used anywhere?

The class added here is to resolve a test compilation error in the nacos-client version 2.0.3 jar:

Compilation failed; see the compiler output below.
Error: Warning found, but specified -Werror
G:\temp.gradle\caches\modules-2\files-2.1\com.alibaba.nacos\nacos-client\2.0.3\a73c03b466a1f1565ee2b0e459c43046d6c8c15b\nacos-client-2.0.3.jar(/com/alibaba/nacos/shaded/com/google/common/util/concurrent/ListenableFuture.class): Warning: Unable to find class 'DoNotMock' annotation method 'value()': Can't find it com.alibaba.nacos.shaded.com.google.errorprone.annotations.DoNotMock class file.

This issue can be reproduced by removing the class file DoNotMock and then running ./gradlew :instrumentation:nacos-client-2.0.0:javaagent:test -PtestLatestDeps=true.

- Updated version to support Nacos client version 2.0.0 - Default the plugin to be disabled

Add support for Nacos Client (open-telemetry#9961)

72e3a8e

wl2027 requested a review from a team as a code owner December 8, 2024 05:22

github-actions bot requested a review from theletterf December 8, 2024 05:22

wl2027 added 2 commits December 8, 2024 14:50

refactor(nacos-client): Improve code formatting and muzzle validation

016e9d6

- Improved code formatting - Enhanced muzzle validation - Managed dependency versions

Update(nacos-client): Improve code formatting and muzzle validation

5af892d

123liuziming mentioned this pull request Dec 12, 2024

[Proposal] Semantic conventions for config/registry center open-telemetry/semantic-conventions#1678

Open

laurit reviewed Dec 18, 2024

View reviewed changes

wl2027 and others added 3 commits December 21, 2024 13:00

fix: Update version and code style

afa0080

- Updated version to support Nacos client version 2.0.0 - Default the plugin to be disabled

Merge branch 'open-telemetry:main' into main

ed8435b

fix ci

e376621

wl2027 requested a review from laurit December 21, 2024 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Nacos Client (#9961) #12849

Add support for Nacos Client (#9961) #12849

wl2027 commented Dec 8, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 8, 2024 •

edited

Loading

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024

laurit Dec 18, 2024

wl2027 Dec 21, 2024 •

edited

Loading

Add support for Nacos Client (#9961) #12849

Are you sure you want to change the base?

Add support for Nacos Client (#9961) #12849

Conversation

wl2027 commented Dec 8, 2024 • edited Loading

Enhancement Methods

Enable Configuration

Span Info Details

linux-foundation-easycla bot commented Dec 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wl2027 Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

wl2027 commented Dec 8, 2024 •

edited

Loading

linux-foundation-easycla bot commented Dec 8, 2024 •

edited

Loading

wl2027 Dec 21, 2024 •

edited

Loading