Add shutdown method to TracingInstrument

slashmo commented 4 years ago

Closes #116

pokryfka commented 4 years ago

I think it should be mentioned who owns the lifecycle of the instrument.

I am assuming InstrumentationSystem will not try to shutdown registered instrument - ?

// create and boostrap the instrument
let instrument = XRayRecorder()
defer {
    instrument.shutdown()
}

InstrumentationSystem.bootstrap(instrument)

ktoso commented 4 years ago

Yeah perhaps we need to discuss this more...

So logging and metrics systems do not offer shutdown, if a backend is async and has connections it is assumed that the user takes care of it... I guess we're in the same boat, but we have this case of flushing -- to be honest the same could apply to a logger which batches writes and a metrics backend hm... We added forceFlush so perhaps we should stay out of the business of shutting down after all?

In reality, many users are likely to use https://github.com/swift-server/swift-service-lifecycle so users could:

let instrument = XRayRecorder()

lifecycle.registerShutdown(
    label: "server",
    .sync(shutdownMyServer)
)

lifecycle.registerShutdown(
    label: "tracer",
    .sync(instrument.shutdown)
)

🤔

I suppose we're might be okey then without requiring a shutdown in the API per se, because what we really care about across all implementations is handled by the forceFlush, wdyt? So yeah, backtracking on this ticket it seems IMO :)

slashmo / gsoc-swift-tracing

Add shutdown method to TracingInstrument #121