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.

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

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

Merged
merged 11 commits into from
May 17, 2024
Merged
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Release Notes.
* Fix typos in `URLParser`.
* Add support for `Derby`/`Sybase`/`SQLite`/`DB2`/`OceanBase` jdbc url format in `URLParser`.
* Optimize spring-plugins:scheduled-annotation-plugin compatibility about Spring 6.1.x support.
* Add a forceIgnoring mechanism in a CROSS_THREAD scenario.

All issues and pull requests are [here](https://github.com/apache/skywalking/milestone/213?closed=1)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,10 @@ public interface AbstractTracerContext {
* Get current primary endpoint name
*/
String getPrimaryEndpointName();

/**
* Change the current context to IgnoredTracerContext.
gzlicanyi marked this conversation as resolved.
Show resolved Hide resolved
*/
AbstractTracerContext forceIgnoring();

}
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,12 @@ public static void continued(ContextSnapshot snapshot) {
throw new IllegalArgumentException("ContextSnapshot can't be null.");
}
if (!snapshot.isFromCurrent()) {
get().continued(snapshot);
if (snapshot.isValid()) {
gzlicanyi marked this conversation as resolved.
Show resolved Hide resolved
get().continued(snapshot);
} else {
AbstractTracerContext context = get().forceIgnoring();
CONTEXT.set(context);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Why need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
image
image

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@wu-sheng wu-sheng May 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


private final CorrelationContext correlationContext;
private final ExtensionContext extensionContext;
private final ProfileStatusContext profileStatusContext;

private int stackDepth;

public IgnoredTracerContext() {
this.stackDepth = 0;
this.activeSpanStack = new LinkedList<>();
this.correlationContext = new CorrelationContext();
this.extensionContext = new ExtensionContext();
this.profileStatusContext = ProfileStatusContext.createWithNone();
Copy link
Member

Choose a reason for hiding this comment

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

You just need to call new constructor.

}

public IgnoredTracerContext(LinkedList<AbstractSpan> activeSpanStack) {
this.activeSpanStack = activeSpanStack;
Copy link
Member

Choose a reason for hiding this comment

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

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

this.correlationContext = new CorrelationContext();
this.extensionContext = new ExtensionContext();
this.profileStatusContext = ProfileStatusContext.createWithNone();
Expand Down Expand Up @@ -84,34 +91,35 @@ public int getSpanId() {

@Override
public AbstractSpan createEntrySpan(String operationName) {
stackDepth++;
activeSpanStack.addLast(NOOP_SPAN);
Copy link
Member

Choose a reason for hiding this comment

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

All logic here should not be changed.

Copy link
Member

Choose a reason for hiding this comment

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

We just need depth, no further.

return NOOP_SPAN;
}

@Override
public AbstractSpan createLocalSpan(String operationName) {
stackDepth++;
activeSpanStack.addLast(NOOP_SPAN);
return NOOP_SPAN;
}

@Override
public AbstractSpan createExitSpan(String operationName, String remotePeer) {
stackDepth++;
activeSpanStack.addLast(NOOP_SPAN);
return NOOP_SPAN;
}

@Override
public AbstractSpan activeSpan() {
return NOOP_SPAN;
return activeSpanStack.getLast();
}

@Override
public boolean stopSpan(AbstractSpan span) {
stackDepth--;
if (stackDepth == 0) {
activeSpanStack.removeLast();
if (activeSpanStack.isEmpty()) {
ListenerManager.notifyFinish(this);
}
return stackDepth == 0;

return activeSpanStack.isEmpty();
}

@Override
Expand All @@ -134,6 +142,11 @@ public String getPrimaryEndpointName() {
return null;
}

@Override
public AbstractTracerContext forceIgnoring() {
return this;
}

public static class ListenerManager {
private static List<IgnoreTracerContextListener> LISTENERS = new LinkedList<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,12 @@ public String getPrimaryEndpointName() {
return primaryEndpoint.getName();
}

@Override
public AbstractTracerContext forceIgnoring() {

return new IgnoredTracerContext(activeSpanStack);
}

/**
* Re-check current trace need profiling, encase third part plugin change the operation name.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package org.apache.skywalking.apm.agent.core.context;

import org.apache.skywalking.apm.agent.core.boot.ServiceManager;
import org.apache.skywalking.apm.agent.core.conf.Config;
import org.apache.skywalking.apm.agent.core.context.ids.NewDistributedTraceId;
import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
import org.apache.skywalking.apm.agent.core.context.trace.NoopSpan;
import org.apache.skywalking.apm.agent.core.profile.ProfileStatusContext;
import org.apache.skywalking.apm.agent.core.test.tools.AgentServiceRule;
import org.apache.skywalking.apm.agent.core.test.tools.SegmentStorage;
import org.apache.skywalking.apm.agent.core.test.tools.SegmentStoragePoint;
import org.apache.skywalking.apm.agent.core.test.tools.TracingSegmentRunner;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.Assert;

@RunWith(TracingSegmentRunner.class)
public class ContinuedContextTest {

@SegmentStoragePoint
private SegmentStorage tracingData;

@Rule
public AgentServiceRule agentServiceRule = new AgentServiceRule();

@BeforeClass
public static void beforeClass() {
Config.Agent.KEEP_TRACING = true;
}

@AfterClass
public static void afterClass() {
Config.Agent.KEEP_TRACING = false;
ServiceManager.INSTANCE.shutdown();
}

@Test
public void testContinued() {

NewDistributedTraceId distributedTraceId = new NewDistributedTraceId();
ContextSnapshot snapshot = new ContextSnapshot(
"1, 2, 3",
1,
distributedTraceId,
"/for-test-continued",
new CorrelationContext(),
new ExtensionContext(),
ProfileStatusContext.createWithNone()
);

AbstractSpan span = ContextManager.createLocalSpan("test-span");
ContextManager.continued(snapshot);

Assert.assertEquals(distributedTraceId.getId(), ContextManager.getGlobalTraceId());
ContextManager.stopSpan();
}

@Test
public void testContinuedWithIgnoredSnapshot() {

ContextSnapshot snapshot =
new ContextSnapshot(null, -1, null, null, new CorrelationContext(), new ExtensionContext(), ProfileStatusContext.createWithNone());

AbstractSpan span = ContextManager.createLocalSpan("test-span");
ContextManager.continued(snapshot);

Assert.assertEquals(ContextManager.activeSpan(), span);

AbstractSpan span2 = ContextManager.createLocalSpan("test-span2");
Assert.assertTrue(span2 instanceof NoopSpan);

ContextManager.stopSpan();
ContextManager.stopSpan();
}

}