Make LoggingFilter Generic #143

Closed
mosesn opened this Issue Mar 13, 2013 · 3 comments

Comments

2 participants
@mosesn
Contributor

mosesn commented Mar 13, 2013

motivation

The code for LoggingFilter is already extremely generic, but it only works for Http, basically because it was written for finagle-http. My proposal is to finagle it to work for any protocol.

I was implementing a generic LoggingFilter, and realized that it was basically a copy of the http version, which seems dumb. I think it's a pretty useful thing, and I'd like to be able to use the raw finagle version.

proposed implementation

Move the LoggingFilter and LogFormatter code over to finagle-core making them generic, and leave a LoggingFilter and LogFormatter in finagle-http, but marked as deprecated. Bump the minor version. In the next major version, strip the http versions.

We should also add a logError method to the LoggingFilter class, so that we can log exceptions differently from completions.

The proposed method signature for logError would be

def logError(responseTime: Duration, request: Req, error: Throwable)

who

I'd be happy to do it, and will assign it to myself after we've reached a consensus on what we want to do.

misc

I'm interested in hearing suggestions, or reasons why we don't want to do this.

@stevegury

This comment has been minimized.

Show comment Hide comment
@stevegury

stevegury Mar 15, 2013

Contributor

Sorry for the delay again.

We would be happy to see your pull request on this.
No need to bump the minor, if you leave the old LogFormatter in com.twitter.finagle.http.filter, there shouldn't be any API breakage.

I agree with the rest of your proposition, maybe should you use logException instead of logError (I'm fine with both).

Contributor

stevegury commented Mar 15, 2013

Sorry for the delay again.

We would be happy to see your pull request on this.
No need to bump the minor, if you leave the old LogFormatter in com.twitter.finagle.http.filter, there shouldn't be any API breakage.

I agree with the rest of your proposition, maybe should you use logException instead of logError (I'm fine with both).

@mosesn

This comment has been minimized.

Show comment Hide comment
@mosesn

mosesn Mar 17, 2013

Contributor

Alright, I made a pull request, the api changed slightly, I described the change in my PR. Changed to logException.

Contributor

mosesn commented Mar 17, 2013

Alright, I made a pull request, the api changed slightly, I described the change in my PR. Changed to logException.

@stevegury

This comment has been minimized.

Show comment Hide comment
@stevegury

stevegury Apr 18, 2013

Contributor

Closing, the fix is in our internal repo, it should show up here soon.

Contributor

stevegury commented Apr 18, 2013

Closing, the fix 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