Fix duplicate traceId when use forward scheme in spring.cloud.gateway.routes.uri properties #672
Conversation
| if (isAlreadyRouted(exchange) || !isHttpScheme(exchange)) { | ||
| // In these cases, NettyRoutingFilter will not send request and HttpClientFinalizer#send will not be | ||
| // invoked. | ||
| // If HttpClientFinalizer#send don't invoke, "SpringCloudGateway/RoutingFilter" span created below will | ||
| // not close and produce a duplicate traceId bug. | ||
| // Return before create span to avoid this bug. | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
If there is no span created, will the trace be broken? In the forward case, how the tracing context is going to propagate? I don't see the new workflow in your provided MVP. The bug description is clear about why tracing context was leaked.
There was a problem hiding this comment.
These conditions are as same as NettyRoutingFilter.
If the request is a http/https request, tracing context is going to propagate like NettyRoutingFilter#filter -> HttpClientFinalizer#send -> HttpClientFinalizer#responseConnection .
If the request is not a http/https request, tracing context is going to propagate like NettyRoutingFilter#filter -> ForwardRoutingFilter#filter -> DispatcherHandler#handle, in this case, HttpClientFinalizer will not participate in the propagation.
By the way, in the Spring Cloud Gateway releases before 2021/05/24, conditions there are use equals instead of equalsIgnoreCase. Tracing context will still leaked when use a older version Spring Cloud Gateway and the scheme of request is HTTP/HTTPS ,
There was a problem hiding this comment.
By the way, in the Spring Cloud Gateway releases before 2021/05/24, conditions there are use equals instead of equalsIgnoreCase.
2021 seems not long ago, could you try to fix that case too?
Could we use witness class to flag those versions and use a different interceptor, in order to match the target codes?
There was a problem hiding this comment.
No witness class around that Spring Cloud Gateway release,
How about we add some code in NettyRoutingFilterInterceptor#afterMethod instead of the check at NettyRoutingFilterInterceptor#beforeMethod, the code in NettyRoutingFilterInterceptor#afterMethod may like this:
@Override
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret) throws Throwable {
if (ContextManager.isActive()) {
// if HttpClientFinalizerSendInterceptor does not invoke, we will stop the span there to avoid context leak.
ContextManager.stopSpan();
}
return ret;
}That is the way apm-spring-cloud-gateway-2.0.x-plugin to stop the span.
Consider two propagates above, have a check and stop the span in NettyRoutingFilterInterceptor#afterMethod may better than have a check at NettyRoutingFilterInterceptor#beforeMethod.
There was a problem hiding this comment.
If we can't find a way(witness class or method) to separate the two cases, it is better we could close the span in the after to avoid context leak.
|
Ok, I will modify and test the code that close the span in afterMethod hook. |
…t invoke, the span created at NettyRoutingFilterInterceptor can not stop.
|
Could you review this PR? I have done a refactor on my code. |
|
impala case seems to have some issues, I am not sure the reason. |
It seems like docker pull failed in impala test case. |
|
I am trying to remove that case. This PR will be merged after I fix this. |
Fix duplicate traceId when use forward scheme in spring.cloud.gateway.routes.uri properties
I provide a Minimal, Reproducible Example to explain when and why this bug happens.
Add a unit test to verify that the fix works.
Explain briefly why the bug exists and how to fix it.
Update the
CHANGESlog.