Skip to content

Conversation

@wl2027
Copy link
Contributor

@wl2027 wl2027 commented Dec 8, 2024

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.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

@wl2027 wl2027 requested a review from a team as a code owner December 8, 2024 05:22
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from theletterf December 8, 2024 05:22
* SPDX-License-Identifier: Apache-2.0
*/

package com.alibaba.nacos.common.remote.client;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@Target(TYPE)
@Retention(RUNTIME)
@Documented
public @interface DoNotMock {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used anywhere?

Copy link
Contributor Author

@wl2027 wl2027 Dec 21, 2024

Choose a reason for hiding this comment

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

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.

wl2027 and others added 3 commits December 21, 2024 13:00
- Updated version to support Nacos client version 2.0.0
- Default the plugin to be disabled
@wl2027 wl2027 requested a review from laurit December 21, 2024 08:49
Comment on lines +69 to +70
assertNotNull(response);
assertTrue(response.isSuccess());
Copy link
Member

Choose a reason for hiding this comment

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

in most of our tests, we use the assertThat() convention for consistency

Suggested change
assertNotNull(response);
assertTrue(response.isSuccess());
assertThat(response).isNotNull();
assertThat(response.isSuccess()).isTrue();

rpcClient.getClass().getName(), "handleServerRequest", request));
});
});
testing.clearData();
Copy link
Member

Choose a reason for hiding this comment

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

i wouldn't expect these clearData() method calls to be needed. were they added due to some issue you encountered?

Comment on lines +9 to +10
versions.set("[2.0.0,)")
skip("0.5.0", "0.6.1", "1.1.2", "1.1.4", "1.4.7")
Copy link
Member

Choose a reason for hiding this comment

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

you have the versions.set() to only include versions 2.0.0 and above of the nacos-client module, so I wouldn't expect to need to skip anything below 2.x ?

| [Micrometer](https://micrometer.io/) | 1.5+ (disabled by default) | [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], [Database Client Metrics] [6] |
| [MyBatis](https://mybatis.org/mybatis-3/) | 3.2+ | N/A | none |
| [Nacos Client](https://nacos.io/) | 2.0.0+ | N/A | none |
Copy link
Member

Choose a reason for hiding this comment

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

could you put a note in here that indicates it's disabled by default? similar to how Micrometer does it

wl2027 and others added 5 commits July 4, 2025 13:23
…opentelemetry/javaagent/instrumentation/nacos/client/v2_0_0/NacosClientSingletons.java

Co-authored-by: Jay DeLuca <[email protected]>
…/alibaba/nacos/common/remote/client/RpcClientTest.java

Co-authored-by: Jay DeLuca <[email protected]>
…/alibaba/nacos/common/remote/client/RpcClientTest.java

Co-authored-by: Jay DeLuca <[email protected]>
@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants