-
Notifications
You must be signed in to change notification settings - Fork 606
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
[Feature] Support Tracing for GlobalFilter and GatewayFilter in Gateway #736
[Feature] Support Tracing for GlobalFilter and GatewayFilter in Gateway #736
Conversation
Please update the Spring Gateway cases, UT is not enough to verify plugins codes. There are many test scenarios here, such as this is for 3.x GW. If you don't know how to run this, please follow Plugin automatic test framework docs. You could change codes of those cases, and verify them locally with new expectation files. |
You can see, your new codes break gateway-3.x-filter-context-scenario. That is another thing you should fix about the test scenarios. |
Sure, I'll take a look at the documentation you provided first. |
@wuwen5 Could you take a look at this? |
I think the issue seems to be repetitive with this #539 , and we can support it in this way.
|
@yqw570994511 Could you verify this new way? If it works, we don't need extra filter interceptor. |
Thank you, I'll try this way |
Thank you, I'll try this way |
I tried it according to the method you gave, and it achieved the same effect as my previous code: I got the following result: But when I am in another situation, it can't meet my needs. The result of using the new solution is as follows: I think the new scheme can support more scenarios and should help many people like me who have similar needs. |
If this targets to support custom filters, I think it makes sense. |
The problem I found is that when the gateway forwards to downstream web services, the traceId changes and the entire process is not linked together |
Why ping me? |
The tid of the log printed in the spring gateway interceptor is inconsistent with the tid of the log printed in the downstream service interceptor after forwarding. Is there any way to troubleshoot? |
I am not following. How to debug Agent and plugins are common knowledge. |
Here is a pull request discussion. Please don't mix things here. |
…e_support_gateway_filter_trace_id
…race_id' into feature_support_gateway_filter_trace_id
It is good to see tests are passed. Thanks. One question about this doc, with your new plugin gets merged, does user still need to add manual codes? Which scenarios are automatically working, which are still not? |
STACK_DEEP.get().getAndIncrement(); | ||
if (isEntry()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you have some duplicate executions here and same as isExit
.
STACK_DEEP.get().getAndIncrement()
will return 0 for the first time(as Entry), and STACK_DEEP.get().decrementAndGet()
will return 0 too for the last time(as Exit).
Your codes call getThreadLocal
and getIntValue
twice. You could polish codes about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yqw570994511 Could you check this and polish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yqw570994511 Could you check this and polish?
Thank you, I will adjust my code as follows
public class GatewayFilterInterceptor implements InstanceMethodsAroundInterceptor {
private static final ThreadLocal<AtomicInteger> STACK_DEEP = ThreadLocal.withInitial(() -> new AtomicInteger(0));
@Override
public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
MethodInterceptResult result) throws Throwable {
if (isEntry()) {
ServerWebExchange exchange = (ServerWebExchange) allArguments[0];
EnhancedInstance enhancedInstance = getInstance(exchange);
AbstractSpan span = ContextManager.createLocalSpan("SpringCloudGateway/GatewayFilter");
if (enhancedInstance != null && enhancedInstance.getSkyWalkingDynamicField() != null) {
ContextManager.continued((ContextSnapshot) enhancedInstance.getSkyWalkingDynamicField());
}
span.setComponent(SPRING_CLOUD_GATEWAY);
}
}
public static EnhancedInstance getInstance(Object o) {
EnhancedInstance instance = null;
if (o instanceof DefaultServerWebExchange) {
instance = (EnhancedInstance) o;
} else if (o instanceof ServerWebExchangeDecorator) {
ServerWebExchange delegate = ((ServerWebExchangeDecorator) o).getDelegate();
return getInstance(delegate);
}
return instance;
}
@Override
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret) throws Throwable {
if (isExit()) {
if (ContextManager.isActive()) {
ContextManager.stopSpan();
}
}
return ret;
}
@Override
public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, Throwable t) {
ContextManager.activeSpan().log(t);
}
private boolean isEntry() {
return STACK_DEEP.get().getAndIncrement() == 0;
}
private boolean isExit() {
boolean isExit = STACK_DEEP.get().decrementAndGet() == 0;
if (isExit) {
STACK_DEEP.remove();
}
return isExit;
}
}
If you think it is feasible, can I submit the code accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please go ahead
I tried it according to the method you gave, and it achieved the same effect as my previous code
I got the following result But when I am in another situation, it can't meet my needs. My project is configured with logback to output skywalking's traceId to help me track problems, but using the above method, I can see that skywalking's traceId is not output. In the production environment, I really need my log to print out the traceId, and I also hope that I can achieve this goal by writing business code as much as possible. For example, I hope my filter code can output the traceId in the log like this:
The result of using the new solution is as follows: I think the new scheme can support more scenarios and should help many people like me who have similar needs
I tried it according to the method you gave, and it achieved the same effect as my previous code I got the following result But when I am in another situation, it can't meet my needs. My project is configured with logback to output skywalking's traceId to help me track problems, but using the above method, I can see that skywalking's traceId is not output. In the production environment, I really need my log to print out the traceId, and I also hope that I can achieve this goal by writing business code as much as possible. For example, I hope my filter code can output the traceId in the log like this:
The result of using the new solution is as follows: I think the new scheme can support more scenarios and should help many people like me who have similar needs
Writing code similar to this is enough:
}
|
Could you reformat your comment? It is a little chaos from reading. I tried to edit but still not clear. |
The reason I am asking about, is not doubting your PR. The key is, I want to set up a good boundary for community users. |
Because in this doc, it mentioned about fetching trace IDs, I had known they are not able to output in the same place of the logs. We don't need to talk about that part. The question is, are all filters logs carrying logs automatically now? Or some fliters can, others can't. If it is later, then which filters still can't have the automatic injected trace contexts in their logs. |
Writing code similar to this is enough:
|
Is there any filter not supposed? |
Any filter that implements the GlobalFilter and GatewayFilter interfaces can be supported. If the user only calls the code chain.filter(exchange) in each filter, the traceId will be fully recorded. However, if the user writes this code chain.filter(exchange).then(), the code in then() will not be able to obtain the traceId. In this case, you need to manually call WebFluxSkyWalkingTraceContext.traceId(exchange) in then() to obtain it. Here is an example:
|
The GlobalFilter and GatewayFilter loaded by default by the Spring Gateway will also be intercepted. Do you think this is an unnecessary filter? |
I didn't have a concept which interceptor is necessary or unnecessary. Could you polish the codes by review? I think we are almost good. |
Thank you, I will adjust my code as follows
|
I posted this discussion according to your comments, apache/skywalking#12860 |
Maybe you need to add spring-cloud-gateway plugin to your projcet. @huicunjun |
…g-cloud-gateway-plugin #12839
CHANGES
log.