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

Tracing Context not logged in access-log when replacing ReactorNettyHttpTracing with standard HttpClient/HttpServer integration #3003

Closed
sandra-markerud opened this issue Dec 19, 2023 · 6 comments
Assignees
Labels
for/stackoverflow Questions are best asked on SO or Gitter

Comments

@sandra-markerud
Copy link

Describe the Bug

Spring Boot 2 with Spring Sleuth -> Spring Boot 3 with Micrometer migration
Hello, I try to upgrade our reactive spring-cloud-gateway from Spring Boot 2 with Sleuth to Spring Boot 3 with Micrometer Tracing.
Due to the findings and fixes in #2850 and https://github.com/grassehh/spring-boot-3-tracing-coroutine I already came a long way in fixing all our problems.

However, one problem remains:
In 9 of 10 cases the Netty access-log does not contain the traceId and spanId.

Steps to Reproduce

  • Clone this project.
  • Checkout any of the spring-boot-3 branches (both kotlin and java available)
  • Either start the app and call the server as explained in the README or run the provided tests
  • The tests concerning the tracing context of the access-log are @RepeatedTests and executed 10 times. Here it occasionally happens that in one out of ten runs the access-log does contain the tracing context

Expected Result

The Netty access-log should always contain the Tracing Context

Thank you in advance for your help!

@sandra-markerud sandra-markerud added status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels Dec 19, 2023
@violetagg violetagg self-assigned this Dec 20, 2023
@violetagg violetagg added for/stackoverflow Questions are best asked on SO or Gitter for/user-attention This issue needs user attention (feedback, rework, etc...) and removed status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels Dec 20, 2023
@violetagg
Copy link
Member

@sandra-markerud For your use case you need to add the change below to your de.markerud.upgrade.configuration.TracingChannelDuplexHandler

    @Override
    public void flush(ChannelHandlerContext ctx) {
        try (Scope scope = contextSnapshotFactory.setThreadLocalsFrom(ctx.channel())) {
            ctx.flush();
        }
    }

@sandra-markerud
Copy link
Author

@violetagg Thank you so much!
I can confirm, that the access-log now shows the tracing context.

I have one more question though:
The TracingChannelDuplexHandler is always coupled with Logbook.
How could I separate this?
Say, I always want the tracing-context logged in the access-log, but I need to be able to disable logbook.

@violetagg
Copy link
Member

@sandra-markerud Can you try the code below?

class TracingChannelDuplexHandler extends ChannelDuplexHandler {
    private final ContextSnapshotFactory contextSnapshotFactory;

    public TracingChannelDuplexHandler(ContextSnapshotFactory contextSnapshotFactory) {
        this.contextSnapshotFactory = contextSnapshotFactory;
    }

    @Override
    public void channelRead(ChannelHandlerContext ctx, Object msg) {
        try (Scope scope = contextSnapshotFactory.setThreadLocalsFrom(ctx.channel())) {
            ctx.fireChannelRead(msg);
        }
    }

    @Override
    public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) {
        try (Scope scope = contextSnapshotFactory.setThreadLocalsFrom(ctx.channel())) {
            ctx.write(msg, promise);
        }
    }

    @Override
    public void flush(ChannelHandlerContext ctx) {
        try (Scope scope = contextSnapshotFactory.setThreadLocalsFrom(ctx.channel())) {
            ctx.flush();
        }
    }
}

@sandra-markerud
Copy link
Author

@violetagg this would still work for the access-log. Yes.
But now the logbook logs would not be written.
I need to instrument the TracingChannelDuplexHandler either with or without logbook, depending on if logbook is enabled or not

@violetagg
Copy link
Member

@sandra-markerud I don't understand ... you can use different ChannelDuplexHandler implementations depending on whether you want to use logbook or not.

@sandra-markerud
Copy link
Author

@violetagg You're right of course. My bad!
Thank you a lot for your quick response.

@violetagg violetagg removed the for/user-attention This issue needs user attention (feedback, rework, etc...) label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for/stackoverflow Questions are best asked on SO or Gitter
Projects
None yet
Development

No branches or pull requests

2 participants