Consider making tracing optional

google / tarpc

An RPC framework for Rust with a focus on ease of use.

MIT License

3.3k stars 201 forks source link

Consider making tracing optional #402

Open 3XX0 opened 1 year ago

3XX0 commented 1 year ago

While tracing is nice, it adds several dependencies and some overhead to the RPCs. Ideally this should be a configurable feature so that projects can choose whether to include it or not.

tikue commented 1 year ago

Thanks, you make a good point! I'd happily accept a PR; otherwise, I'll see if I can get to this in the next few weeks.

3XX0 commented 1 year ago

I've looked into it but Span are embedded in a lot of places. Not exactly sure how to go about it since I'm not really that familiar with the code base.

tikue commented 1 year ago

Hm, yeah that's true—thinking about this again, I think the tracing crate is already decoupled from specific tracers. I expect the core tracing crate, for use by other libraries, is intended to be lightweight enough to not need to be optional.

Is there significant RPC overhead of tracing when no tracing subscriber is initialized? I would consider this a bug to be addressed by https://github.com/tokio-rs/tracing; perhaps file a performance issue?
Same question regarding compilation overhead. I suspect tracing overhead would be dwarfed by the likes of tokio?

3XX0 commented 1 year ago

Yeah, so regarding the extra dependencies and compilation overhead, this is going to require significant efforts. One workaround could be writing a dummy module that implements Span and does nothing, but still...

Regarding the runtime and RPC overhead, it looks like tokio tracing won't do anything when there is no subscriber. However, there is a trace_context regardless, so maybe it's just a matter of making this optional when no subscriber is registered?

tikue commented 9 months ago

Maybe the issue is just that passing around Contexts is too expensive (it's copying like 25 bytes or something)? Do you have any performance profiles of this overhead being a problem? It could potentially be refactored to be heap-allocated and then passed around by pointer, to save like 17 bytes...

3XX0 commented 9 months ago

It's not a big overhead but I wish there was a way to send things context-free especially for things like inter-process communication where it's not really warranted.

I would say it is more problematic with regards to all the dependencies and compilation time it brings. In particular, it makes this project de facto licensed under Apache-2.0 because of opentelemetry

tikue commented 9 months ago

One way to send things context-free would be to use a transport that converts the context to a traceless type before sending a message, and then just creates a default trace context on the other end? Or is the pain more about having to create the context for each RPC, like https://github.com/google/tarpc/issues/310?

3XX0 commented 9 months ago

Yes that could work, but this doesn't solve the fact that all opentelemetry would still be built-in, does it?