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

Http4s Middlewares for Client and Server #75

Closed
wants to merge 13 commits into from

Conversation

tbrown1979
Copy link
Contributor

Addresses #5

Okay, so there's been a lot of talk on #5 about what we should do for an http4s module. I've had these changes locally for a long time so I've decided to PR it. They're similar to what I've done for an internal library at my company. I realize this PR isn't really inline with what has been suggested on #5, so feel free to request changes and I will happily make them. I just wanted to get something up that we can iterate on and ultimately get merged.

@tpolecat
Copy link
Member

I need to reconcile this with something I wrote myself, so hang tight. Thanks.

def spannedClient[F[_]](client: Client[F], clientName: String)(implicit F: Sync[F], S: Span[F]): Client[F] = {
Client[F](request =>
for {
s <- S.span(s"http4s-request-$clientName")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression Span isn't meant to be used like that (it's a data type and not a type class) and that Trace[F].span / Trace[F].add should be used to create new spans. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I suppose the difference between Trace and Span is just not very clear to me. I can change this to use Trace instead if that's desired!

@tbrown1979
Copy link
Contributor Author

I have some much better changes to make here I think, so I'll update this sometime in the next few days.

@DamienOReilly
Copy link

Any new updates here?

@tbrown1979
Copy link
Contributor Author

@DamienOReilly Sorry, haven't been able to swing back around. I'll see if I can get this updated soon!

@DamienOReilly
Copy link

@tbrown1979 no worries, thanks for the update.

@timbess
Copy link
Contributor

timbess commented Jun 30, 2020

@tbrown1979 Is there any way someone else could assist with the completion of this PR? I'm making some http4s services soon and really want to use this library for tracing. I'm willing to contribute and am really trying to avoid forking this and merging it myself as a temporary measure 😅

@tbrown1979
Copy link
Contributor Author

I won't be finishing this PR. If someone else would like to pick this up feel free!

@tbrown1979 tbrown1979 closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants