Support across thread tracing for SOFA-RPC#675
Conversation
|
Please explain your scenario. When this happens, and what was not working. |
|
I think you should add |
|
I think it is not hard to run. The basic workflow is, the test is using a demo app with a compiled latest Java agent up and running. Then there is a mock traffic generator to trigger traces to the mock collector. The last step is to write expected spans to verify. |
|
I have commit some code to verify the callback spans through the expected file. However I have some thing not very sure:
|
| segments: | ||
| - segmentId: not null | ||
| spans: | ||
| - operationName: HEAD:/sofarpc-scenario/case/healthCheck |
There was a problem hiding this comment.
/healthCheck should not be included in the file.
| refType: CrossProcess, parentSpanId: 1, parentTraceSegmentId: not null, parentServiceInstance: not | ||
| null, parentService: sofarpc-scenario, traceId: not null} | ||
| skipAnalysis: 'false' | ||
| - segmentId: not null |
There was a problem hiding this comment.
Is expectedData.yaml file strongly depends on the stop order of each span? I am using async invoke ,so the stop order is not fixed. It may affect CI success rate.
Considering this is a new segment, which part of the span order are you referring to?
There was a problem hiding this comment.
There was a problem hiding this comment.
Segment order is not a part of validation in the file. You don't need to worry about it.
| spanType: Exit | ||
| peer: 127.0.0.1:12200 | ||
| skipAnalysis: false | ||
| tags: | ||
| - {key: url, value: 'bolt://127.0.0.1:12200/org.apache.skywalking.apm.testcase.sofarpc.interfaces.SofaRpcDemoService.onAppResponse(java.lang.String)'} |
There was a problem hiding this comment.
By reading this, the callback is calling another RPC?
There was a problem hiding this comment.
I think we could make the test a little simpler. Just end the chain from Thread/com.alipay.sofa.rpc.message.bolt.BoltInvokerCallback/onResponse. We don't have to do another RPC. So this one, and the other new entry span(new segment) is not necessary, too.
There was a problem hiding this comment.
Yes callback is calling another RPC. I will simplify it.
| @Override | ||
| public void onConstruct(EnhancedInstance objInst, Object[] allArguments) { | ||
| if (ContextManager.isActive()) { | ||
| objInst.setSkyWalkingDynamicField(ContextManager.capture()); |
There was a problem hiding this comment.
There is a risk of using this field to propagate tracing context. The input(inherit InvokeCallback) is a object, which could be reused by many callback invocations.
If SOFA RPC indicates this callback service should never be shared with different RPCs? Especially if this callback could be statusless.
An alternative solution is, you could use a wrapper, a plugin level InvokeCallback implementation to wrap the original one. We could make sure, that is one time used, and add a field(not need to be dynamic field) to hold the context.
There was a problem hiding this comment.
In the context of SOFARPC, InvokeCallback is a wrapper. Users directly use the SofaResponseCallback class. Each SOFARPC callback request corresponds uniquely to an InvokeCallback.
However, in the context of SOFA BOLT (BOLT being a wrapper for Netty which SOFARPC relies on), InvokeCallback may be reused.
It's a good idea to create a plugin level wrapper, but I am afraid I will introduce an error because of cannot dealing with classloader correctly.
I plan to create a class InvokeCallbackWrapper in plugin code. How to make it loaded by business classloader?
public class InvokeCallbackWrapper implements InvokeCallback {
private ContextSnapshot contextSnapshot;
private InvokeCallback invokeCallback;
public InvokeCallbackWrapper(InvokeCallback invokeCallback) {
this.contextSnapshot = ContextManager.capture();
this.invokeCallback = invokeCallback;
}
@Override
public void onResponse(final Object o) {
ContextManager.createLocalSpan("Thread/" + invokeCallback.getClass().getName() + "/onResponse");
if (contextSnapshot != null) {
ContextManager.continued(contextSnapshot);
}
try {
invokeCallback.onResponse(o);
} catch (Throwable t) {
ContextManager.activeSpan().log(t);
throw t;
} finally {
ContextManager.stopSpan();
contextSnapshot = null;
}
}
@Override
public void onException(final Throwable throwable) {
ContextManager.createLocalSpan("Thread/" + invokeCallback.getClass().getName() + "/onException");
if (contextSnapshot != null) {
ContextManager.continued(contextSnapshot);
}
if (throwable != null) {
AbstractSpan abstractSpan = ContextManager.activeSpan();
if (abstractSpan != null) {
abstractSpan.log(throwable);
}
}
try {
invokeCallback.onException(throwable);
} catch (Throwable t) {
ContextManager.activeSpan().log(t);
throw t;
} finally {
ContextManager.stopSpan();
contextSnapshot = null;
}
}
@Override
public Executor getExecutor() {
return invokeCallback.getExecutor();
}
}There was a problem hiding this comment.
I plan to create a class InvokeCallbackWrapper in plugin code. How to make it loaded by business classloader?
All plugin interceptor relative codes are loaded in your target class loader, otherwise, all existing logic will fail. I think in this case, users of SOFA would not be aware of the wrapper, as it is only running internally.
There was a problem hiding this comment.
Classloader doesn't need to be worried, AFAIK.
...rpc-plugin/src/main/java/org/apache/skywalking/apm/plugin/sofarpc/InvokeCallbackWrapper.java
Outdated
Show resolved
Hide resolved
...rpc-plugin/src/main/java/org/apache/skywalking/apm/plugin/sofarpc/InvokeCallbackWrapper.java
Outdated
Show resolved
Hide resolved
...rpc-plugin/src/main/java/org/apache/skywalking/apm/plugin/sofarpc/InvokeCallbackWrapper.java
Outdated
Show resolved
Hide resolved
| protected ContextSnapshot getContextSnapshot() { | ||
| return contextSnapshot; | ||
| } | ||
|
|
||
| protected InvokeCallback getInvokeCallback() { | ||
| return invokeCallback; | ||
| } |
There was a problem hiding this comment.
These could be replaced by annotations.
|
Generally, this PR is good, just some nits. Please fix them, then it is good to merge. |
...rpc-plugin/src/main/java/org/apache/skywalking/apm/plugin/sofarpc/InvokeCallbackWrapper.java
Outdated
Show resolved
Hide resolved
...rpc-plugin/src/main/java/org/apache/skywalking/apm/plugin/sofarpc/InvokeCallbackWrapper.java
Outdated
Show resolved
Hide resolved
|
I tried to apply the change, but it seems not work. |
Missed |

Now SOFA-PRC plugin only support in thread tracing. It does not support across thread tracing.
InvokeCallbackclass is the key class which does the async callback work. This pr add the ability of across thread tracing by enhancingInvokeCallbackclass the way likeorg.apache.skywalking.apm.toolkit.activation.trace.CallableOrRunnableActivationdo.Tests(including UT, IT, E2E) are added to verify the new feature.
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the
CHANGESlog.