Fix double render issue (Issue #257)

[gabrielzezze]

Fix on issue #257.

The duplicate request was caused by fetching the timeline inside FlameGraphRenderer component and It's parent.
The issue was fixed by removing the FlameGraphRenderer fetching and pulling the data from the redux store, this change required the addition of extra keys in the redux store to scope the flamebearer data as discussed in the thread below to keep the the comparison view and comparison diff functionality.

Any suggestions or tips are welcome.
Thanks.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #492 (2600fb8) into main (0cbf399) will decrease coverage by 0.03%.
The diff coverage is 16.48%.

❗ Current head 2600fb8 differs from pull request most recent head dbf639b. Consider uploading reports for the commit dbf639b to get more accurate results

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
- Coverage   57.06%   57.03%   -0.02%     
==========================================
  Files         178      176       -2     
  Lines        7414     7407       -7     
  Branches      211      226      +15     
==========================================
- Hits         4230     4224       -6     
+ Misses       2842     2835       -7     
- Partials      342      348       +6     

Impacted Files	Coverage Δ
webapp/javascript/util/flamebearer.ts	40.55% <7.15%> (-21.95%)	⬇️
webapp/javascript/redux/actions.js	16.90% <10.77%> (-5.83%)	⬇️
webapp/javascript/redux/actionTypes.js	100.00% <100.00%> (ø)
pkg/storage/tree/tree.go	82.66% <0.00%> (-1.30%)	⬇️
pkg/server/controller.go	56.98% <0.00%> (-0.97%)	⬇️
pkg/agent/gospy/gospy.go	22.39% <0.00%> (ø)
pkg/storage/tree/collapsed.go
pkg/storage/tree/pprof.go
pkg/storage/tree/profile_extra.go
pkg/convert/profile_extra.go	77.20% <0.00%> (ø)
... and 2 more

---

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 0cbf399...dbf639b. Read the comment docs.

[eh-am]

Hey, thank you for your contribution!

I think what you can do is to scope the flamebearer to the view in the store, something like this:

single: {
  flamebearer: {}
},
comparison: {
  left: {
   flamebearer: {}
  },
  right: {
   flamebearer: {}
  }
},
diff: {
  flamebearer: {}
}

which we would then pass as a prop here -> https://github.com/pyroscope-io/pyroscope/blob/main/webapp/javascript/components/ComparisonApp.jsx#L36

That wouldn't be super optimized (for example, comparison.left is probably the same as single.flamebearer) but honestly, it should be fine for now!

[gabrielzezze]

@eh-am Hi,

Thanks a lot for the help, as single.flamebearer would be the same as comparison.left.flamebearer was thinking about just adding the comparison.flamebearer key but your idea makes the code more structured and in general makes more sense so I'll follow with it, thanks again.

Sorry for any inconvenience.

[gabrielzezze]

Hi,

Sorry for the delay, did what was previously discussed on this thread and It's working, any changes required or improvements opportunities just let me know.

Thanks.

[eh-am]

Hey, nice work! Don't be sorry, I'm enjoying these PRs!

So I've added a couple comments code wise, I think you may be interested in them.

That being said, I think we gonna need one more iteration on this code.
I am not a biiiiiiiig fan of adding flags and doing different things based on these flags, here being the viewType and viewSide.

You know what we can do that will simplify this a bit? (And by simplify I mean make it more obvious to the reader.)

a) Dispatch the data from the parent component, pretty much as you have been doing, for example, the comparison page (we would be doing this for all kinds, single, comparison, diff ):

(ComparisonApp.jsx)

      actions.fetchTimeline(diffRenderURL, 'comparison');

But then, instead of calling this generic action fetchTimeline, let's call fetchComparisonApp (a brand new action) instead.

      actions.loadComparison(diffRenderURL, 'comparison');

b) That gives us much more assurance on what we are acting on, for example here in the comparison diff we can REQUIRE the presence of the viewSide, since we know we need it to be "both" | "left" | "right".

      actions.loadComparison(diffRenderURL, 'diff', 'both');

c) Now that we know what we are dealing with, we don't have to run deltaDiffWrapper, except for diff. Also adding leftTicks and rightTicks, that is also something that's only relevant for diff.

In our example (comparison), in the reducer we have something like:

case RECEIVE_COMPARISON:
      const { viewSide } = action.payload;
      const { flamebearer, timeline } = data;

     let left, right;
     switch (viewSide) {
        // we are only loading left
        // right comes from the existing state
        case 'left': {
           left = action.payload.left;
           right = state.comparison.right;
           break;
        }

        // we are only loading right
        // left comes from the existing state
        case 'right': {
           right = action.payload.right;
           left = state.comparison.left;

           break;
        }

        // we are loading both
        case 'right': {
           right = action.payload.right;
           left = action.payload.left;

           break;
        }
     }

     return {
        ...state,
        timeline: decodeTimelineData(timeline),
        comparison: {
          left,
          right,
        }
     }

d) Then the flamegraphRenderer is dumb, as in it doesn't communicate with the state.
It only receives a flamegraph from a prop and that's it.

ComparisonApp.jsx

(props.comparison comes from the state)

          <FlameGraphRenderer
            flamebearer={props.comparison.left}
            viewType="double"
            viewSide="left"
            data-testid="flamegraph-renderer-left"
          />

          <FlameGraphRenderer
            flamebearer={props.comparison.right}
            viewType="double"
            viewSide="right"
            data-testid="flamegraph-renderer-right"
          />

Let me know if this too much to do at once, we can definitely do this in smaller, discrete steps!

[gabrielzezze]

Thank you @eh-am!

Don't worry, It does seem a lot to do but I think I can handle it. I'll start by adding the new actions and making the FlameGraphRenderer dumb and then will work on the minor code comments.

If there is anything else I could improve just let me know.

[eh-am]

I just realized I mixed diff and comparison above, diff requires the diffDeltaWrapper and the leftTicks and rightTicks, none of these are relevant to comparison. But I think you got the idea!

[gabrielzezze]

Hey @eh-am,

If I'm not mistaken I have covered all the changes we discussed, could you please have a look at it ?

Please let me know if there's anything, not the way you expected.
Thanks.

[eh-am]

Hey @gabrielzezze!

It seems the flamegraph is a bit broken now:

The cypress tests are also failing (which I assume it's because the flamegraph is a bit broken):
https://github.com/pyroscope-io/pyroscope/actions/runs/1441676316

I had a quick look at the code and it seems it's breaking because deltaDiffWrapper is indeed required for all cases (My bad for misleading you there!)

So what we can do is to run deltaDiffWrapper after the fetch, for example:

      .then((response) => response.json())
      .then((data) => {
        const calculatedLevels = deltaDiffWrapper(
          data.flamebearer.format,
          data.flamebearer.levels
        );

        data.flamebearer.levels = calculatedLevels;
        return data;
      })

We gonna repeat the same thing 3 times, but it's fine since I am going to refactor all these fetches into a dataService some time in the future.

[eh-am]

The other thing with the comparison page is that the logic is a bit different:

• If leftRenderURL changes, you fetch the left flamegraph
• If rightRenderURL changes, you fetch the right flamegraph

Using hooks, would be something like this

  useEffect(() => {
    actions.fetchComparisonAppData(renderURL, 'left');
  }, [renderURL]);

• these values come from the store

const mapStateToProps = (state) => ({
  ...state,
  renderURL: buildRenderURL(state),
  leftRenderURL: buildRenderURL(state, state.leftFrom, state.leftUntil),
  rightRenderURL: buildRenderURL(state, state.rightFrom, state.rightUntil),
});

• Then the timelines (the one at the top, the one on the left and the one on the right), are all the same! Which comes from renderURL. So in that case you need to ovewrite left.timeline and right.timeline to get the same value as the timeline at the top, which I believe it's just timeline (at the root).

Let me know if you have any questions!

[eh-am]

Added a couple comments!

[gabrielzezze]

Hi @eh-am,

Sorry for the delay, the national holiday got me occupied but now that's over. So what I did was:

1. Fixed the leftRenderURL and rightRenderURL props.
2. Changed the logic on the reducer to change the timeline key only when It was called from renderURL changes.
3. Separated the timeline controllers on the fetchComparisonAppData function, this was needed as using only the currentTimelineController was causing (on the initial page load) the leftRenderURL change to abort the renderURL fetch and the rightRenderURL change was aborting the leftRenderURL fetch and that meant only the right side was being loaded.

I made some checks of my own but was unable to run the Cypress tests locally, let me know if there's anything I've missed.
Thanks

[gabrielzezze]

I just saw the Cypress tests failed again...I'll work to fix it by tomorrow.
Any help or tips are appreciated.
Thanks

[eh-am]

@gabrielzezze Thank you again!

Don't worry about that cypress test! It's flaky and I will probably remove it sometime soon.

I did a quick test here and everything looks good, except for the Diff view which is a bit messed up (see screenshot)
I will have a look more in depth and I will let you know!

[eh-am]

Hey @gabrielzezze , turns out it was a silly thing. I pushed in another branch for you:
ae2e0aa
(You still have to make these changes in this branhc)

Everything else looks pretty good!
Sorry for the trouble, I really appreciate the effort you took on this PR 🦾

[gabrielzezze]

Hey @eh-am, thanks for the help!

As you pointed out in your branch I was calling deltaDiffWrapper two times on the ComparisonDiffApp, once in the fetch action and another in the reducer...now that's fixed and that should be it if I'm not mistaken.

I really enjoyed the process of making this PR, thanks for all the support!