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

[2/2] Add differential flamegraph script #2044

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

ddelnano
Copy link
Member

@ddelnano ddelnano commented Nov 4, 2024

Summary: Add differential flamegraph script

This depends on #2043.

Relevant Issues: N/A

Type of change: /kind new-pxl-script

Test Plan: Ran the local UI and verified that the non negated version works as expected
differential

Changelog Message: Add px/differential_flamegraph script for comparing flamegraphs profiles between pods. See this post for more background on how to use a differential flamegraph.

Comment on lines +19 to +22
negate = False
# TODO(ddelnano): negation requires switching the type of join from right to left
# or returning a different DataFrame. This might not be possible with pxl's current
# functionality, but this should be implemented once it's possible.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the negate parameter would be a script variable. In order to compute the negated flamegraph properly, the how on line 26 must change from right to left in addition to changing the groupby on line 35 to be against the pod1 dataframe.

I tried to implement this by calling merge_and_compute_delta with swapped parameters or to use px.select to pick the correct join type (right vs left). Neither of these seemed to work. I think we might need some additional pxl support in order to accomplish this, but please let me know if you have any ideas.

@NickLanam
Copy link
Member

If you make this PR target the other one instead of main, then it'll only show this PR's changes. GitHub will magically change the target back when the other one merges.

@ddelnano ddelnano marked this pull request as draft November 4, 2024 21:25
@ddelnano
Copy link
Member Author

ddelnano commented Nov 4, 2024

@NickLanam thanks for the tip and I'll give that a try on future PRs to avoid recreating this one.

ddelnano added a commit that referenced this pull request Nov 12, 2024
…ameGraph` display spec (#2043)

Summary: Support rendering differential flamegraphs in the
`StackTraceFlameGraph` display spec

This change makes it possible to render a [differential
flamegraphs](https://www.brendangregg.com/blog/2014-11-09/differential-flame-graphs.html).
These are useful for comparing flamegraph profiles against each other in
order to see what code paths are called more or less than the baseline
profile. When a Vis spec specifies a `differenceColumn`, the widget will
render this differential flamegraph. The difference column computes the
sample delta between the baseline profile and the new profile.

Relevant Issues: N/A

Type of change: /kind feature

Test Plan: New unit tests pass and pxl script relying on this works as
expected (see #2044)


![differential](https://github.com/user-attachments/assets/3f86ee2d-c0a1-402b-9455-b1e22b522fe1)

Changelog Message: Add support for rendering differential flamegraphs in
the `StackTraceFlameGraph` display spec
Signed-off-by: Dom Del Nano <[email protected]>
(cherry picked from commit 12881672c021e47a9090df3335c29f6048e99df3)
@ddelnano ddelnano force-pushed the ddelnano/add-diff-flamegraph-script branch from 2af5c0c to a54f660 Compare November 12, 2024 22:22
"stacktraceColumn": "stack_trace",
"countColumn": "count",
"percentageColumn": "percent",
"podColumn": "pod",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a54f660 added the podColumn column. From testing the original change (everything before a54f660), I would very infrequently receive crashes on this line.

I unfortunately don't have the stack trace handy, but the only thing I can attribute it to is that it's possible for the stacktraceColumn field to result in an undefined or empty value that causes the crash. My suspicion is this doesn't happen for the px/perf_flamegraph script because it forces the scopedStackTrace variable to contain semicolons and (k8s) (source).

@ddelnano ddelnano marked this pull request as ready for review November 12, 2024 22:53
@ddelnano ddelnano merged commit fd5cd63 into pixie-io:main Nov 13, 2024
18 of 19 checks passed
@ddelnano ddelnano deleted the ddelnano/add-diff-flamegraph-script branch November 13, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants