finagle icon indicating copy to clipboard operation
finagle copied to clipboard

Ability to cancel tracing for a specific request from within the handler

Open dziemba opened this issue 3 years ago • 4 comments

Is your feature request related to a problem? Please describe. There is no obvious way to disable tracing (zipkin etc.) for a single request within a finagle-http-server. My usecase is that I would like to exclude healthcheck requests (that unfortunately cannot be moved to e.g. a separate admin-server for $reasons) from sending any tracing information to zipkin.

Describe the solution you'd like It would be great if I could call a method like c.t.f.tracing.Trace.cancelSampling() from within my http Service[Request, Response] to conditionally disable/cancel all tracing for a specific request.

Describe alternatives you've considered Alternatively it could also be useful if I could pass a Request => Boolean function to the http-server that would decide if a request should be traced at all.

dziemba avatar Mar 03 '21 13:03 dziemba

Possibly related to https://github.com/twitter/finatra/issues/271.

cacoco avatar Mar 03 '21 16:03 cacoco

The TraceInitializerFilter is responsible for initializing the trace id for the request to be handled. This is placed low in the stack before any other modules that might emit traces.

Ideally this feature would be handled in an opinionated library like Finatra where there's robust per-route configuration. However, you can still do what you want by creating a module placed right above the trace initializer module in the http server stack. The module would apply a filter that unsets the sampling decision under some condition, like the request path. This isn't the best solution but would get you this behavior until thinking through how this would be integrated more naturally into finagle/finatra.

hamdiallam avatar Mar 03 '21 17:03 hamdiallam

Thanks for the pointers!

I believe I got it working now like this:

class DisableTracingForSpecificRequests[Req <: Request, Rep](shouldTraceRequest: Request => Boolean)
  extends Stack.Module0[ServiceFactory[Req, Rep]] {
  val role: Stack.Role = Stack.Role("DisableTracingForSpecificRequests")
  val description: String = "Disable tracing sampling for specific requests, based on the input function"

  def make(next: ServiceFactory[Req, Rep]): ServiceFactory[Req, Rep] = {
    val traceDisabler = Filter.mk[Req, Rep, Req, Rep] { (req, svc) =>
      if (shouldTraceRequest(req)) svc(req)
      else {
        val newId = Trace.id.copy(_sampled = Some(false))
        Trace.letId(newId) {
          svc(req)
        }
      }
    }

    traceDisabler.andThen(next)
  }
}


def shouldTrace(req: Request): Boolean = {
  req.path != "/-/health"
}

Http.server
  .withStack(_.insertAfter(TraceInitializerFilter.role, new DisableTracingForSpecificRequests[Request, Response](shouldTrace)))
  [...]

That's good enough for me for now. I think it would still be nice if it was simpler than this but I see that it's not very easy to integrate that into the existing architecture.

dziemba avatar Mar 04 '21 12:03 dziemba

That implementation looks correct. Glad it's working. I'll keep this issue open to find some time to design better integration

hamdiallam avatar Mar 04 '21 19:03 hamdiallam