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

X-Request-ID Middleware #3487

Closed
wants to merge 9 commits into from
Closed

Conversation

Slakah
Copy link
Contributor

@Slakah Slakah commented Jun 10, 2020

Implements #3485 .

TODO

  • Update docs
  • Deal with UUID.randomUUID() blocking (option 2 of #3485 (comment))
  • Include the request id as attributes in both the request and response - suggested by @ChristopherDavenport

@ChristopherDavenport
Copy link
Member

ChristopherDavenport commented Jun 10, 2020

[ ] Attribute Key and append the UUID to both Request and Response via the attribute Key

Copy link
Member

@rossabaker rossabaker left a comment

I think this is on the right track. Since @voidcontext has been studying the middlewares in such depth, I'm curious what he thinks about the overloads, so we can steer toward some consistency.

def apply[G[_], F[_]](
fk: F ~> G,
headerName: CIString = requestIdHeader,
genReqId: Option[F[UUID]] = None
Copy link
Member

@rossabaker rossabaker Jun 11, 2020

Choose a reason for hiding this comment

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

I wonder about two constructors here instead of this optional parameter, because if we provide some generator, I don't think we need the Sync constraint.

Copy link
Contributor Author

@Slakah Slakah Jun 11, 2020

Choose a reason for hiding this comment

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

Okidoke, I'll mess around with the method types and see if I can get something nice working.

}
}

def httpApp[F[_]: Sync](httpApp: HttpApp[F]): HttpApp[F] =
Copy link
Member

@rossabaker rossabaker Jun 11, 2020

Choose a reason for hiding this comment

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

@voidocontext was moving toward having an httpApp and httpRoutes object, and then providing overloads off of those. Maybe that's a good idea here, particularly if we do the second constructor.

Copy link
Contributor Author

@Slakah Slakah Jun 11, 2020

Choose a reason for hiding this comment

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

Don't suppose you have an example? Tried doing some snooping on @ voidocontext, but couldn't find anything.

Made the change 611c898 , this "feels" more ergonomic, but might be difficult to maintain in future.

Really appreciate the help so far! However I might need a bit more instruction (maybe method signatures) to get this PR over the line.

Copy link
Member

@rossabaker rossabaker Jun 12, 2020

Choose a reason for hiding this comment

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

He discussed it on #2402. I don't think there are any examples yet, but I understood him to mean something like:

object httpApp {
   def apply(headerName: CIString)(httpApp: HttpApp[F]): HttpApp[F] = ???
   def apply[F[_]: Sync](
      headerName: CIString = requestIdHeader,
      genReqId: F[UUID]
  )(httpApp: HttpApp[F]): HttpApp[F] = ???
}

Since you're overloading apply, it doesn't really save much. But if you were giving everything its own name, that technique would give you a nice namespace for the parallel methods to all be called the same thing.

Copy link
Contributor Author

@Slakah Slakah Jun 12, 2020

Choose a reason for hiding this comment

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

Happy to apply the same pattern that we're going to use for all the other middleware.

Copy link
Contributor

@voidcontext voidcontext Jun 19, 2020

Choose a reason for hiding this comment

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

Apologies for the delay, I got swamped with other things, but indeed this is what I had in mind when I suggested this approach.

genReqId now effectively has a default arg
@Slakah
Copy link
Contributor Author

Slakah commented Jun 11, 2020

Hi @ChristopherDavenport , re:

Attribute Key and append the UUID to both Request and Response via the attribute Key

Are you referring to being able to override the X-Request-ID header name?

@rossabaker
Copy link
Member

rossabaker commented Jun 12, 2020

Are you referring to being able to override the X-Request-ID header name?

I think he's referring to the vault on the request. Here's an example from a client middleware:

  private val redirectUrisKey = Key.newKey[IO, List[Uri]].unsafeRunSync

  /**
    * Get the redirection URIs for a `response`.
    * Excludes the initial request URI
    */
  def getRedirectUris[F[_]](response: Response[F]): List[Uri] =
    response.attributes.lookup(redirectUrisKey).getOrElse(Nil)

Both the request and response have attributes, which is a Vault.

I'm not sure if it's that useful on the request, since it can be fetched as a header. Although another version of this middleware that passes them along as attributes instead of adding synthetic headers could be appealing.

I don't mean to speak for Chris, but I happened to be awake. I'll let him set us both right. 😆

@Slakah
Copy link
Contributor Author

Slakah commented Jun 14, 2020

Made the following changes:

  • Used the object-apply pattern (from #2402) when building the RequestId middleware
  • Added some quick docs around how to use the middleware

Not added anything to the request/response attribute, as I don't think I would use that feature, so it makes it difficult to design/implement. If we feel strongly about it though, happy to add it, but would need additional instruction as to what to use for the attribute key.

@Slakah Slakah force-pushed the request-id-middleware branch from 8c098e2 to 5a29b9f Compare Jun 15, 2020
@ChristopherDavenport
Copy link
Member

ChristopherDavenport commented Jun 15, 2020

I feel very strongly about applying it as an attribute to all requests and responses.

  1. Its immutable data so can be added at almost zero cost.
  2. Adding the attribute allows you to see into the data even if it was created via the Random
  3. It lets you have access within your HttpApp. Whereas otherwise this is only put into the response and is invisible to the inside kleisli.

@Slakah
Copy link
Contributor Author

Slakah commented Jun 15, 2020

I feel very strongly about applying it as an attribute to all requests and responses.

Okidoke, added an implementation + test. Please let me know if you folks want anything else changing.

@rossabaker rossabaker added the retarget label Jun 18, 2020
@rossabaker
Copy link
Member

rossabaker commented Jun 18, 2020

I think this is another one we can backport and release in 0.21.x.

rossabaker pushed a commit to rossabaker/http4s that referenced this issue Jun 18, 2020
@rossabaker
Copy link
Member

rossabaker commented Jun 18, 2020

Added to the 0.21.x series. Thanks!

@rossabaker rossabaker closed this Jun 18, 2020
@Slakah Slakah deleted the request-id-middleware branch Jun 20, 2020
@Slakah Slakah mentioned this pull request Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
retarget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants