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

Proposal: Alter RNG for TraceId and SpanId #808

Closed
hdost opened this issue Jun 7, 2022 · 7 comments · Fixed by #1106
Closed

Proposal: Alter RNG for TraceId and SpanId #808

hdost opened this issue Jun 7, 2022 · 7 comments · Fixed by #1106
Labels
A-trace Area: issues related to tracing enhancement New feature or request

Comments

@hdost
Copy link
Contributor

hdost commented Jun 7, 2022

image
Base on doing some cursory benchmarking I was seeing that in main roughtly 28% of the time spent on Span::start() is 20% in trace_id generation and 8% in span_id generation,

I originally dug into this because of #800 out of interest in how much time sampling versus non sampling took.

I can run some additional benchmarks to ensure consistency. However, I am currently thinking something like https://crates.io/crates/oorandom would be a faster default. It's #[no_std] and also #[forbid(unsafe_code)]
Finally it's a PCG so statistically it should be ok.

@djc
Copy link
Contributor

djc commented Jun 7, 2022

Different RNG does not necessarily require a different crate -- the rand crate comes with several types of RNG. The first question is if we need a cryptographically secure RNG for this, which we do seem to be using here. If we think we don't need a secure RNG, we could try the SmallRng, as explained in https://docs.rs/rand/latest/rand/rngs/index.html#our-generators.

@djc djc changed the title Proposal: Alter RNG for TraceId and Proposal: Alter RNG for TraceId and SpanId Jun 7, 2022
@hdost
Copy link
Contributor Author

hdost commented Jun 11, 2022

Different RNG does not necessarily require a different crate -- the rand crate comes with several types of RNG. The first question is if we need a cryptographically secure RNG for this, which we do seem to be using here. If we think we don't need a secure RNG, we could try the SmallRng, as explained in https://docs.rs/rand/latest/rand/rngs/index.html#our-generators.

Agreed as first change SmallRng might be alright. I'll do some initial testing.

@hdost hdost mentioned this issue Jun 17, 2022
@hdost
Copy link
Contributor Author

hdost commented Jun 17, 2022

From my initial benchmarks looks like minor but not insignificant improvement. ~9% improvement ran a few times to attempt normalizing it.

@TommyCpp TommyCpp added enhancement New feature or request A-trace Area: issues related to tracing labels Jul 16, 2022
@shaun-cox
Copy link
Contributor

@hdost, curious if you're still planning to move forward with this change?

@hdost
Copy link
Contributor Author

hdost commented Jun 9, 2023

Yes I can rebase put out a patch.

hdost added a commit to hdost/opentelemetry-rust that referenced this issue Jun 9, 2023
Represents a minimum a 11%-22% improvement in relevant benchmarks.

Fixes open-telemetry#808
@hdost
Copy link
Contributor Author

hdost commented Jun 9, 2023

@djc So I put out patch #1106 I looked into SmallRng, which did provide some speed increases, max 9% however, it's less portable than the Pcg Rng that I used in #1106 and the results are quite good as well.

hdost added a commit to hdost/opentelemetry-rust that referenced this issue Jun 9, 2023
Represents a minimum a 3-8% improvement in relevant benchmarks.

Fixes open-telemetry#808
@hdost
Copy link
Contributor Author

hdost commented Oct 18, 2023

Finally got back to this, and when running the stress test for the SmallRng we're seeing about a 6% improvement at the best, to negligible at worst.

Using Existing Rng

Number of threads: 6
Throughput: 6,093,600 iterations/sec
Throughput: 6,099,200 iterations/sec
Throughput: 6,141,400 iterations/sec
Throughput: 6,004,200 iterations/sec

Using SmallRng

Number of threads: 6
Throughput: 6,448,000 iterations/sec
Throughput: 6,407,800 iterations/sec
Throughput: 6,429,200 iterations/sec
Throughput: 6,338,800 iterations/sec

hdost added a commit to hdost/opentelemetry-rust that referenced this issue Oct 18, 2023
SmallRng provides 0-6% improvement in Traces.

Relates open-telemetry#808
hdost added a commit that referenced this issue Nov 12, 2023
…1106)

Move to SmallRng from ThreadRng

SmallRng provides 0-6% improvement in Traces.

Relates #808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trace Area: issues related to tracing enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants