typelevel / natchez-http4s Goto Github PK

The limitation with the current encoding is that its reliant on the spans. And needs the new root span for incomming requests. As well some tracing may be leveraged throughout the app. So instead we can construct it initially at the beginning of the world and then allow a "principled" modification to reset for contexts like requests etc.

If we get a FiberLocal this will be nice as this could be in the F of an app, or anything with LiftIO, but this gist displays it can be done with only an IOLocal.

IOLocal

https://gist.github.com/ChristopherDavenport/d871b8c12720fd30501c7c62efaf4167

FiberLocal

https://gist.github.com/ChristopherDavenport/66afc215d0111b2916f6eb820a3e344c

Description

It would be useful to be able to provide custom tracers, of the form:

• Request[F] => F[Unit]
• Response[F] => F[Unit]
• Throwable => F[Unit]

Use Case

• Trace the request+response body based on Content-Type encoding and, for example, do-not-trace headers
• Write non-sensitive HTTP headers as trace properties
• Custom traces based on Throwable properties for monitoring & alerting

API Suggestion

def server[F[_]: Trace](
  routes: HttpRoutes[F],
  withRequestTracer: Option[Request[F] => F[Unit]] = None,
  withResponseTracer: Option[Response[F] => F[Unit]] = None,
  withErrorTracer: Option[Throwable => F[Unit]] = None
)(
  implicit ev: MonadCancel[F, Throwable]
): HttpRoutes[F] =
  Kleisli { req =>
    val additionalRequestTracer = withRequestTracer.getOrElse((_: Request[F]) => ().pure[F])
    val additionalResponseTracer = withResponseTracer.getOrElse((_: Response[F]) => ().pure[F])
    val additionalErrorTracer = withErrorTracer.getOrElse((_: Throwable) => ().pure[F])
    // ...
    val addRequestFields: F[Unit] =
      Trace[F].put(
        Tags.http.method(req.method.name),
        Tags.http.url(req.uri.renderString),
      ) *> additionalRequestTracer(req)
     // etc    

Hi, thanks for the great work on this lib & others !

Steps to reproduce : fail a traced http route, for instance with IO.raiseError(new RuntimeException)

Expected : no "error.message" mapping, or default to empty string
Actual : NullPointerException is raised by Natchez Middleware, as the Circe Encoder fails to encode null value as a string (used with LogTracer).

https://github.com/tpolecat/natchez-http4s/blob/main/modules/http4s/src/main/scala/natchez/http4s/NatchezMiddleware.scala#L62
-> I guess we cannot expect every java + scala codebase with dependencies to always have a message value, and I believe this line should be null checked.

Also in core natchez, a fail-fast null check on put method could be helpful.

Description

Request cancellation doesn't populate the request fields (though case.exit is correctly set as cancelled), breaking any monitoring filters that use for example http.url exists.

Solution

Courtesy of ChristopherDavenport, guaranteeCase could be used to cover the Canceled case:

OptionT {
  routes(req).guaranteeCase{
      case Canceled() => // append request + error fields?
      case Errored(e) => OptionT.liftF(addRequestFields *> addErrorFields(e))
      case Succeeded(fa) => fa.value.flatMap{
         case Some(handler) => addRequestFields *> addResponseFields(handler)
         case None => OptionT.liftF(Applicative[F].unit))
     }
  }
}

Hello, we're using natchez for tracing, very pleased.

We're trying to bump to 0.0.22, but our HTTP4s services start failing requests due to a ClassCastException.

I did some digging, and I found the cause. Recent versions of dd-trace have started looking at the http.status_code flag, which is expected to be an Integer, not a String:

DataDog/dd-trace-java@e6a2a36#diff-9df24c921979abf616256e047e43b08aa0ad40ef7ac721632512765ec47e1b9cL103

This only recently started breaking because the status code was hardcoded to 0 before; they weren't examining the tag.

The documentation for natchez-https perhaps hints that the String type is surprising, if I'm interpreting "String (!)" correctly.

Is there a reason it's a String, or would you be open to moving it to an Int? I'm happy to contribute with a PR if that's something you'd like.