-
Notifications
You must be signed in to change notification settings - Fork 858
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
Http trace context benchmark #815
Http trace context benchmark #815
Conversation
Codecov Report
@@ Coverage Diff @@
## master #815 +/- ##
============================================
+ Coverage 78.98% 80.71% +1.73%
- Complexity 789 820 +31
============================================
Files 105 108 +3
Lines 2845 2899 +54
Branches 273 265 -8
============================================
+ Hits 2247 2340 +93
+ Misses 494 452 -42
- Partials 104 107 +3
Continue to review full report at Codecov.
|
This looks like a good start. Can you publish the current benchmarks as a part of the PR description, along with what hardware you're running on? |
api/src/jmh/java/io/opentelemetry/trace/propagation/HttpTraceContextExtractBenchmark.java
Outdated
Show resolved
Hide resolved
This is my first approach for jmh, after we will agree on current state of tests i will run it under my home laptop |
@jkwatson @carlosalberto btw |
Yes, but let's add them as a follow-on PR. Benchmarks for parsing of trace-context headers would be extremely useful. |
Please rebase :) |
api/src/jmh/java/io/opentelemetry/trace/propagation/HttpTraceContextInjectBenchmark.java
Show resolved
Hide resolved
Windows 10 Pro
@jkwatson i think we should have some hardware to run performance tests on, also i have issue with encoding on windows dunno how to solve for now
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
First: Results are very hard to read, in opencensus I used jmhreport https://github.com/census-instrumentation/opencensus-java/blob/master/build.gradle#L17 to expose things in a nicer format. Second: I am very confused about the fact that we want to benchmark with different inputs, I think we should just create N random TraceParent (don't measure this) then extract all of them. |
Thanks for response @bogdandrutu, i will add this plugin. After i will show u result with it |
@DotSpy my point is that because we have a lot of combinations span_id/trace_id the benchmarks run only 10 time each, so the numbers are not very stable/relevant. So my suggestion was to actually have maybe just one span_id/trace_id combination and have that benchmark run more times to be able to determine the average better. |
@bogdandrutu i just pre-generated a bunch of random trace/span ids as we discuss in comments with @jkwatson and @carlosalberto, i think it will be good for now just increase iterations to 50 per pair of span-id\trace-id. |
@DotSpy I am still confused why we need to have only 50 iterations, in my previous life I used to have 100K iterations to see something useful (like percentile, etc.). I would strongly suggest to have more than 10K anyway. I think what we can do is to build a vector of trace-parents with these ideas and do something like: |
@bogdandrutu if i got u right i did it with iterations\params, should i provide now results via jmhReport plugin? |
@DotSpy nice, this looks how I was thinking :). Some new results would be very good :) |
CI will fail until #866 merged |
Please rerun CI and if all ok it can be merged |
Did you rebase? |
@bogdandrutu sry, now seems ok |
Extract and inject benchmarks for http context #792