implement diff rendering #289

iOliverNguyen · 2021-07-16T13:13:08Z

This PR implements diff rendering:

send request with new format: single | double
add adapters for different formats
render diff graph
render color as red/green (just work for now, will need a better calculation later)
render table with 3 options: self(left), self(right) ; total(left), total(right) ; self(diff), total(diff)
split API to /render and /render-diff
add tooltip
add color legend

Depends: #288

codecov · 2021-07-16T13:15:05Z

Codecov Report

Merging #289 (73b52ab) into main (d73d9f8) will increase coverage by 0.75%.
The diff coverage is 71.69%.

@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   53.91%   54.66%   +0.75%     
==========================================
  Files         100      101       +1     
  Lines        4426     4622     +196     
==========================================
+ Hits         2386     2526     +140     
- Misses       1811     1866      +55     
- Partials      229      230       +1

Impacted Files	Coverage Δ
pkg/server/handler.go	`0.44% <0.00%> (-<0.01%)`	⬇️
pkg/server/render.go	`42.31% <22.39%> (-35.87%)`	⬇️
pkg/server/controller.go	`31.80% <50.00%> (+0.99%)`	⬆️
pkg/storage/tree/treediff.go	`98.51% <98.51%> (ø)`
pkg/storage/tree/flamebearer.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e811432...73b52ab. Read the comment docs.

kolesnikovae · 2021-07-24T11:42:36Z

pkg/server/render.go

+	var err error
+	var out, leftOut, rghtOut *storage.GetOutput
+
+	leftStartTime, leftEndTime, leftOK := parseRenderRangeParams(r.URL.Query(), "leftFrom", "leftUntil")
+	rghtStartTime, rghtEndTime, rghtOK := parseRenderRangeParams(r.URL.Query(), "rightFrom", "rightUntil")
+
+	isFormatDouble := rghtOK || leftOK
+	if isFormatDouble {


It may be way more convenient to introduce a new endpoint. Let's say, render-diff (or query-diff), that would accept POST with the following payload:

type RenderDiffParams struct { Format string MaxNodes int Left, Right RenderTreeParams } type RenderTreeParams struct { From string To string Query *string Name *string }

I generally follow the convention from the original code, where /render is GET with from=...&until=....

Because out is required, hence from and until are required. So extending the query with leftFrom, leftUntil, rightFrom, rightUntil is a reasonable choice.

(personally, I prefer using POST in API, but I followed the project convention here)

I think we define conventions ourselves :)

I believe that every endpoint/handler should solve exactly one problem, therefore I'd introduce a new one for rending diff. And POST with JSON-encoded parameters would be the most appropriate here, IMO.

I'm ok with that. Let's add @petethepig to this conversation so we can have another opinion.

@olvrng I generally agree with @kolesnikovae — I think it would make the API simpler if we split it. But I know with these kinds of things there might be other non-obvious reasons to doing things in one function, so I would like to talk to you about this before we change it.

kolesnikovae · 2021-07-24T11:55:30Z

pkg/server/render.go

+
+	isFormatDouble := rghtOK || leftOK
+	if isFormatDouble {
+		out, leftOut, rghtOut, err = ctrl.loadTreeConcurrently(p.gi, p.gi.StartTime, p.gi.EndTime, leftStartTime, leftEndTime, rghtStartTime, rghtEndTime)


Not quite sure what we need this out for.

This out is required for getting some information later, especially "timeline": out.Timeline. The timeline data is required for rendering, well, timeline on the UI.

If we don't use the tree from out, perhaps it worth extending storage with a method for timeline retrieval (without the resulting tree).

Also I'm wondering how we should handle diff/comparison with respect to queries/tags.

Maybe we should add another PR for extending storage and queries/tags.

kolesnikovae · 2021-07-24T12:04:29Z

pkg/server/render.go

+	defer func() {
+		rerr := recover()
+		if rerr != nil {
+			_err = fmt.Errorf("panic: %v", rerr)
+		}
+	}()


Consider implementing recoveryMiddleware that would wrap all handlers and catch all panics.

Actually, this method is intended to be called in loadTreeConcurrently:

var treeErr, leftErr, rghtErr error var wg sync.WaitGroup wg.Add(3) go func() { defer wg.Done(); treeOut, treeErr = ctrl.loadTree(gi, treeStartTime, treeEndTime) }() go func() { defer wg.Done(); leftOut, leftErr = ctrl.loadTree(gi, leftStartTime, leftEndTime) }() go func() { defer wg.Done(); rghtOut, rghtErr = ctrl.loadTree(gi, rghtStartTime, rghtEndTime) }() wg.Wait()

So middleware is not an option here. We must catch all panics in each goroutine or the whole process will stop.

recoveryMiddleware is not supposed to be a replacement for this recovery. There are many other places in handlers routines that may cause panic.

I'm not sure I understand your last comment. Should I keep that code or use something else?

If you are concerned with possible panics, in addition to this panic recovery (which scope is loadTree) consider implementing recovery middleware like this: https://github.com/pyroscope-io/pyroscope/blob/3bca77ab0930ef02021719b463a2009a31085450/pkg/server/controller.go#L303
that would catch all panics in all handlers (but, of course, not in goroutines created by them). Although a panic in an HTTP handler does not cause app crash (directly), recovering improves user experience, especially if server logs are collected and parsed.

Regarding panic recovery in general: it may make sense to capture stack trace, otherwise error message may be not informative.

No, I mean the code is intended to be used in goroutines, not in an HTTP handler:

func (ctrl *Controller) loadTreeConcurrently(...) { var treeErr, leftErr, rghtErr error var wg sync.WaitGroup wg.Add(3) // call here ⤵ in a new goroutine go func() { defer wg.Done(); treeOut, treeErr = ctrl.loadTree(gi, treeStartTime, treeEndTime) }() go func() { defer wg.Done(); leftOut, leftErr = ctrl.loadTree(gi, leftStartTime, leftEndTime) }() go func() { defer wg.Done(); rghtOut, rghtErr = ctrl.loadTree(gi, rghtStartTime, rghtEndTime) }() wg.Wait() // ... } func (ctrl *Controller) loadTree(...) { defer func() { rerr := recover() // <- capture here // ... }() // ... }

We should always capture panic inside a goroutine, or the process will crash. This is different from HTTP handlers.

(for HTTP handlers, the project should already have recover middleware somewhere, it's out of scope for this PR)

Okay, thanks @olvrng.

Meanwhile, please take into account:

Regarding panic recovery in general: it may make sense to capture stack trace, otherwise error message may be not informative.

Yes, I see. A stack trace log should be good enough for now. Is it ok?

Yep, debug.Stack() should tell us everything.

pkg/storage/tree/treediff.go

kolesnikovae · 2021-07-24T13:01:57Z

I have a suggestion to merge it with #290. What is the benefit of having two separate branches?

iOliverNguyen · 2021-07-25T04:24:26Z

I have a suggestion to merge it with #290. What is the benefit of having two separate branches?

I generally split server and client implementation. As described the commit tree:

94dd1a9 - (23 hours ago) add diff color (feature/diff-client)
7965619 - (24 hours ago) simplify parseFlamebearerFormat
6061286 - (24 hours ago) render diff with table
5075196 - (25 hours ago) render diff with mousemove
bc3f7a0 - (3 days ago) render diff with new levels format
fb984a0 - (4 days ago) send requests to render api with new format
bd2230c - (24 hours ago) add format to flamebearer response (feature/diff-server)
d0bedc0 - (26 hours ago) query 3 trees for diff
1819adb - (4 days ago) implement diff api by combining 2 trees
c1d1429 - (13 days ago) implement ui for diff

The client implementation (feature/diff-client) is made dependant on the server implementation (feature/diff-server). This way, each team can do review on their side. Also, when there is another fix that needs to be applied to the server, I can add it to the feature/diff-server branch, and rebase feature/diff-client on it.

94dd1a9 - (23 hours ago) add diff color (feature/diff-client)
7965619 - (24 hours ago) simplify parseFlamebearerFormat
6061286 - (24 hours ago) render diff with table
5075196 - (25 hours ago) render diff with mousemove
bc3f7a0 - (3 days ago) render diff with new levels format
fb984a0 - (4 days ago) send requests to render api with new format
a00000 - (1 hour ago) another commit to fix something on the server (feature/diff-server)
bd2230c - (24 hours ago) add format to flamebearer response
d0bedc0 - (26 hours ago) query 3 trees for diff
1819adb - (4 days ago) implement diff api by combining 2 trees
c1d1429 - (13 days ago) implement ui for diff

petethepig · 2021-07-27T22:12:34Z

@olvrng I closed the other 2 prs to reduce confusion and consolidate the discussion to just one PR. Feel free to copy the todos from the other PRs.

Rperry2174 · 2021-07-29T20:58:50Z

Thanks for the explanation comment I think this looks really good -- we talked a little bit about this in slack but just some thoughts:

~~We should use some shade of grey instead of orange to show the "base" metric and then either green or red in contrast to that to show the diff~~ (well play around with color scheme later)
We should probably remove the line that sums the two graphs and instead replace it with descriptive information i.e.: base graph: left, comparison graph: right
Here the scale should go from -100% to +100% (i believe) so that all colors are represented

iOliverNguyen · 2021-07-30T03:06:04Z

I updated the legend and tooltip based on your comment.

Rperry2174 · 2021-07-30T20:34:15Z

webapp/javascript/components/ProfilerTable.jsx

+        </td>
+        {/* NOTE: it seems React does not understand multiple backgrounds, have to workaround:  */}
+        {/*   The `style` prop expects a mapping from style properties to values, not a string. */}
+        <td STYLE={backgroundImageDiffStyle(x.selfLeft, x.selfRght, maxSelf, color, 'L')}>


Can you clarify this comment a little more? I noticed the console has this error, but are you saying this is intended?

I try to use multiple backgrounds here. The original code that I tested (and it didn't work) was:

 <td style={backgroundImageDiffStyleALT(x.selfLeft, x.selfRght, maxSelf, color, 'L')}>

function backgroundImageDiffStyleALT(a, b, total, color, side) { // ... // tried object instead of string for css return { backgroundImage: `linear-gradient(${clr}, ${clr}), linear-gradient(${cld}, ${cld})`; backgroundPosition: `${neg(-k)}px 0px, ${neg(-kd)}px 0px`; backgroundRepeat: `no-repeat`; }; }

It didn't work. And I had to change to the current code:

 <td STYLE={backgroundImageDiffStyle(x.selfLeft, x.selfRght, maxSelf, color, 'L')}>

function backgroundImageDiffStyle(a, b, total, color, side) { // ... // workaround: use string for css return ` background-image: linear-gradient(${clr}, ${clr}), linear-gradient(${cld}, ${cld}); background-position: ${neg(-k)}px 0px, ${neg(-kd)}px 0px; background-repeat: no-repeat; `; }

If you can make the original code work or find a better workaround, I'm happy to learn about it.

I fixed the first warning Warning: Each child in a list should have a unique "key" prop.. My mistake.

Rperry2174 · 2021-07-30T20:48:39Z

webapp/javascript/util/flamebearer.js

      level[i] += prev;
      prev = level[i] + level[i + 1];
    }
  }
 }
+
+export function deltaDiffWrapper(format, levels) {
+  if (format === "double") {


Can you explain why there are two deltaDiff here with different start / steps with one happening right before the other?

The corresponding code on the server is here: diff

func CombineToFlamebearerStruct(leftTree, rightTree *Tree, maxNodes int) *Flamebearer { // ... // delta encoding deltaEncoding(res.Levels, 0, 7) deltaEncoding(res.Levels, 3, 7) return &res }

With format=double, the response from the server combines values from 2 left and right trees into a single array for levels, start at i+0 and i+3 with step=7:

i+0 = x offset, left tree i+1 = total , left tree i+2 = self , left tree i+3 = x offset, right tree i+4 = total , right tree i+5 = self , right tree i+6 = index in the names array

The levels array is encoded using delta encoding for x offset on the left and the right trees before being sent to the client. Therefore, the client needs to decode it before rendering.

Rperry2174 · 2021-08-06T19:34:42Z

Looks great thanks @olvrng! After merging this I'll follow up with creating some new issues for some of the todo's / future improvements needed here. Feel free to do the same for anything you feel like you didn't get to on this PR but is worth addressing in the future

iOliverNguyen requested a review from Rperry2174 July 16, 2021 13:13

iOliverNguyen changed the title ~~WIP: feature/diff client~~ WIP: send request with new URL format Jul 16, 2021

iOliverNguyen changed the title ~~WIP: send request with new URL format~~ WIP: send request with new URL format for diff Jul 16, 2021

iOliverNguyen mentioned this pull request Jul 16, 2021

Implement mock ui for diff #288

Closed

11 tasks

iOliverNguyen force-pushed the feature/diff-client branch 4 times, most recently from c400afe to 638c239 Compare July 24, 2021 04:56

iOliverNguyen changed the title ~~WIP: send request with new URL format for diff~~ WIP: imlement client for rendering diff Jul 24, 2021

iOliverNguyen force-pushed the feature/diff-client branch 2 times, most recently from 7d13c67 to 94dd1a9 Compare July 24, 2021 06:35

iOliverNguyen changed the title ~~WIP: imlement client for rendering diff~~ WIP: implement client for rendering diff Jul 24, 2021

kolesnikovae reviewed Jul 24, 2021

View reviewed changes

iOliverNguyen requested a review from petethepig July 26, 2021 12:51

petethepig mentioned this pull request Jul 27, 2021

WIP: implement server for rendering diff #290

Closed

iOliverNguyen changed the title ~~WIP: implement client for rendering diff~~ WIP: implement diff rendering Jul 28, 2021

iOliverNguyen added 10 commits July 28, 2021 20:30

implement ui for diff

7529f0e

implement diff api by combining 2 trees

e17dac4

query 3 trees for diff

bf26f28

add format to flamebearer response

fc2253b

send requests to render api with new format

610b869

render diff with new levels format

feba101

render diff with mousemove

bc8e815

render diff with table

cac0308

simplify parseFlamebearerFormat

c4efed5

add diff color

6d4884a

iOliverNguyen force-pushed the feature/diff-client branch from d7a81d9 to 0a8f5af Compare July 29, 2021 03:42

petethepig marked this pull request as ready for review July 29, 2021 07:07

add columns to diff table

6b7337d

iOliverNguyen force-pushed the feature/diff-client branch from 0a8f5af to 6b7337d Compare July 29, 2021 08:37

add diff to tooltip

ddd1f96

iOliverNguyen force-pushed the feature/diff-client branch from 51e9d26 to 3719757 Compare July 29, 2021 10:09

add diff legend

dc39e85

iOliverNguyen force-pushed the feature/diff-client branch from 3719757 to dc39e85 Compare July 29, 2021 10:16

iOliverNguyen force-pushed the feature/diff-client branch 5 times, most recently from d508400 to 42f52b1 Compare July 30, 2021 08:07

Rperry2174 reviewed Jul 30, 2021

View reviewed changes

update legend and tooltip

73b52ab

iOliverNguyen force-pushed the feature/diff-client branch from 42f52b1 to 73b52ab Compare July 30, 2021 23:37

iOliverNguyen changed the title ~~WIP: implement diff rendering~~ implement diff rendering Aug 2, 2021

Rperry2174 merged commit e089d5b into main Aug 6, 2021

Rperry2174 deleted the feature/diff-client branch August 6, 2021 19:34

This was referenced Aug 6, 2021

differential flamegraph for comparision view #95

Closed

Separate 'comparison' endpoint from 'diff' endpoint #321

Closed

Find better color / UI for showing diff in tables #322

Closed

Reorganize components on diff page #323

Closed

Rperry2174 mentioned this pull request Sep 22, 2021

Switch from Samples to Percentages for Diff view #408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement diff rendering #289

implement diff rendering #289

iOliverNguyen commented Jul 16, 2021 •

edited

Loading

codecov bot commented Jul 16, 2021 •

edited

Loading

kolesnikovae Jul 24, 2021

iOliverNguyen Jul 25, 2021

kolesnikovae Jul 26, 2021

iOliverNguyen Jul 26, 2021

petethepig Jul 27, 2021

kolesnikovae Jul 24, 2021 •

edited

Loading

iOliverNguyen Jul 25, 2021

kolesnikovae Jul 26, 2021

iOliverNguyen Jul 26, 2021

kolesnikovae Jul 24, 2021

iOliverNguyen Jul 25, 2021

kolesnikovae Jul 26, 2021

iOliverNguyen Jul 26, 2021

kolesnikovae Jul 26, 2021

iOliverNguyen Jul 26, 2021 •

edited

Loading

kolesnikovae Jul 26, 2021

iOliverNguyen Jul 26, 2021

kolesnikovae Jul 26, 2021

kolesnikovae commented Jul 24, 2021

iOliverNguyen commented Jul 25, 2021 •

edited

Loading

petethepig commented Jul 27, 2021 •

edited

Loading

Rperry2174 commented Jul 29, 2021 •

edited

Loading

iOliverNguyen commented Jul 30, 2021

Rperry2174 Jul 30, 2021 •

edited

Loading

iOliverNguyen Jul 30, 2021 •

edited

Loading

iOliverNguyen Jul 30, 2021

Rperry2174 Jul 30, 2021

iOliverNguyen Jul 30, 2021

Rperry2174 commented Aug 6, 2021

implement diff rendering #289

implement diff rendering #289

Conversation

iOliverNguyen commented Jul 16, 2021 • edited Loading

codecov bot commented Jul 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolesnikovae Jul 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iOliverNguyen Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolesnikovae commented Jul 24, 2021

iOliverNguyen commented Jul 25, 2021 • edited Loading

petethepig commented Jul 27, 2021 • edited Loading

Rperry2174 commented Jul 29, 2021 • edited Loading

iOliverNguyen commented Jul 30, 2021

Rperry2174 Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

iOliverNguyen Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rperry2174 commented Aug 6, 2021

iOliverNguyen commented Jul 16, 2021 •

edited

Loading

codecov bot commented Jul 16, 2021 •

edited

Loading

kolesnikovae Jul 24, 2021 •

edited

Loading

iOliverNguyen Jul 26, 2021 •

edited

Loading

iOliverNguyen commented Jul 25, 2021 •

edited

Loading

petethepig commented Jul 27, 2021 •

edited

Loading

Rperry2174 commented Jul 29, 2021 •

edited

Loading

Rperry2174 Jul 30, 2021 •

edited

Loading

iOliverNguyen Jul 30, 2021 •

edited

Loading