-
Notifications
You must be signed in to change notification settings - Fork 432
benchmark(bigtable): update sync and async scan benchmarks #15557
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
benchmark(bigtable): update sync and async scan benchmarks #15557
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15557 +/- ##
=======================================
Coverage 93.03% 93.04%
=======================================
Files 2408 2408
Lines 220145 220149 +4
=======================================
+ Hits 204810 204833 +23
+ Misses 15335 15316 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| #ifdef PROFILE | ||
| auto profile_data_path = google::cloud::internal::GetEnv("PROFILER_PATH"); | ||
| if (profile_data_path) ProfilerStart(profile_data_path->c_str()); | ||
| auto profiler_start = std::chrono::steady_clock::now(); | ||
| #endif // PROFILE |
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.
not a blocker for the PR but maybe as a follow-up: it might be worth adding documentation to the README or onboarding docs on how to perform throughput benchmarking with this newly added logic?
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.
Added doc link and execution instructions.
| {"--enable-metrics", | ||
| "whether to enable Client Side Metrics for benchmarking", | ||
| [&options](std::string const& val) { | ||
| options.enable_metrics = ParseBoolean(val).value_or(true); | ||
| }}, |
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.
would you provide some context into why we are choosing to have client-side metrics for benchmarking behind a flag?
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.
Because some of the latency metrics are part of the inner loop of delivering assembled rows to the application, I want to be able to easily compare the performance impact of metrics on versus off.
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.
Thanks for the context!
scotthart
left a comment
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.
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @mpeddada1)
| {"--enable-metrics", | ||
| "whether to enable Client Side Metrics for benchmarking", | ||
| [&options](std::string const& val) { | ||
| options.enable_metrics = ParseBoolean(val).value_or(true); | ||
| }}, |
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.
Because some of the latency metrics are part of the inner loop of delivering assembled rows to the application, I want to be able to easily compare the performance impact of metrics on versus off.
| #ifdef PROFILE | ||
| auto profile_data_path = google::cloud::internal::GetEnv("PROFILER_PATH"); | ||
| if (profile_data_path) ProfilerStart(profile_data_path->c_str()); | ||
| auto profiler_start = std::chrono::steady_clock::now(); | ||
| #endif // PROFILE |
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.
Added doc link and execution instructions.
Adds scan_async_throughput_benchmark as an async version of scan_throughput_benchmark. Both now support profiling around ReadRows sections which require gperftools to be installed and can be enabled via:
This change is