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

Consider making tracing optional #402

Open
3XX0 opened this issue Apr 7, 2023 · 8 comments
Open

Consider making tracing optional #402

3XX0 opened this issue Apr 7, 2023 · 8 comments

Comments

@3XX0
Copy link

3XX0 commented Apr 7, 2023

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
Copy link
Collaborator

tikue commented Apr 7, 2023

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
Copy link
Author

3XX0 commented Apr 12, 2023

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
Copy link
Collaborator

tikue commented Apr 14, 2023

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
Copy link
Author

3XX0 commented Apr 21, 2023

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
Copy link
Collaborator

tikue commented Feb 2, 2024

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
Copy link
Author

3XX0 commented Feb 8, 2024

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
Copy link
Collaborator

tikue commented Feb 8, 2024

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 #310?

@3XX0
Copy link
Author

3XX0 commented Feb 9, 2024

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

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

No branches or pull requests

2 participants