Skip to content
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

Make Server Logger Implementation Generic #2396

Merged

Conversation

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Feb 8, 2019

No description provided.

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Feb 8, 2019

Note: This buffers on RequestLogger despite never logging anything for it on anything on it for this Route. I don't have a great way to get around this at present.

Copy link
Member

@rossabaker rossabaker left a comment

Fine by me. I think it's wrong to log before it gets to the HttpApp layer, but I don't feel strongly enough to keep confusing our users over it.

)
}

def httpApp[F[_]: Concurrent](
Copy link
Member

@rossabaker rossabaker Feb 8, 2019

We haven't specialized our others that need an ~> like this. I think we should do all or none. Not strongly opinionated which.

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Feb 8, 2019

My biggest point of contention has been that there exists ping and other informational routes that we would prefer to explicitly NOT log as they are noisy.

I'm not really picky as far as our implementation of applying the ~> or not. Just seemed easier to me to have default cases, as otherwise to use a basic logger people would need to know about FunctionK. Which is a high bar to set for logging http interactions. However, that also makes our api more verbose. I think perhaps we should add more of these moving forward.

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Feb 8, 2019

Plus it gives us some low hanging fruit issues which are always nice.

Copy link
Member

@rossabaker rossabaker left a comment

This might benefit from a scalafix as well to change the apply method.

aeons
aeons approved these changes Feb 12, 2019
@ChristopherDavenport ChristopherDavenport merged commit 9f9e450 into http4s:master Feb 12, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ChristopherDavenport ChristopherDavenport deleted the genericLogger branch Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants