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

Make tracing an optional feature #713

Open
seanmonstar opened this issue Sep 11, 2023 · 4 comments · May be fixed by #776
Open

Make tracing an optional feature #713

seanmonstar opened this issue Sep 11, 2023 · 4 comments · May be fixed by #776

Comments

@seanmonstar
Copy link
Member

Make tracing available as a Cargo feature, probably default off.

@tottoto tottoto linked a pull request May 8, 2024 that will close this issue
@glorv
Copy link

glorv commented Jun 20, 2024

While test integrating tonic into TiKV to replace grpc-rs, I found that tonic's performance is not as good as grpc-rs as it consumes more cpu. And from the profile, I saw that a lot of cpu time is cost by the tracing logic in h2. In some workloads, the qps with tonic can be 15% lower than grpc-rs. Here is a cpu profiling snapshot:

image

After patching tonic with a h2 package with all tracing code removed, tonic's performance is slightly better than gprc-rs in almost all workload without any further optimization.

@ajwerner
Copy link
Contributor

ajwerner commented Aug 6, 2024

There's something about this that surprises me. Namely, should tracing really incur a 15% overhead if the tracing level is low? That seems shocking. @glorv any interest in providing repro steps or raw profiles?

@glorv
Copy link

glorv commented Aug 7, 2024

There's something about this that surprises me. Namely, should tracing really incur a 15% overhead if the tracing level is low? That seems shocking. @glorv any interest in providing repro steps or raw profiles?

@ajwerner It's an attempt to replace grpc-rs with tonic for tikv. The workload is sysbench 's oltp_write_only that is a CPU heavy workload with high write concurrency(256 in my benchmark). So any part consumes more CPU will result with a performance regression.

Here are 2 profiles with the same workload, the 2nd is patched with all tracing code removed.

profile.zip
(Because github doesn't allow to paste raw .proto file, I put the two profile samples in this zip).

@ajwerner
Copy link
Contributor

Thanks, that's helpful context, and sad. It feels to me like the subscriber should have indicated that the span is not enabled and the span never should have been created. I wonder how you're setting up your tracing subscriber. I wonder if you set it up such that all of the tracing metadata from this crate are not marked enabled when returning from https://docs.rs/tracing/latest/tracing/trait.Subscriber.html#tymethod.enabled, whether you'd see any of this overhead.

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

Successfully merging a pull request may close this issue.

3 participants