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

copy server logging middleware to client module #1820

Merged
merged 1 commit into from May 2, 2018

Conversation

Projects
None yet
3 participants
@bpholt
Contributor

bpholt commented May 1, 2018

The copied files are mostly unchanged, except for updating the package name, running scalafmt, and changing the interface of Logger.apply to work better with Client[F].

(Hopefully this is set up against the right base branch to be merged and released as part of the 0.18 series.)

logBody: Boolean,
redactHeadersWhen: CaseInsensitiveString => Boolean = Headers.SensitiveHeaders.contains
)(client: Client[F]): Client[F] =
Client.fromHttpService(

This comment has been minimized.

@bpholt

bpholt May 1, 2018

Contributor

I'm not sure whether this is the best approach here. It seems to work ok for low-volume clients but the to/from HttpService round-trip definitely adds some overhead. I'm also not sure how to implement this on master, which has deprecated HttpService[F]. I'd love suggestions on how to remove the HttpService[F] interface from these client classes and just deal with Client[F].

This comment has been minimized.

@rossabaker

rossabaker May 2, 2018

Member

It seems highly likely that a client will be based on either Http[Stream[F, ?], F] or Http[Resource[F, ?], F] in the future. And then the logging middleware can be built on Http[G, F] and be unified for servers and clients.

@rossabaker

It'd be nice if we could share more code between the two implementations, but we have MiMa constraints to get this into release-0.18.x. I imagine a lot of the guts could be pulled out into core and then referred to in server and client. @ChristopherDavenport, does that seem worthwhile? You're the expert in this area.

logBody: Boolean,
redactHeadersWhen: CaseInsensitiveString => Boolean = Headers.SensitiveHeaders.contains
)(client: Client[F]): Client[F] =
Client.fromHttpService(

This comment has been minimized.

@rossabaker

rossabaker May 2, 2018

Member

It seems highly likely that a client will be based on either Http[Stream[F, ?], F] or Http[Resource[F, ?], F] in the future. And then the logging middleware can be built on Http[G, F] and be unified for servers and clients.

@ChristopherDavenport

If this is compliant for 0.18 I am all for it.

I think that message typeclass along with the Http changes make make a lot of sharing possible when this is advanced into the new branch.

@rossabaker

Alright, let's make it work here and let's make it prettier on master.

@rossabaker rossabaker merged commit dd481fb into http4s:release-0.18.x May 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bpholt bpholt deleted the Dwolla:client-logger branch May 25, 2018

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