Add a forceIgnoring mechanism and apply it to the plugins (Spring Clo… #689

gzlicanyi · 2024-05-13T08:04:34Z

Add a forceIgnoring mechanism and apply it to the plugins (Spring Cloud Gateway, HttpClient, JDK-ForkJoinPool, JDK-Threading, JDK-ThreadPool, Toolkit-Trace, Toolkit-WebFlux)

Tests(including UT, IT, E2E) are added to verify the new feature.
Update the CHANGES log.

proposal

Motivation

context: apache/skywalking#12161.

Generally speaking, if the source Span is ignored, the downstream Span also needs to be ignored, such as in same-thread operations and cross-process operations. However, for Spans created by cross-thread operations, such as in the scenario of Spring Cloud Gateway, there is currently no mechanism to make them ignored along with the parent Span.

The force-ignoring mechanism is a supplement to the ignoring mechanism for cross-thread operations.

Architecture Graph

Proposed Changes

The reason why you can only modify within the ContextManager#createLocalSpan method, and not within the ContextManager#continued method, is because the latter cannot change an already created Context and Span.

I haven't found a way to support this feature without disrupting the existing API, because the tracing context is created before the snapshot is continued.

Currently modified plugins are:
httpasyncclient-4.x
httpclient-5.x
jdk-forkjoinpool
jdk-threading
jdk-threadpool
spring-cloud-gateway-2.0.x
spring-cloud-gateway-2.1.x
spring-cloud-gateway-3.x
spring-cloud-gateway-4.x
toolkit-trace-activation
toolkit-webflux-activation

Imported Dependencies libs and their licenses.

No new dependency.

Compatibility

In scenarios where it is necessary to keep the span node of the entire trace intact, such as in Spring Cloud Gateway, the call to the ContextManager#continued method needs to be replaced with the ContextManager#createLocalSpan method that includes a snapshot.

General usage docs

…ud Gateway, HttpClient, JDK-ForkJoinPool, JDK-Threading, JDK-ThreadPool, Toolkit-Trace, Toolkit-WebFlux)

wu-sheng · 2024-05-13T08:14:32Z

Please write a proposal about what is a force-ignoring mechanism. The discussion we did was only about the context.

gzlicanyi · 2024-05-13T08:17:31Z

Please write a proposal about what is a force-ignoring mechanism. The discussion we did was only about the context.

Do you have a proposal template?

wu-sheng · 2024-05-13T08:20:33Z

For an official proposal, you could refer to SWIP(https://skywalking.apache.org/docs/main/next/en/swip/readme/), which is mandatory for OAP, but not for the agent.
A key part of a proposal is about why you want to change, how you design looks like, what are the impacts. Then we could discuss for more details.

A reminder for you, the way you changes, is making a significant impact for existing APIs, this may not be a good way.

gzlicanyi · 2024-05-13T10:10:41Z

Please write a proposal about what is a force-ignoring mechanism. The discussion we did was only about the context.

Added.

wu-sheng · 2024-05-13T10:15:58Z

Add what?

gzlicanyi · 2024-05-13T10:18:39Z

Add what?

here: #689 (comment)

wu-sheng · 2024-05-13T10:31:45Z

I think we need to separate the context. Cross thread is different from cross process.
Cross process is defined here, https://skywalking.apache.org/docs/main/latest/en/api/x-process-propagation-headers-v3/. This can't be changed as it crosses all language agents.

About crosee thread, the status should only documented in the snapshot. But I am not clear about how you changed a sampled tracing context into a ignoring context?

wu-sheng · 2024-05-13T10:32:35Z

I am feeling you are doing a hijack only, which should not be the official way.

gzlicanyi · 2024-05-13T10:49:05Z

I think we need to separate the context. Cross thread is different from cross process. Cross process is defined here, https://skywalking.apache.org/docs/main/latest/en/api/x-process-propagation-headers-v3/. This can't be changed as it crosses all language agents.

About crosee thread, the status should only documented in the snapshot. But I am not clear about how you changed a sampled tracing context into a ignoring context?

This question can be divided into two parts:

In the scenario of cross-thread, is it necessary to continue the upstream context?
If the first point is necessary, how can we modify it to better align with the official way?

wu-sheng · 2024-05-13T10:56:04Z

I don't have direct answer, if I had, I already told you in the last discussion.

The reason it doesn't exist is, the tracing context is created before the snapshot is continued.

You proposal should cover about how.

The cross process is more complex. And sampling is service(process) oriented. There is a flag in the protocol about sampling, but you have to evaluate all agent codes(all languages), about whether there will be an impact.

wu-sheng · 2024-05-13T11:01:25Z

You are adding a mechanism for local span creation. This should not happen, if there is a context switching out, it must be controlled by ContextManager.

wu-sheng · 2024-05-13T11:42:50Z

When this is about ContextManager, it should not affect TracingContext#CreateLocal span. Only snapshot should be changed, and ContextManager asked TracingContext switching to IgnoringTracingCntext with the same stack. Also, the plugins should not be changed, and all span and context manager APIs should not be changed.

gzlicanyi · 2024-05-13T11:54:03Z

When this is about ContextManager, it should not affect TracingContext#CreateLocal span. Only snapshot should be changed, and ContextManager asked TracingContext switching to IgnoringTracingCntext with the same stack. Also, the plugins should not be changed, and all span and context manager APIs should not be changed.

If the TracingContext switches to IgnoringTracingContext in the continued method, the span reference created by the ContextManager#createLocalSpan method cannot be changed, and the plugin still uses the span before the switch.
Additionally, the already created but useless TracingContext will increment the sampling count of the operationName by one.

wu-sheng · 2024-05-13T12:30:05Z

I think you got it wrong. It should be in ContextManager#continued method, which controls the context.

wu-sheng · 2024-05-13T12:30:55Z

This is a force sampling(unsampling), we don't need to worry about sampling counter.

gzlicanyi · 2024-05-13T12:44:15Z

I think you got it wrong. It should be in ContextManager#continued method, which controls the context.

Understood. How can the span used by the plugin be changed to a NoopSpan?

gzlicanyi · 2024-05-13T12:57:35Z

This is a force sampling(unsampling), we don't need to worry about sampling counter.

ForceSampling might not necessarily be true. I feel that it could still impact the count of the SamplingService.

wu-sheng · 2024-05-13T13:05:56Z

This is a force sampling(unsampling), we don't need to worry about sampling counter.

ForceSampling might not necessarily be true. I feel that it could still impact the count of the SamplingService.

It just counts one more, so what? Sampling is never to be accurate.

wu-sheng · 2024-05-13T13:08:21Z

I think you got it wrong. It should be in ContextManager#continued method, which controls the context.

Understood. How can the span used by the plugin be changed to a NoopSpan?

Consider creating AbstractTracerContext#forceIgnoring. TracingContext#forceIgnoring could return a IgnoredTracerContext with the same stack depth, to make sure no tracing context stack leak. IgnoredTracerContext just simply returns this as no change is required.

gzlicanyi · 2024-05-14T09:38:52Z

I think you got it wrong. It should be in ContextManager#continued method, which controls the context.

Understood. How can the span used by the plugin be changed to a NoopSpan?

Consider creating AbstractTracerContext#forceIgnoring. TracingContext#forceIgnoring could return a IgnoredTracerContext with the same stack depth, to make sure no tracing context stack leak. IgnoredTracerContext just simply returns this as no change is required.

IgnoredTracerContext retains the activeSpanStack of TracingContext, do you think there's no problem with it?

gzlicanyi · 2024-05-14T09:41:25Z

I think you got it wrong. It should be in ContextManager#continued method, which controls the context.

Understood. How can the span used by the plugin be changed to a NoopSpan?

Consider creating AbstractTracerContext#forceIgnoring. TracingContext#forceIgnoring could return a IgnoredTracerContext with the same stack depth, to make sure no tracing context stack leak. IgnoredTracerContext just simply returns this as no change is required.

IgnoredTracerContext retains the activeSpanStack of TracingContext, do you think there's no problem with it?

If there is no problem, indeed the 'continued' method can complete the transition of the context. However, for the leftover spans, I am not entirely sure if they might cause other issues.

wu-sheng · 2024-05-14T10:39:25Z

Which problem do you mean No matter what spans they are. As they are not reported, they are fine to be GCed.

gzlicanyi · 2024-05-14T10:41:21Z

Which problem do you mean No matter what spans they are. As they are not reported, they are fine to be GCed.

I have made alterations using a new way and submitted another PR. Can you check if there are any problems?

wu-sheng · 2024-05-14T10:43:16Z

Please update here.

This reverts commit e5d4a48.

…ring Cloud Gateway, HttpClient, JDK-ForkJoinPool, JDK-Threading, JDK-ThreadPool, Toolkit-Trace, Toolkit-WebFlux)" This reverts commit d751b53.

wu-sheng · 2024-05-15T01:51:15Z

...nt-core/src/main/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContext.java

@@ -34,14 +34,21 @@ public class IgnoredTracerContext implements AbstractTracerContext {
    private static final NoopSpan NOOP_SPAN = new NoopSpan();
    private static final String IGNORE_TRACE = "Ignored_Trace";

+    private LinkedList<AbstractSpan> activeSpanStack;


Why need this?

Because the old span is still in use, if it's discarded, span#prepareForAsync will throw an error.

This is not friendly for GC. We could add ingored flag to the span and skip this check.

Another option maybe to create a ForceIgnoreTracerContext to deal with created span cases.
And this only happens in very rare case, so we should process that only when we checked all active spans, which have one running in async mode.

Do you prefer replacing the span's context with IgnoredTracerContext for old spans with old context references, or would you rather ignore any operations involving context within the span?

I didn't dig in so deeply, as I am feeling this change is not easy.

I just provided two possibilities, no preference. One key part is, what you are adding is not happening as always, you should not make the agent kernel costs too much.

I don't know whether this is possible.

That was why I said, this could be done easily if this only happens in your private fork. But in general, I don't have a good idea could be done easily.

But this has to be very good, as your changes are going to affect every user for sure.

wu-sheng · 2024-05-15T01:52:12Z

...nt-core/src/main/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContext.java

+    }
+
+    public IgnoredTracerContext(LinkedList<AbstractSpan> activeSpanStack) {
+        this.activeSpanStack = activeSpanStack;


We only need the depth to be initialized based on stack depth, rather than holding all

wu-sheng · 2024-05-15T01:52:47Z

...nt-core/src/main/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContext.java

+        this.activeSpanStack = new LinkedList<>();
+        this.correlationContext = new CorrelationContext();
+        this.extensionContext = new ExtensionContext();
+        this.profileStatusContext = ProfileStatusContext.createWithNone();


You just need to call new constructor.

wu-sheng · 2024-05-15T01:53:07Z

...nt-core/src/main/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContext.java

-        stackDepth++;
+        activeSpanStack.addLast(NOOP_SPAN);


All logic here should not be changed.

We just need depth, no further.

...pm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/ContextManager.java

...t-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java

...re/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/AbstractTracingSpan.java

wu-sheng · 2024-05-16T07:11:59Z

It seems the UTs failing. Please recheck and fix.

wu-sheng · 2024-05-16T13:33:25Z

Please follow the comments to polish the codes. Generally, this PR is good.

polish the codes. Co-authored-by: 吴晟 Wu Sheng <wu.sheng@foxmail.com>

wu-sheng · 2024-05-17T05:44:42Z

Thanks for adding this.

Add a forceIgnoring mechanism and apply it to the plugins (Spring Clo…

d751b53

…ud Gateway, HttpClient, JDK-ForkJoinPool, JDK-Threading, JDK-ThreadPool, Toolkit-Trace, Toolkit-WebFlux)

wu-sheng added feature TBD labels May 13, 2024

Fix the continued call within the createLocalSpan method.

e5d4a48

Add a forceIgnoring mechanism in a CROSS_THREAD scenario.

b579dcf

gzlicanyi mentioned this pull request May 14, 2024

Add a forceIgnoring mechanism in a CROSS_THREAD scenario. #692

Closed

2 tasks

gzlicanyi added 4 commits May 14, 2024 19:31

Revert "Fix the continued call within the createLocalSpan method."

8a793f4

This reverts commit e5d4a48.

Revert "Add a forceIgnoring mechanism and apply it to the plugins (Sp…

1f739cc

…ring Cloud Gateway, HttpClient, JDK-ForkJoinPool, JDK-Threading, JDK-ThreadPool, Toolkit-Trace, Toolkit-WebFlux)" This reverts commit d751b53.

Merge branch 'forceIgnoring_by_continued' into forceIgnoring

1aa68cc

Merge remote-tracking branch 'old-origin/main' into forceIgnoring

c10decc

wu-sheng requested changes May 15, 2024

View reviewed changes

wu-sheng added this to the 9.3.0 milestone May 15, 2024

wu-sheng added core and removed feature TBD labels May 15, 2024

Add ignored flag to span

77b1dc6

wu-sheng reviewed May 16, 2024

View reviewed changes

...pm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/ContextManager.java Show resolved Hide resolved

wu-sheng reviewed May 16, 2024

View reviewed changes

...t-core/src/main/java/org/apache/skywalking/apm/agent/core/context/AbstractTracerContext.java Outdated Show resolved Hide resolved

wu-sheng reviewed May 16, 2024

View reviewed changes

...re/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/AbstractTracingSpan.java Outdated Show resolved Hide resolved

gzlicanyi added 2 commits May 16, 2024 18:04

fix ContextSnapshot mock

a3eda47

Add the IgnoreSnapshot test in CallableOrRunnableInterceptorTest.

0e2368a

Apply suggestions from code review

321eb30

polish the codes. Co-authored-by: 吴晟 Wu Sheng <wu.sheng@foxmail.com>

wu-sheng approved these changes May 17, 2024

View reviewed changes

wu-sheng merged commit b608d74 into apache:main May 17, 2024
191 checks passed

wu-sheng added the enhancement New feature or request label May 17, 2024

gzlicanyi deleted the forceIgnoring branch May 17, 2024 06:05

Add a forceIgnoring mechanism and apply it to the plugins (Spring Clo… #689

Add a forceIgnoring mechanism and apply it to the plugins (Spring Clo… #689

Conversation

gzlicanyi commented May 13, 2024 • edited Loading

proposal

Motivation

Architecture Graph

Proposed Changes

Imported Dependencies libs and their licenses.

Compatibility

General usage docs

wu-sheng commented May 13, 2024

gzlicanyi commented May 13, 2024

wu-sheng commented May 13, 2024

gzlicanyi commented May 13, 2024

wu-sheng commented May 13, 2024

gzlicanyi commented May 13, 2024

wu-sheng commented May 13, 2024

wu-sheng commented May 13, 2024

gzlicanyi commented May 13, 2024

wu-sheng commented May 13, 2024

wu-sheng commented May 13, 2024

wu-sheng commented May 13, 2024

gzlicanyi commented May 13, 2024

wu-sheng commented May 13, 2024

wu-sheng commented May 13, 2024

gzlicanyi commented May 13, 2024

gzlicanyi commented May 13, 2024 • edited Loading

wu-sheng commented May 13, 2024

wu-sheng commented May 13, 2024 • edited Loading

gzlicanyi commented May 14, 2024

gzlicanyi commented May 14, 2024

wu-sheng commented May 14, 2024

gzlicanyi commented May 14, 2024

wu-sheng commented May 14, 2024

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

wu-sheng May 15, 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

wu-sheng commented May 16, 2024

wu-sheng commented May 16, 2024

wu-sheng commented May 17, 2024

gzlicanyi commented May 13, 2024 •

edited

Loading

gzlicanyi commented May 13, 2024 •

edited

Loading

wu-sheng commented May 13, 2024 •

edited

Loading

wu-sheng May 15, 2024 •

edited

Loading