feat(webapp): Issue when comparison / diff timelines are out of range #1615

pavelpashkovsky · 2022-10-14T07:18:21Z

Brief

add possibility to sync timeline selection Issue when comparison / diff timelines are out of range #1614

Changes

UI which allows user to sync timeline selection when it's out of range
DEMO https://pr-1615.pyroscope.io/

codecov · 2022-10-14T07:22:03Z

Codecov Report

Base: 66.10% // Head: 66.52% // Increases project coverage by +0.43% 🎉

Coverage data is based on head (d2dac57) compared to base (e04a9a5).
Patch coverage: 86.37% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1615      +/-   ##
==========================================
+ Coverage   66.10%   66.52%   +0.43%     
==========================================
  Files         146      152       +6     
  Lines        4987     5113     +126     
  Branches     1144     1179      +35     
==========================================
+ Hits         3296     3401     +105     
- Misses       1685     1706      +21     
  Partials        6        6

Impacted Files	Coverage Δ
...pp/javascript/components/TimelineChart/markings.ts	`95.46% <ø> (ø)`
webapp/javascript/ui/Button.module.scss	`61.54% <ø> (ø)`
webapp/javascript/ui/Button.tsx	`78.38% <0.00%> (+1.24%)`	⬆️
webapp/javascript/ui/StatusMessage/index.tsx	`65.00% <55.56%> (ø)`
...ascript/ui/StatusMessage/StatusMessage.module.scss	`61.54% <61.54%> (ø)`
...ipt/components/TimelineChart/centerTimelineData.ts	`87.50% <87.50%> (ø)`
.../components/TimelineChart/SyncTimelines/useSync.ts	`95.84% <95.84%> (ø)`
...t/components/TimelineChart/SyncTimelines/index.tsx	`100.00% <100.00%> (ø)`
...nts/TimelineChart/SyncTimelines/styles.module.scss	`61.54% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2022-10-14T07:22:44Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
webapp/public/assets/app.js	483.06 KB (+0.16% 🔺)	9.7 s (+0.16% 🔺)	3.1 s (-27.86% 🔽)	12.8 s
webapp/public/assets/app.css	18.19 KB (+1.04% 🔺)	364 ms (+1.04% 🔺)	0 ms (+100% 🔺)	364 ms
webapp/public/assets/styles.css	9.52 KB (0%)	191 ms (0%)	0 ms (+100% 🔺)	191 ms
packages/pyroscope-flamegraph/dist/index.js	128.37 KB (+0.03% 🔺)	2.6 s (+0.03% 🔺)	1.6 s (+12.14% 🔺)	4.2 s
packages/pyroscope-flamegraph/dist/index.node.js	128.79 KB (+0.03% 🔺)	2.6 s (+0.03% 🔺)	721 ms (-11.43% 🔽)	3.3 s
packages/pyroscope-flamegraph/dist/index.css	7.55 KB (+0.68% 🔺)	151 ms (+0.68% 🔺)	0 ms (+100% 🔺)	151 ms

eh-am · 2022-10-14T12:43:47Z

I think we can solve this at a different level.

Given https://demo-dev.pyroscope.io/comparison?query=rideshare-app-golang.alloc_objects%7B%7D&rightQuery=rideshare-app-golang.alloc_objects%7B%7D&leftQuery=rideshare-app-golang.alloc_objects%7B%7D&from=1665749249&until=1665749701

if the interval leftFrom and leftUntil is outside the [from, until] range, display a notification, maybe on top of the timeline? We have an errorMessage ocmponent somewhere, I think it could be something like that, which is similar to bootstrap's alert: https://getbootstrap.com/docs/4.0/components/alerts/

pavelpashkovsky · 2022-10-14T13:12:18Z

@eh-am I think your variant is really good, interesting what @Rperry2174 thinks about it. and what is possible appearance and position of this alert?

pavelpashkovsky · 2022-10-17T12:24:17Z

/create-server

pavelpashkovsky · 2022-10-17T14:17:54Z

I can add tests after your review

Rperry2174 · 2022-10-18T18:49:44Z

Lets do the following:

Move the notification to below the main timeline
When clicking the button it should extend the main timeline to include the entire blue range and also the entire pink range

The logic behind 2 is that this issue most frequently occurs when the main timeline is dynamic, but the comparison sub-timeline's are fixed and so we should adjust the main timeline to fit those

pavelpashkovsky · 2022-10-20T11:56:41Z

/create-server

pavelpashkovsky · 2022-10-20T14:34:16Z

/create-server

pavelpashkovsky · 2022-10-20T14:51:15Z

hey guys, demo shows previous commit version. caching issue?

Rperry2174 · 2022-10-21T03:21:58Z

Looking at the commit I think its the correct version... what makes you think its the wrong version?

Rperry2174 · 2022-10-21T03:39:06Z

Looks great a couple of tweaks here:

Let's add an "ignore" button which makes the alert disappear

For the colors lets do (and let's make sure these are variables as well):

background: #f2cd51
text: --ps-immutable-placeholder-text
border: --ps-ui-border

For the text let's change it to be a little more specific:

"Warning: Baseline timeline selection is out of range"
"Warning: Comparison timeline selection is out of range"
"Warning: Baseline and comparison timeline selections are out of range"

pavelpashkovsky · 2022-10-21T12:15:09Z

/create-server

Rperry2174 · 2022-10-21T22:21:44Z

There's currently an issue where when you select a time range after the default time ranges load, you get the error message because the pink timerange is almost immediately "out of range".

In order to prevent this, let's change the logic so that there is a "buffer" of 5 minutes on the "minumum" side. For example:

Should not show a warning message:
Baseline (pink) timerange: 00:56 to 1:30
Comparison (blue) timerange: 1:30 to 2:00
Main timerange: 1:00 to 2:00

baseline min (00:56) to main min (1:00) is 4 minutes which is < 5 minute buffer so do not show a warning message

Should show a warning message:
Baseline (pink) timerange: 00:54 to 1:30
Comparison (blue) timerange: 1:30 to 2:00
Main timerange: 1:00 to 2:00

baseline min (00:54) to main min (1:30) is 6 minutes whic is > 5 minute buffer so do show a warning message

Let's for now put this "buffer" in a variable and we may adjust this buffer later, but it should fix the problem for now.

pavelpashkovsky · 2022-10-24T10:22:45Z

/create-server

eh-am

Looks good! I have few thoughts though:

RE: reusability

There's already a StatusMessage component, which I think can be reused here.

https://github.com/pyroscope-io/pyroscope/blob/main/webapp/javascript/ui/StatusMessage/index.tsx

Instead of creating custom buttons, we should reuse the existing button we have, and if necessary extend it in such a way that's reusable.

RE: organization

Right now it works for comparison/diff since both use left/right, but not necessarily. Let's decouple input/output, plus it becomes testable.

The API would be something like

<SyncTimelines onSync={onSync} main={mainTimeline} left={leftTimeline} right={rightTimeline} />

Since code will be pretty similar between comparison/diff, then you can add another interaction layer to reuse the same logic if that helps. But biggest point is that the syncTimelines functionality should NOT be aware of the store/anything else. It just takes inputs and calls outputs.

Tests

Given my previous point, it will be easy to test this component. And it makes it as good documentation.

webapp/javascript/components/TimelineChart/SyncTimelines/useSync.ts

pavelpashkovsky · 2022-10-25T09:51:15Z

@eh-am

RE: organization

I updated API in this way, now this component doesn't have any connection to redux store

RE: reusability

I use StatusMessage , but initial UI design of this component differs from what we need for this warning. StatusMessage API allows styling it in way we need, but I did nothing with error and success appearance, what looks a bit weird from my perspective. Can you please take a look at this component and way of using it, maybe you have some better solution

pavelpashkovsky · 2022-10-25T10:33:57Z

/create-server

webapp/javascript/components/TimelineChart/SyncTimelines/index.tsx

webapp/javascript/ui/StatusMessage/index.tsx

pavelpashkovsky · 2022-10-26T11:10:07Z

pavelpashkovsky · 2022-10-26T12:16:37Z

/create-server

Rperry2174 · 2022-10-27T03:00:11Z

/create-server

webapp/javascript/components/TimelineChart/SyncTimelines/SyncTimelines.spec.tsx

eh-am · 2022-10-27T10:49:20Z

webapp/javascript/components/TimelineChart/SyncTimelines/useSync.ts

+  const [
+    {
+      xaxis: { from: leftMarkingsFrom, to: leftMarkingsTo },
+    },
+  ] = markingsFromSelection('single', {
+    ...leftSelection,
+  } as Selection);
+  const [
+    {
+      xaxis: { from: rightMarkingsFrom, to: rightMarkingsTo },
+    },
+  ] = markingsFromSelection('single', {
+    ...rightSelection,
+  } as Selection);


My understanding is that you are using this to convert an arbitrary from/to to a timestamp right? If so you can use https://github.com/pyroscope-io/pyroscope/blob/569091ed1a27cbe86611420b4fdf3bcdf3b330d6/webapp/javascript/util/formatDate.ts#L70-L89
not the best name i know

I am saying this bc markingsFromSelection is all about markings, those vertical lines in the timeline:
.
Point is that that markingsFromSelection function is purely visual. Since you don't need that aspect, you can use formatAsOBject directly.

webapp/javascript/components/TimelineChart/SyncTimelines/useSync.ts

petethepig

Haven't looked at the code but the demo looks great!

pavelpashkovsky · 2022-10-31T10:11:55Z

/create-server

feat: sync timeline

795a064

pavelpashkovsky force-pushed the track-selection-out-of-range branch from 0f6c08d to 795a064 Compare October 17, 2022 11:59

Merge branch 'main' into track-selection-out-of-range

86fc99d

Ivanikitin added the frontend Mostly JS code label Oct 18, 2022

feat: sync timeline v2

e4e60b1

pavelpashkovsky force-pushed the track-selection-out-of-range branch from 94adaa2 to e4e60b1 Compare October 20, 2022 11:49

refactor: selection logic

414d686

pavelpashkovsky added 2 commits October 21, 2022 14:37

refactor: logic to show specific title, colors, ignoring

28e7257

fix: tslint issue

50773ff

chore: adjust detecting logic

ee6b1c2

pavelpashkovsky changed the title ~~feat(webapp): Issue when comparison / diff timelines are out of range [WIP]~~ feat(webapp): Issue when comparison / diff timelines are out of range Oct 24, 2022

pavelpashkovsky marked this pull request as ready for review October 24, 2022 14:18

eh-am reviewed Oct 24, 2022

View reviewed changes

webapp/javascript/components/TimelineChart/SyncTimelines/useSync.ts Outdated Show resolved Hide resolved

refactor: SyncTimeline component API

41df876

refactor: use StatusMessage, Button

e922cb1

eh-am reviewed Oct 25, 2022

View reviewed changes

webapp/javascript/components/TimelineChart/SyncTimelines/index.tsx Outdated Show resolved Hide resolved

eh-am reviewed Oct 25, 2022

View reviewed changes

webapp/javascript/ui/StatusMessage/index.tsx Outdated Show resolved Hide resolved

refactor: StatusMessage usage

0c9c8dd

chore: tests

93bdb11

pavelpashkovsky added 3 commits October 26, 2022 16:37

chore: extend Button "kind"

9c39d10

chore: testing functionality

591063f

fix: css format check

cae9d1e

pavelpashkovsky commented Oct 27, 2022

View reviewed changes

webapp/javascript/components/TimelineChart/SyncTimelines/SyncTimelines.spec.tsx Outdated Show resolved Hide resolved

eh-am reviewed Oct 27, 2022

View reviewed changes

Rperry2174 mentioned this pull request Oct 27, 2022

feat(webapp): Add relative time period dropdown to comparison / diff view #1638

Merged

github-actions bot added the has conflicts label Oct 27, 2022

pavelpashkovsky added 5 commits October 28, 2022 15:20

Merge branch 'main' into track-selection-out-of-range

c88e02b

refactor: deleted data-testid as unused

1f675a5

refactor: rename vars, split logic

ff8544b

refactor: upd test querying logic

3695546

refactor: @extend for 'outline' btn

03f70d4

pavelpashkovsky removed the has conflicts label Oct 28, 2022

petethepig self-requested a review October 31, 2022 05:00

petethepig approved these changes Oct 31, 2022

View reviewed changes

refactor: extracting unix time, test

d2dac57

Rperry2174 merged commit 211ccca into main Nov 1, 2022

Rperry2174 deleted the track-selection-out-of-range branch November 1, 2022 15:12

grafakus mentioned this pull request Oct 30, 2023

fix(SyncBanner): Add 10% buffer on both sides of the timeline #2608

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(webapp): Issue when comparison / diff timelines are out of range #1615

feat(webapp): Issue when comparison / diff timelines are out of range #1615

pavelpashkovsky commented Oct 14, 2022 •

edited

Loading

codecov bot commented Oct 14, 2022 •

edited

Loading

github-actions bot commented Oct 14, 2022 •

edited

Loading

eh-am commented Oct 14, 2022

pavelpashkovsky commented Oct 14, 2022

pavelpashkovsky commented Oct 17, 2022

pavelpashkovsky commented Oct 17, 2022

Rperry2174 commented Oct 18, 2022 •

edited

Loading

pavelpashkovsky commented Oct 20, 2022

pavelpashkovsky commented Oct 20, 2022

pavelpashkovsky commented Oct 20, 2022

Rperry2174 commented Oct 21, 2022

Rperry2174 commented Oct 21, 2022 •

edited

Loading

pavelpashkovsky commented Oct 21, 2022

Rperry2174 commented Oct 21, 2022 •

edited

Loading

pavelpashkovsky commented Oct 24, 2022

eh-am left a comment

pavelpashkovsky commented Oct 25, 2022

RE: organization

RE: reusability

pavelpashkovsky commented Oct 25, 2022

pavelpashkovsky commented Oct 26, 2022

pavelpashkovsky commented Oct 26, 2022

Rperry2174 commented Oct 27, 2022

eh-am Oct 27, 2022

petethepig left a comment

pavelpashkovsky commented Oct 31, 2022

feat(webapp): Issue when comparison / diff timelines are out of range #1615

feat(webapp): Issue when comparison / diff timelines are out of range #1615

Conversation

pavelpashkovsky commented Oct 14, 2022 • edited Loading

Brief

Changes

codecov bot commented Oct 14, 2022 • edited Loading

Codecov Report

github-actions bot commented Oct 14, 2022 • edited Loading

size-limit report 📦

eh-am commented Oct 14, 2022

pavelpashkovsky commented Oct 14, 2022

pavelpashkovsky commented Oct 17, 2022

pavelpashkovsky commented Oct 17, 2022

Rperry2174 commented Oct 18, 2022 • edited Loading

pavelpashkovsky commented Oct 20, 2022

pavelpashkovsky commented Oct 20, 2022

pavelpashkovsky commented Oct 20, 2022

Rperry2174 commented Oct 21, 2022

Rperry2174 commented Oct 21, 2022 • edited Loading

pavelpashkovsky commented Oct 21, 2022

Rperry2174 commented Oct 21, 2022 • edited Loading

pavelpashkovsky commented Oct 24, 2022

eh-am left a comment

Choose a reason for hiding this comment

RE: reusability

RE: organization

Tests

pavelpashkovsky commented Oct 25, 2022

RE: organization

RE: reusability

pavelpashkovsky commented Oct 25, 2022

pavelpashkovsky commented Oct 26, 2022

pavelpashkovsky commented Oct 26, 2022

Rperry2174 commented Oct 27, 2022

eh-am Oct 27, 2022

Choose a reason for hiding this comment

petethepig left a comment

Choose a reason for hiding this comment

pavelpashkovsky commented Oct 31, 2022

pavelpashkovsky commented Oct 14, 2022 •

edited

Loading

codecov bot commented Oct 14, 2022 •

edited

Loading

github-actions bot commented Oct 14, 2022 •

edited

Loading

Rperry2174 commented Oct 18, 2022 •

edited

Loading

Rperry2174 commented Oct 21, 2022 •

edited

Loading

Rperry2174 commented Oct 21, 2022 •

edited

Loading