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

Fix request logs with request parameter in layout pattern #1828

Merged
merged 2 commits into from Nov 18, 2016

Conversation

Projects
None yet
4 participants
@arteam
Member

arteam commented Nov 16, 2016

Add a safe version of RequestParameterConverter which works with async appenders.
It loads request parameters from a cached map rather than trying to load request data
from the original request, which may be closed during the time of a call. The map is
cached by the prepareForDefferedProcessing method, which invokes before invoking
appenders, so in theory the call to the map should be safe.

As a result, Dropwizard users can use %reqParameter{something} in their request
logging patterns.

References #1827, #1672, #1686.

Fix request logs with request parameter in layout pattern
Add a safe version of `RequestParameterConverter` which works with async appenders.
It loads request parameters from a cached map rather than trying to load request data
from the original request, which may be closed during the time of a call.  The map is
cached by the  `prepareForDefferedProcessing` method, which invokes before invoking
appenders, so in theory the call to the map should be safe.

As a result, Dropwizard users can use `%reqParameter{something}` in their request
logging patterns.

References #1827, #1672, #1686.
@coveralls

This comment has been minimized.

coveralls commented Nov 16, 2016

Coverage Status

Coverage increased (+0.02%) to 81.931% when pulling 37eff11 on fix_reqAttribute_request_logs into 12b497e on release/1.0.x.

@jplock jplock added the bug label Nov 16, 2016

@jplock jplock added this to the 1.0.5 milestone Nov 16, 2016

@Wstunes

This comment has been minimized.

Wstunes commented Nov 18, 2016

@coveralls @arteam @jplock
I've tested the new fix_reqAttribute_request_logs branch.
No logs are missing now. But another critical problem appears.
I just used the same log format in the base.yml like this


requestLog:
    appenders:
      - type: file
        currentLogFilename: ./logs/runtimeService-requests.log
        archive: true
        logFormat: "%h|%reqParameter{something}|%reqParameter{somethingelse}

But in the request log file it writes
%PARSER_ERROR[something]|%PARSER_ERROR[somethingelse]
The logback request parameter parser is not working fine.

I've done a deep test in your branch and found out the problem.
If the log format configuration in the base.yml contains a patternLayout like reqParameter and reqAttribute, the corresponding "RequestAttributeConverter.class" and "RequestParameterConverter.class" will invoke their start() method to register when the application is started.
But now you've overrided the original RequestParameterConverter to SafeRequestParameterConverter in the defaultConverterMap. I find when the application is on, the start() method in "RequestAttributeConverter.class" is invoked but nothing is invoked in the new "SafeRequestParameterConverter.class". This results in the PASERERROR.
And Could you leave your contact information with me because it's a urgent issue for me . My email is wangshuogr@qq.com. Thanks

Make `SafeRequestParameterConverter` public
Otherwise Logback doesn't pick it up as a converter.
@coveralls

This comment has been minimized.

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.02%) to 81.931% when pulling 2e9a884 on fix_reqAttribute_request_logs into 12b497e on release/1.0.x.

@arteam

This comment has been minimized.

Member

arteam commented Nov 18, 2016

@Wstunes reviewed and confirmed the pull request works in #1827. I am going to go ahead and merge it. If anyone has concerns with it, please says so.

@arteam arteam merged commit cadfe89 into release/1.0.x Nov 18, 2016

@jplock jplock deleted the fix_reqAttribute_request_logs branch Nov 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment