Improve Tracing #177
Improve Tracing #177
Conversation
| Stream.unfoldEval(initialConsulIndex) { maybeIndex => | ||
| // since this is a background task, it doesn't make sense to attach it to the trace that initially started it | ||
| import natchez.Trace.Implicits.noop |
There was a problem hiding this comment.
The noop Trace[F] previously passed to lookup prevented the span creation immediately within lookup (e.g. the span named com.dwolla.consul.ConsulServiceDiscoveryAlg.lookup) but the issue is observable when using a Client[F] that has been wrapped in the Natchez middleware from natchez-http4s. Assuming it's using a Local[F, Span[F]]-based MTL Trace[F], the span referenced by the Natchez middleware is not the noop span, but the current value of the Local instance, meaning the middleware creates a new span and sends that trace's kernel as part of the request being made.
| .map(Span.Options.Defaults.withLink) | ||
| .flatMap { | ||
| entryPoint | ||
| .root("com.dwolla.consul.ConsulServiceDiscoveryAlg.continuallyUpdating", _) |
There was a problem hiding this comment.
We technically don't need to create a new root span here; we could use the Local[F, Span[F]] to set the no-op span for the fa scope. But we have to break bincompat to demand the Local[F, Span[F]], so we might as well also ask for the EntryPoint[F] and create a new root span.
There was a problem hiding this comment.
And AFAICT we really do need Local[F, Span[F]]. One alternative I tried, using Trace[F].span("continuallyUpdating", Span.Options.Suppress), did successfully suppress the span creation from the Natchez http4s middleware, but the duration of the continuallyUpdating was still tied to the execution of the background requests, so doesn't accomplish the main goal.
The currently implementation attaches the background long polling requests to the trace of the first invocation of
authoritiesForServicefor a given service. This keeps that trace alive much longer than it should.This PR fixes that by requiring an
EntryPoint[F]andLocal[F, Span[F]]on construction of theConsulServiceDiscoveryAlg[F], so that when a new background process is started, it can work in its own trace. The downside of this approach is that it requires breaking binary compatibility, but I think that's an acceptable tradeoff.