-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
ref(profiling): implement chunked profile candidate query for flamegraph #95873
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
ref(profiling): implement chunked profile candidate query for flamegraph #95873
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #95873 +/- ##
==========================================
+ Coverage 86.49% 87.68% +1.18%
==========================================
Files 10532 10583 +51
Lines 608735 610037 +1302
Branches 23950 23950
==========================================
+ Hits 526530 534900 +8370
+ Misses 81912 74844 -7068
Partials 293 293 |
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.
I'm not fully convinced the profiling-flamegraph-always-use-direct-chunks
branch will work as intended but I'm open to giving it a try and iterate.
# If we still don't have enough continuous profile candidates + transaction profile candidates, | ||
# we'll fall back to directly using the continuous profiling data | ||
if ( | ||
len(continuous_profile_candidates) + len(transaction_profile_candidates) < max_profiles | ||
and always_use_direct_chunks | ||
): |
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.
- I don't remember how well we dedupe profile chunks so this might result in misleading flamegraphs
- I'm not fully convinced this condition is enough in higher throughput situations. Though most of the time, the problem we're trying to resolve by directly looking at chunks is in low throughput situations so it may be okay.
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.
- we don't dedupe in
vroom
so, if we were to send a duplicate in the list it would be used twice for the aggregation, but here we prevent that by keeping track ofseen_chunks
. So, if we fall in this branch, we'll still check that set and only add chunks that were not previously added and avoid adding duplicates. - agree. I'm mainly worried about:
- lower throughput situations
- still keeping the max number of chunks/profiles <= max_profiles
Anyway as you mentioned in another comment, we can give it a try to see how it works and later iterate to improve it.
This PR introduces an optimized chunked querying strategy for profile flamegraphs that improves performance when querying large time ranges. Instead of querying the entire datetime range at once, the new approach splits queries into exponentially growing chunks, starting with smaller intervals and potentially finding sufficient data without needing to query the full range.
Key Changes:
organizations:profiling-flamegraph-use-increased-chunks-query-strategy
for gradual rolloutget_profile_candidates_from_transactions_v2()
andget_profile_candidates_from_profiles_v2()
methods with chunked querying logic