Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

generalizes loggingfilter #145

Closed
wants to merge 2 commits into
from

Conversation

2 participants
Contributor

mosesn commented Mar 17, 2013

Fixes #143

motivation

Described in #143

implementation

Described in #143

changes in api

The http logging filter's type has changed slightly, so that .log takes a REQUEST, where REQUEST <: Request. Internally, this is never breaking, because we can prove that we always send REQUEST to log. However, someone could theoretically be abusing this log, although it seems unlikely. They would have to have subclassed the http LoggingFilter, and be making calls to it from inside of that subclass, passing it a Request, of a different type from the REQUEST that they parameterized LoggingFilter with.

second thoughts

I added the exception logging because the http LoggingFilter logs exceptions, but I'm beginning to think it might be scope creep, since it's easy to just add a MonitorFilter which logs exceptions. On the other hand, if you hate it so much, you can override LoggingFilter's logException method to be a noop. I think this is the right way to do it, because it gives users more power, which seems like the scala way.

Contributor

stevegury commented Mar 23, 2013

I pulled this internally, it'll show up on Github soon.
Thank you for doing that and sorry for the delay, the next time I'll be faster.

Contributor

mosesn commented Mar 23, 2013

No problem, happy to contribute.

Contributor

stevegury commented Apr 18, 2013

This is in our internal repo, it should show up here soon.

@stevegury stevegury closed this Apr 18, 2013

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