[ISSUE #10377] Nacos client observability enhancement - trace part #11138

FAWC438 · 2023-09-15T03:32:21Z

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

The purpose of this PR is to enhance the observability of Nacos client. This pr ensures that users can obtain metric through OpenTelemetry and Prometheus; obtain trace through OpenTelemetry.

This PR is only related to the trace part.

Related issue: #10377

Brief changelog

Trace

Using OpenTelemetry API to create traces that fully complies with the OpenTelemetry standard
All Nacos client spans are divided into 3 levels
1. Service: The span buried points at this level are mainly concentrated in functions/classes that Nacos users can directly call.
2. Work: The span buried points at this level are mainly concentrated in the core code logic of the Nacos client, including but not limited to listener processing, server request processing, etc.
3. HTTP/RPC: The span buried points at this level are mainly focused on the network communication level of the Nacos client, that is, the observation of HTTP/RPC entrances and exits.
Users can obtain trace indicators using any method recommended by OpenTelemetry, including automatic Instrumentation and manual Instrumentation
Related unit tests. It also includes test cases that can export traces directly to jaeger (jaeger requires additional deployment)

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.

pixystone

Need to discuss.

pixystone · 2023-09-18T06:13:25Z

client/pom.xml

+        <dependency>
+            <groupId>io.opentelemetry</groupId>
+            <artifactId>opentelemetry-semconv</artifactId>
+            <version>1.29.0-alpha</version>


Is there any stable version?

The opentelemetry-semconv library is actually designed to make the span field naming comply with otlp regulations. It itself can be regarded as a collection of global variables. However, all versions of this library have the alpha suffix, which is the case in the official examples and also in the maven repository https://mvnrepository.com/artifact/io.opentelemetry/opentelemetry-semconv

pixystone · 2023-09-18T06:17:47Z

client/src/main/java/com/alibaba/nacos/client/config/NacosConfigService.java

+        Span addListenerSpan = ConfigTrace.getClientConfigServiceSpan("addTenantListeners");
+        try (Scope ignored = addListenerSpan.makeCurrent()) {
+
+            worker.addTenantListenersWithContent(dataId, group, content, encryptedDataKey, Arrays.asList(listener));


Tracing process may disturb adding listeners. It is expected that nothing can interrupt the main behaviors.
Make sure that span.makeCurrent() and span.isRecording() may not throw any exceptions in any situations.

All my trace-related operations are implemented through the OpenTelemetry API, including the span life cycle. OpenTelemetry clearly states it in the documentation: Unless your application imports the OpenTelemetry SDK, your instrumentation does nothing and does not impact application performance. https://opentelemetry.io/docs/concepts/instrumentation/libraries/#opentelemetry-api

pixystone · 2023-09-18T06:27:54Z

client/src/main/java/com/alibaba/nacos/client/config/NacosConfigService.java

+                    .queryConfig(dataId, group, worker.getAgent().getTenant(), timeoutMs, false);
+
+            if (queryConfigSpan.isRecording()) {
+                queryConfigSpan.setAttribute("function.current.name",


Please use constants instead of magic value here.

I will fix it soon

pixystone · 2023-09-18T06:31:40Z

client/src/main/java/com/alibaba/nacos/client/config/NacosConfigService.java

@@ -101,11 +105,60 @@ public String getConfig(String dataId, String group, long timeoutMs) throws Naco
    public String getConfigAndSignListener(String dataId, String group, long timeoutMs, Listener listener)
            throws NacosException {
        group = StringUtils.isBlank(group) ? Constants.DEFAULT_GROUP : group.trim();
-        ConfigResponse configResponse = worker.getAgent()
-                .queryConfig(dataId, group, worker.getAgent().getTenant(), timeoutMs, false);
+


Using an enhanced subclass of ClientWorker or AOP processing is a better way to wrap or decorate these methods.

Indeed, I will think about it carefully and discuss it with you

An enhanced subclass may be better, since a span could include more than one method

pixystone · 2023-09-18T06:35:38Z

client/src/main/java/com/alibaba/nacos/client/config/NacosConfigService.java

        if (content != null) {
            LOGGER.warn("[{}] [get-config] get failover ok, dataId={}, group={}, tenant={}, config={}",
                    worker.getAgentName(), dataId, group, tenant, ContentUtils.truncateContent(content));
            cr.setContent(content);
-            String encryptedDataKey = LocalEncryptedDataKeyProcessor
-                    .getEncryptDataKeyFailover(agent.getName(), dataId, group, tenant);
+            String encryptedDataKey = LocalEncryptedDataKeyProcessor.getEncryptDataKeyFailover(agent.getName(), dataId,


Tracing the update of local failover cache is also important.

No problem, I will do it here

pixystone · 2023-09-18T07:35:14Z

client/src/main/java/com/alibaba/nacos/client/naming/remote/gprc/NamingGrpcClientProxy.java

+            requestToServer(request, BatchInstanceResponse.class);
+            redoService.instanceRegistered(serviceName, groupName);
+
+            if (span.isRecording()) {


Some attributes can be set before doing actions in order to record more information for exceptions.

For some attributes, especially those that are expensive to calculate ( eg. some instance states), should be added if span is recording. https://opentelemetry.io/docs/concepts/instrumentation/libraries/#performance However, some attributes in the Nacos client may not need to be operated in this way (in fact, I have made the version number of the nacos client a must-have attribute for each span). I will discuss it with you in detail.

… when exceptions occur

…metry doc

…methods

pixystone

LGTM. Request some further changes in another PR.

pixystone · 2023-10-30T02:06:53Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

@@ -1098,10 +1158,19 @@ public ConfigResponse queryConfig(String dataId, String group, String tenant, lo
        }

        private Response requestProxy(RpcClient rpcClientInner, Request request) throws NacosException {
-            return requestProxy(rpcClientInner, request, 3000L);
+            return agent.requestProxy(rpcClientInner, request, 3000L);


This is a method with some default values. Please use this.requestProxy to keep modification of the origin method like metrics or tracing enhancement.

pixystone · 2023-10-30T02:07:03Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

@@ -477,8 +501,11 @@ private void refreshContentAndCheck(String groupKey, boolean notify) {

    private void refreshContentAndCheck(CacheData cacheData, boolean notify) {
        try {
-            ConfigResponse response = getServerConfig(cacheData.dataId, cacheData.group, cacheData.tenant, 3000L,
-                    notify);
+            String group = cacheData.group;


I suppose getServerConfig is same.

pixystone · 2023-10-30T02:21:14Z

client/src/main/java/com/alibaba/nacos/client/monitor/TraceDynamicProxy.java

There is no reason to use dynamic proxy. It would be enough if just delegating all ClientWorker methods.

FAWC438 and others added 30 commits July 3, 2023 19:26

Use Micrometer to monitor the metrics previously detected by Prometheus

9e3536e

Replace all Prometheus implement to Micrometer

4bbc318

Merge branch 'alibaba:develop' into develop-issue#10377

d61992d

Add unit test

d6554e3

Merge branch 'alibaba:develop' into develop-issue#10377

c5dcb08

Merge branch 'alibaba:develop' into develop-issue#10377

0c501c0

Define unit test case

64a7189

Remove unnecessary dependencies

88beb24

Remove unnecessary dependencies

6aa4cf4

Fix magic value

61db466

Optimize code architecture

e50b2ac

Use a new CompositeMeterRegistry instead of the globalRegistry

f1a916c

Use NacosClientProperties to get the env value

2681739

Finish configNotifyCostDuration metric

52e1557

Finish all config metric

54ee6f4

Finish record naming rpc request duration

248d406

Finish all naming meters

b515014

Fix meter names

d971376

Add unit tests

d3d9226

Add Config trace spans

72fdf26

Add Naming trace spans

877cc12

Add trace unit test

b82af66

Inject trace context with request headers

1c14f87

Test trace to Jaeger

c3ee3a5

Add config trace nested spans

656d7e5

Add naming trace nested spans

51e6809

Add naming trace namespace attr and fix unit tests

58587df

Fix Jaeger test case

6cfe500

Update author info

b8aeb1a

Add config serverNumber metric

11b66fd

FAWC438 added 2 commits September 12, 2023 15:36

Fix trace tests assert

7fc120c

Add server request handler traces

4bb7f87

pixystone suggested changes Sep 18, 2023

View reviewed changes

FAWC438 added 3 commits September 18, 2023 20:28

Fix config magic values and make sure config trace attributes are set…

ec5aeac

… when exceptions occur

Fix naming magic values and make sure naming trace attributes are set…

c99aa7d

… when exceptions occur

Add trace about EncryptDataKey

e0a120f

KomachiSion added the Summer label Sep 19, 2023

FAWC438 requested a review from pixystone September 19, 2023 09:31

FAWC438 changed the title ~~[OSPP #10377] Nacos client observability enhancement~~ [ISSUE #10377] Nacos client observability enhancement Sep 21, 2023

FAWC438 changed the title ~~[ISSUE #10377] Nacos client observability enhancement~~ [ISSUE #10377] Nacos client observability enhancement - trace part Sep 21, 2023

FAWC438 added 2 commits September 21, 2023 11:16

Roll back enhanced subclass

d679112

Roll back enhanced subclass

2f0724f

FAWC438 force-pushed the trace branch from 9428e5e to 2f0724f Compare September 21, 2023 03:22

FAWC438 added 12 commits September 21, 2023 13:29

Nacos common no longer depends on opentelemetry-api

88e9ce1

Add spanProxy

cd9d73c

Set spanProxy params type to SpanBuilder

74b0e44

Set the span kind of outgoing spans to SpanKind.CLIENT

d238d46

Call getTracer() every time when acquiring spans to follow OpenTele…

5fbe8ab

…metry doc

Fix a null pointer issue

eb77bd3

Using dynamic proxy to tracing ClientWorker

9ebd64f

Using dynamic proxy to tracing all config module, except some static …

80ea766

…methods

Using dynamic proxy to tracing Service level naming spans

e27e0d1

Using dynamic proxy to tracing naming redo service

03df79e

Almost all trace spans are refactored by JDK dynamic proxy

d6e0ade

Add unit tests for TraceDynamicProxy

b78d68d

pixystone approved these changes Oct 30, 2023

View reviewed changes

Merge branch 'summer-ospp#10377' into trace

6426151

KomachiSion approved these changes Oct 30, 2023

View reviewed changes

KomachiSion merged commit e955313 into alibaba:summer-ospp#10377 Oct 30, 2023
1 check passed

FAWC438 mentioned this pull request Dec 26, 2023

[ISSUE #10377] Migrating Client OpenTelemetry trace from dynamic proxies to delegate classes #11559

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #10377] Nacos client observability enhancement - trace part #11138

[ISSUE #10377] Nacos client observability enhancement - trace part #11138

FAWC438 commented Sep 15, 2023 •

edited

Loading

pixystone left a comment

pixystone Sep 18, 2023

FAWC438 Sep 18, 2023

pixystone Sep 18, 2023

FAWC438 Sep 18, 2023

pixystone Sep 18, 2023

FAWC438 Sep 18, 2023

pixystone Sep 18, 2023

FAWC438 Sep 18, 2023

FAWC438 Sep 18, 2023

pixystone Sep 18, 2023

FAWC438 Sep 18, 2023

pixystone Sep 18, 2023

FAWC438 Sep 18, 2023

pixystone left a comment

pixystone Oct 30, 2023

pixystone Oct 30, 2023

pixystone Oct 30, 2023

[ISSUE #10377] Nacos client observability enhancement - trace part #11138

[ISSUE #10377] Nacos client observability enhancement - trace part #11138

Conversation

FAWC438 commented Sep 15, 2023 • edited Loading

What is the purpose of the change

Brief changelog

Trace

Verifying this change

pixystone left a 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

pixystone left a 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

FAWC438 commented Sep 15, 2023 •

edited

Loading