fix: Allow access log to log paths jetty rejects#9970
fix: Allow access log to log paths jetty rejects#9970joschi merged 1 commit intodropwizard:release/5.0.xfrom
Conversation
josephlbarnett
force-pushed
the
access-requestlog-dup-headers
branch
from
8cc2b43 to
96a4740
Compare
Tentative fix for dropwizard#9773 The test change proves that logback-access's `ResponseWrapper` is not brilliant. Should probably be fixed in logback-access, but could be fixed
josephlbarnett
force-pushed
the
access-requestlog-dup-headers
branch
from
96a4740 to
dd6ef69
Compare
joschi
left a comment
joschi
left a comment
There was a problem hiding this comment.
@josephlbarnett Thanks for your contribution! LGTM.
@zUniQueX Do you also want to take a look since you've been working on the Jetty 12 integration before?
|
@josephlbarnett Thanks for your contribution. I personally don't like what logback does with their wrappers by building an incomplete servlet context. But maybe that's less overhead than what we're currently doing with our own wrapping. Maybe we should stick to Jetty's own request logging as default in 5.x and deprecate the logback logging? This would have the advantage of decoupling from the servlet stage in the request logging too. @joschi What do you think about that? I've just taken a small look at this change and have to dive into this further. But I think we should also remove the workaround class |
Do you have an idea how much work that would be? Are there suitable extension points so that we can hook it up with our logging (SLF4J) directly without the abstractions in logback-access? |
Not much, as we already use Jetty's own request logging in the dropwizard "classic" request logging 😉 |
|
I'm currently evaluating what the change of entirely removing logback-access would imply for JSON logging etc. Probably, we'll need to rethink how JSON logging for HTTP requests should be done, as we then don't have the logback |
|
@joschi I've provided a POC of removing |
Maven and java packages changed to ee10 versions of things. Websocket API changes invovling callbacks/etc, also the upgrade process changes to deal with jetty Request vs/ servlet requests. Similar mock/logging changes for those request APIs. Adopt dropwizard/dropwizard#9970 to avoid errors in request logs. Downstreams might be ok, but API changes ae major enough that they might not be so bump version and label as breaking change.
Maven and java packages changed to ee10 versions of things. Websocket API changes invovling callbacks/etc, also the upgrade process changes to deal with jetty Request vs/ servlet requests. Similar mock/logging changes for those request APIs. Adopt dropwizard/dropwizard#9970 to avoid errors in request logs. Downstreams might be ok, but API changes ae major enough that they might not be so bump version and label as breaking change.
4 participants
Tentative fix for #9773
Extends #9788 to maintain duplicate header handling while we wait for qos-ch/logback-access#23