[ Tracing] Add instrumentation support #1686
Conversation
|
PR missing one of the required labels: {'enhancement', 'new feature', 'dependencies', 'bug', 'internal', 'breaking-change', 'documentation'} |
|
Pipeline fails on formatting. |
| @@ -141,6 +158,9 @@ impl ZRuntime { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| let f = tracing::Instrument::instrument(f, tracing::Span::current()); | |||
There was a problem hiding this comment.
For performance reasons I'd put all these tracing calls behind a feature flag that is disabled by default.
There was a problem hiding this comment.
Makes sense.
I'll add it in.
| ZRUNTIME_POOL.get(self) | ||
| } | ||
|
|
||
| pub fn spawn<F>(&self, future: F) -> JoinHandle<F::Output> |
There was a problem hiding this comment.
What's the reason behind adding the handle and spawn functions if they are not used anywhere in the Zenoh code? zenoh-runtime is an internal Zenoh crate.
There was a problem hiding this comment.
The reason behind spawn was because a lot of futures were being spawned without an opportunity to add the instrumentation.
With the feature gate now, this just does nothing, but just delegate to the Handle.
handle itself was just because I would prefer the handle to be ignored.
I'd prefer if devs just spawned tasks via spawn instead of derefing to the handle, then using that.
That way we can forward the instrumentation.
I've removed it for now, but at some point I'd like to remove the Deref impl.
Add feature gate for instrumentation. Remove `handle` fn.
When spawning futures, we lose the original span information.
This should fix that.
I tried to capture the obvious spots, but I probably didn't get them all.