Add trace visualization as a FlameGraph #976
Conversation
pavelpashkovsky
requested review from
everett980,
rubenvp8510 and
tiffon
as code owners
There was a problem hiding this comment.
Thanks for contributing! 🥳
A couple of questions on the view:
- The table on the left seems sorted by Self column - is it possible to indicate that visually? I can't tell if the sorting is by Self or Total column, can only infer it from the data
- There is a Total/Self cell in the table that shows "< 0.01 sec" - is it informative? As I understand, "total" here refers to an implied span outside of the actual spans (judging by the flame graph itself, the top purple row), so under what condition could Total/Self be non-zero?
Lastly, please make sure all commits in the PR are signed (it's a CNCF licensing requirement).
| @@ -0,0 +1,18 @@ | |||
| /* | |||
| Copyright (c) 2017 Uber Technologies, Inc. | |||
There was a problem hiding this comment.
Please update the copyright:
| Copyright (c) 2017 Uber Technologies, Inc. | |
| Copyright (c) 2022 The Jaeger Authors. |
| import './index.css'; | ||
|
|
||
| const TraceFlamegraph = ({ trace }: any) => { | ||
| const convertedProfile = convertJaegerTraceToProfile(trace.data); |
There was a problem hiding this comment.
Hehe, kinda nasty having this circular dependency on the internal data model, but ok.
Is it possible to add some basic test for this function in jaeger-ui project? The scenario I am worried about is if we decide to make changes to the trace data model (e.g. by switching to working with OTEL model natively), I would like that test to fail and flag the dependency issue. If convertJaegerTraceToProfile function was implemented right here it would be less of a problem (I assume you have internal unit tests for it).
yurishkuro
changed the title
Codecov Report
@@ Coverage Diff @@
## main #976 +/- ##
==========================================
- Coverage 95.32% 95.29% -0.04%
==========================================
Files 242 243 +1
Lines 7555 7560 +5
Branches 1890 1892 +2
==========================================
+ Hits 7202 7204 +2
- Misses 346 349 +3
Partials 7 7
Help us with your feedback. Take ten seconds to tell us how you rate us. |
| ); | ||
| }; | ||
|
|
||
| export default TraceFlamegraph; |
There was a problem hiding this comment.
maybe add some basic tests like "does not explode" and snapshot?
Signed-off-by: pavelpashkovsky <pavelpashkovsky@gmail.com>
pavelpashkovsky
force-pushed
the
viz-trace-flamegraph
branch
from
d89c744
to
e603f23
Compare
Signed-off-by: pavelpashkovsky <pavelpashkovsky@gmail.com>
Signed-off-by: pavelpashkovsky <pavelpashkovsky@gmail.com>
|
Thank you! |
|
There are some kinks with this: #1012. |
Which problem is this PR solving?
We added the ability to visualize a trace as a flamegraph and thought it might be worth contributing upstream! Would love feedback on if it resolves this issue: #390
Resolves #525
Short description of the changes
182827151-2fc1819e-ceea-40b1-b4fd-40802cb90f8b.mp4