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

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

Open
dziemba opened this issue Mar 3, 2021 · 4 comments
Open
Labels

Comments

@dziemba
Copy link

dziemba commented Mar 3, 2021

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.

@cacoco
Copy link
Contributor

cacoco commented Mar 3, 2021

Possibly related to twitter/finatra#271.

@hamdiallam
Copy link
Contributor

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.

@dziemba
Copy link
Author

dziemba commented Mar 4, 2021

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.

@hamdiallam
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants