Closed GregMefford closed 6 years ago
This looks good to me. Let me know when you are ready and I will merge.
I think this is ready to merge when you are. I'll send the sampling thing as a separate PR when I get it figured out. The only issue with this one is that it's currently tangled with the Datadog sender API because it wasn't obvious to be how to use the test ones you made, and actually it kind of makes sense to test the sender when doing a benchmark since it's a thing that happens in production use-cases. Maybe this benchmark should only test the Spandex
code, though, and we would separately benchmark the SpandexDatadog
code? I'll need to think more about that, but I think this PR is fine for now.
I think I might want to merge the adapter split first, and then merge this. There is a test adapter that is platform agnostic that could be used for these benchmarks in that new branch.
Yep, I agree that’s a better choice.
On Fri, Aug 17, 2018 at 6:02 PM Zach Daniel notifications@github.com wrote:
I think I might want to merge the adapter split first, and then merge this. There is a test adapter that is platform agnostic that could be used for these benchmarks in that new branch.
— You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/zachdaniel/spandex/pull/59#issuecomment-413999104, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEPW14O9VDyvz_e9rHzWdHhW8rN15Z_ks5uRz1egaJpZM4WAgoi .
Alright, the other branch is merged. Not going to cut a release just yet though, want to let it mellow and test it out a bit.
Cool. I will work on rebasing this to use the test adapter.
On Fri, Aug 17, 2018 at 10:51 PM Zach Daniel notifications@github.com wrote:
Alright, the other branch is merged. Not going to cut a release just yet though, want to let it mellow and test it out a bit.
— You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub https://github.com/zachdaniel/spandex/pull/59#issuecomment-414027490, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEPWwo5u8iT5ey0iIW7YhaiO3BzCYO-ks5uR4E8gaJpZM4WAgoi .
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/zachdaniel/spandex/pulls/59.
OK, I got this rebased to remove the dependency on DataDog - it's just benchmarking performance of the code Spandex library now.
Should be ready to merge now if you're happy with it. ✅
Awesome! Merged :)
This is an initial stab at adding Benchee benchmarks for the
Spandex.Plug.*
modules to get a feel for how much performance impact there is when adding Spandex to a service. It is important to note that since theBaselineRouter
doesn't really do anything, thex slower
metrics are meaningless, so we should focus on the raw per-request timing increases due to adding tracing (in terms of how many microseconds is tracing adding to each request we process).I wanted to get this in place so that I can work on an implementation for trace sampling (hence the stubbed-in
inputs
list) and validate that it alleviates the performance overhead of tracing, at least on average, when only sampling a small fraction of requests.Feedback welcome!
Some initial results from my laptop (with a million other things running, so take it with a grain of salt):