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

Make logback access logging work with Jetty 12 #719

Closed
mvestola opened this issue Oct 22, 2023 · 5 comments · Fixed by #760
Closed

Make logback access logging work with Jetty 12 #719

mvestola opened this issue Oct 22, 2023 · 5 comments · Fixed by #760
Assignees
Labels

Comments

@mvestola
Copy link

mvestola commented Oct 22, 2023

Jetty 12 has been released July 20, 2023 and it is the latest stable release of Jetty: https://projects.eclipse.org/projects/rt.jetty/releases/12.0 . There has been quite many changes in Jetty 12 compared to earlier versions. One of the biggest change is probably "The Servlet-Api has been removed from the internals of Jetty allowing for non-servlet-based application creation."

These changes in Jetty 12 seems to make logback access logging not to work at all when using Jetty 12. At least I don't get any errors or stacktraces in logs but it just silently does not work. I am assuming that the problem might be in the log method of logback's RequestLogImpl:

public void log(Request jettyRequest, Response jettyResponse) {
        JettyServerAdapter adapter = makeJettyServerAdapter(jettyRequest, jettyResponse);
        IAccessEvent accessEvent = new AccessEvent(this, jettyRequest, jettyResponse, adapter);
        if (getFilterChainDecision(accessEvent) == FilterReply.DENY) {
            return;
        }
        aai.appendLoopOnAppenders(accessEvent);
    }

So in the log method parameters it takes org.eclipse.jetty.server.Request and passes that to the constructor of AccessEvent which assumes that the request should implement jakarta.servlet.http.HttpServletRequest. This has worked just fine with Jetty 10 and 11. But the case in Jetty 12 is that due to the big change mentioned above, org.eclipse.jetty.server.Request no longer implements jakarta.servlet.http.HttpServletRequest.

In Jetty 10 and 11, Request class is defined as public class Request implements HttpServletRequest: https://github.com/jetty/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L145

In Jetty 12, Request class is defined as public interface Request extends Attributes, Content.Source: https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L122

So my guess is that the access logging most likely silently breaks due to this problem and should somehow be able to change org.eclipse.jetty.server.Request -> jakarta.servlet.http.HttpServletRequest or make AccessEvent to accept org.eclipse.jetty.server.Request or some other better solution. Same problem is of course with the jettyResponse object.

Also package structure has changed in Jetty 12 a bit: https://webtide.com/new-jetty-12-maven-coordinates/ but not sure if this package structure change has any impact on logback since logback does not seem to reference classes in org.eclipse.jetty.eeX packages but only org.eclipse.jetty.server.*.

There is org.eclipse.jetty.ee9.nested.Request in Jetty 12 which implements HttpServletRequest but using that is probably not an option to use since it seems to be Jetty internal class and would then depend on the org.eclipse.jetty.eeX packages.

There has been some plans that Jetty 10 and 11 will probably reach end of life in couple of years, possibly in 2025, so would be good time to start preparing logback to also work with Jetty 12.

@zUniQueX
Copy link

Hi @mvestola. Supporting this without inconvenience for users won't be possible for logback.

As you've correctly mentioned, the Jetty Request doesn't implement the HttpServletRequest interface anymore. The Request objects get wrapped throughout the handler chain and, if a ServletContextHandler is registered, the handler chain will eventually contain a ServletContextRequest where the getServletApiRequest() method can be called to get an instance of HttpServletRequest. But this object won't be available in the request log stage because there the HttpChannelState.ChannelRequest.getLoggedRequest() will be provided. This defaults to the ChannelRequest itself and cannot obtain the ServletContextRequest by default.

To overcome this problem, the ServletContextRequest has to be set as the logged request. This has to be done by unwrapping the request during processing (after the ServletContextHandler has been executed) and calling the setLoggedRequest(Request) method.

It would be inconvenient, if a user would have to register a custom handler to get the request log working, but it would also be incovenient, if logback would register the handler itself.

For reference, see the modifications I've made to Dropwizard to get the RequestLogImpl working with Jetty 12:

@miknem
Copy link

miknem commented Dec 21, 2023

Hi @mvestola we ran into the same problem at the company where I work and we found no workaround for this to get it working with Jetty 12/ Spring Boot 3.2.
So what we did to get around this problem we did a separate implementation of the Logback access RequestLogImpl and corresponding classes to work with the new Jetty 12 classes.
We have published some example code here https://github.com/miknem/logback-access-jetty12 of what we did to get it working (and so far this seems to work, at least for Spring Boot 3.2).

Ideally we would have preferred to get this implemented in the logback-access framework, but like already mentioned above here in this discussion the Jetty classes are completely different and we did not manage to find any good way to create a pull request to get this merged without breaking backwards compatibility with older versions of Jetty.

@yachtintheband
Copy link

Is there a timeline yet on when we think this might go in? Or is the plan that logback access will not be compatible with Jetty long term?

@ceki
Copy link
Member

ceki commented Mar 25, 2024

@yachtintheband logback-access moved to its own repo. It already supports Jetty 12.

@ceki
Copy link
Member

ceki commented Mar 29, 2024

Logback-access version 2.0.0 has been released. It supports Jetty versions 11 and 12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants