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

Altered Logger, RequestLogger and ResponseLogger to be more parametric using Kleisli #1916

Merged
merged 4 commits into from Jun 30, 2018

Conversation

Projects
None yet
3 participants
@aworton
Contributor

aworton commented Jun 7, 2018

Couple of concerns:

  1. Lost the handling of a none in F inside RequestLogger. Would this need to be handled in the passed in natural transformation?

  2. Existing spec only appears to assert that there is no impact on the requests, not that the logging occurs. Also doesn't exercise the !logBody paths.

Thanks in advance for review and feedback.

@rossabaker

Thanks, @aworton. Sorry for the slow review. I got busy late last week.

The None point is interesting. It's unfortunate that in the 0.18 version we have to speculate how the server will ultimately respond to a None. Logger works better on a total function. I think that raises two possibilities:

  1. Make it work only on HttpApp. This is less general than it could be, but steers toward more accurate usage.
  2. Proceed as with this PR and hope people use it tastefully.

I might lean toward the first. What do you think?

redactHeadersWhen)(logger.info(_)) *>
Sync[F].raiseError[Response[F]](e)
redactHeadersWhen)(logAction) *>
Sync[G].raiseError[Response[G]](e)

This comment has been minimized.

@rossabaker

rossabaker Jun 11, 2018

Member

When I need a method off the instance, we usually name the instance as a parameter. So instead of G[_]: Effect, I would add an implicit G: Effect[G], and then this is just G.raiseError(e).

This comment has been minimized.

@aworton

aworton Jun 11, 2018

Contributor

Will implement for consistency.

else
OptionT
.liftF(async.refOf[F, Vector[Segment[Byte, Unit]]](Vector.empty[Segment[Byte, Unit]]))
def apply[F[_] : Sync, G[_] : Effect, A](f: G ~> F, logAction: String => Unit = logger.info(_))(

This comment has been minimized.

@rossabaker

rossabaker Jun 11, 2018

Member

I suspect Effect isn't actually needed here. I found on #1913 that a Sync is sufficient. This is still on the older version of fs2, but I bet it's also true there. Might be worth a try to relax the constraint.

This comment has been minimized.

@aworton

aworton Jun 11, 2018

Contributor

👍

implicit F: Effect[F],
ec: ExecutionContext = ExecutionContext.global): HttpRoutes[F] =
def apply[F[_]: Sync, G[_]: Effect, A](f: G ~> F, logAction: String => Unit = logger.info(_))(
logHeaders: Boolean,

This comment has been minimized.

@rossabaker

rossabaker Jun 11, 2018

Member

You'll want to run test:scalafmt before the final push. CI enforces the code formatting for consistency.

This comment has been minimized.

@aworton

aworton Jun 11, 2018

Contributor

Ah, thanks. Will do.

@aworton

This comment has been minimized.

Contributor

aworton commented Jun 11, 2018

Agree that it's better to go with more accurate usage.

@aworton

This comment has been minimized.

Contributor

aworton commented Jun 20, 2018

Tried relaxing Effect to Sync, but without current success, lowered implicits to Sync, kept type parameter at Effect for consistency.
Modified Logger signatures to be restricted to HttpApp for input and output, but kept signatures in Kleisli format.
Altered tests to use HttpApp rather than HttpRoute.
Ran scalafmt.

If I understand correctly, switching from HttpRoutes to HttpApp means the ability to selectively apply logging to particular routes only is lost. Although I'm not sure if that is that likely to be a common usage, and applying the loggers once to the HttpApp simplifies use.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Jun 25, 2018

I think the build failure is intermittent. Rerunning.

If I understand correctly, switching from HttpRoutes to HttpApp means the ability to selectively apply logging to particular routes only is lost. Although I'm not sure if that is that likely to be a common usage, and applying the loggers once to the HttpApp simplifies use.

That's correct. I think getting rid of the G ~> F, which itself is to support a hack, is worthwhile here.

@rossabaker

Tried relaxing Effect to Sync, but without current success

This didn't seem right to me, but I tried it, and it's because of fs2-0.10. I think we can relax it after fs2-1.0.

I think this is ready.

@jmcardon

Only thing I want to discuss is the default global arg.

Could we smite this?

redactHeadersWhen: CaseInsensitiveString => Boolean = Headers.SensitiveHeaders.contains,
logAction: String => Unit = logger.info(_)
)(@deprecatedName('service) http: Kleisli[F, Request[F], Response[F]])(
implicit ec: ExecutionContext = ExecutionContext.global,

This comment has been minimized.

@jmcardon

jmcardon Jun 29, 2018

Member

can we remove the default arg implicit here? I don't like this.

This comment has been minimized.

@rossabaker

rossabaker Jun 29, 2018

Member

I missed that, and I agree.

Default implicit

@aworton

This comment has been minimized.

Contributor

aworton commented Jun 30, 2018

default implicit in both request and response loggers smat/smote/smiten ;)

@rossabaker

Thanks!

@rossabaker rossabaker merged commit 9588ca2 into http4s:master Jun 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment