Virtualized scrolling for trace detail view #68

tiffon · 2017-08-28T22:37:53Z

TODO

Tests
Additional documentation
Demo for team for sign-off

Description

Started with react-virtualized, but issues around flashes of un-rendered regions while scrolling and dynamically sized / periodically changing content lead to revival of old project that essentially does the same thing: src/components/TracePage/TraceTimelineViewer/ListView/*.

ListView is more optimized to our needs (and less versatile). These optimizations include:

Taking the approach of rendering less often and larger amounts of items per render
Optimizing for scrolling in both directions instead of only the current scroll direction
Having a concept of minimum and regular "overscan", which means if the currently rendered list of items meets a minimum buffer, don't render more, but if it doesn't, then render enough meet the minimum buffer and some extra, as well

Performance

Flashes of un-rendered content

Smaller traces (~500 spans or less)
- Not really an issue in prod or PR 68
Larger traces (~5k spans)
- PR 68 is on par with prod, possible slightly better
- Still fairly easy to see flashes of un-rendered content
- Issue was quite aggravated with react-virtualized (hence, a custom solution is implemented)
Massive traces (~15k spans or more)
- Flashes of un-rendered content is a significant problem for the release currently in prod
- PR 68 performance doesn't vary much between 5k and 15k+ traces

Performance on a 5,000 span trace

Sample size of 1.

/trace/8d224b61611c807f
Duration: 5.83s
Services: 11
Depth: 11
Spans: 5,013
Performance recording enabled (degrades performance)

Initial loading (in milliseconds)

Task	Prod - PR 53 (ms)	PR 68 (ms)	Ratio	Reduction
Total	6,964	1,899	0.27	73%
Loading	45	32
Scripting	3,051	1,198	0.39	61%
Rendering	3,126	430	0.14	86%
Painting	116	34
Other	626	205

Note: Ticket #61 (Use canvas instead of SVG for trace mini-map) will likely further improve initial loading.

Expanding details

Prod : 3753 ms
PR 68: 418 ms (0.11)

Text search ("gopro")

Prod : 5496 ms
PR 68: 227 ms (0.04)

Performance on a 42,000 span trace

Sample size of 1.

/trace/c440ed36
Duration: 3,393.93s
Services: 25
Depth: 11
Total Spans: 42,499
Performance recording enabled (degrades performance)

Initial loading (in milliseconds)

Task	Prod - PR 53 (ms)	PR 68 (ms)	Ratio	Reduction
Total	86,792	7,105	0.08	92%
Scripting	28,273	5,596	0.20	80%
Rendering	57,197	1,424	0.03	97%
Painting	1,321	85	0.06	96%

Expanding details

Prod : 39,500 ms
PR 68: 465 ms (0.01)

Text search ("gopro")

Prod : 43,728 ms
PR 68: 757 ms (0.02)

TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` SpanGraph - Fix #49 - Span position in graph doesn not match its position in the detail - Ticks in span graph made to match trace detail (in number and formatting) - Span graph refactored to trim down files and DOM elements TracePageHeader - `trace` prop removed - Added props for various title values instead of deriving them from `trace` Trace Detail - Several components split out into separate files - `transformTrace` to use alread span tree to determine span depth Span Detail - Fix uber/jaeger #326: extraneous scrollbars in trace views - Fix: Some tags were not being rendered due to clashing keys (observed in a log message) - Tall content scrolls via entire table instead of single table cell - Horizontal scrolling for wide content (e.g. long log values) - Full width of the header is clickable for tags, process, and logs headers (instead of header text, only) - Service and endpoint are shown on mouseover anywhere in span bar row Misc - Several TraceTimelineViewer / utils removed - `TreeNode` `.walk()` method can now be used to calculate the depth, avoiding use of less efficient `.getPath()` - Removed several `console.error` warnings caused by React key issues

TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` Span Bar / Detail - Label on span bars no longer off-screen - Clip or hide span bars when zoomed in (insted of flush left) - Add shadow to left / right boundary when span bar view is clipped - Darkened span name column to differentiate from span bar section - Span detail left column color coded by service - Clicking span detail left column collapses detail - Clicking anywhere left of parent span name toggles children visibility

Sub-page scrolling used for trace detail (TODO: revert this). This lays the ground work for using react-virtualized, but unfortunately has major perf issues, hence the TODO: revert. yarn upgrade --latest. Notable changes: - Removed react-sticky - react-router v4 - react-vis CSS needed to be included via a sym-link Misc tweaks: - Styling adjusted on trace mini-map - Scatterplot dots are sized based on number of spans - Scatterplot dots mouseover shows trace name

Add back the styling that adds an ellipsis to truncated span name text.

https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013

https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020

https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418

https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188

https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990

https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675

…60-react-virtualized

…alized

Outstanding: - Window scroller used https://bvaughn.github.io/react-virtualized/#/components/WindowScroller - Expanding span details - Collapsing / expanding children

Switching to use a props oriented ui state management for span details. This is necessary because the virtualized scroller removes and adds span details as it scrolls. The span detail components cannot retain their own state (e.g. tags table is expanded / collapsed, etc.) because they are not long-lived.

https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924

coveralls · 2017-08-29T23:13:23Z

Changes Unknown when pulling f8dd6b8 on milestone1/issue-60-react-virtualized into ** on master**.

coveralls · 2017-08-30T04:26:35Z

Changes Unknown when pulling cd45e62 on milestone1/issue-60-react-virtualized into ** on master**.

tiffon · 2017-08-30T05:11:09Z

src/components/TracePage/TraceTimelineViewer/ListView/index.js

@@ -367,7 +375,8 @@ export default class ListView extends React.Component<ListViewProps> {
    const max = nodes.length;
    for (let i = 0; i < max; i++) {
      const node: HTMLElement = (nodes[i]: any);
-      const itemKey = node.dataset.itemKey;
+      // const itemKey = node.dataset.itemKey;


TODO(joe): remove this

tiffon · 2017-08-30T05:12:53Z

src/components/TracePage/TraceTimelineViewer/ListView/index.test.js

+        windowAddListenerSpy.mockRestore();
+      });
+
+      it('adds the onScroll listener to the root HTML element after it mounts', () => {


TODO(joe): adds the listener to window

coveralls · 2017-08-30T17:19:42Z

Changes Unknown when pulling f45d360 on milestone1/issue-60-react-virtualized into ** on master**.

coveralls · 2017-08-31T09:59:56Z

Changes Unknown when pulling ff2ae49 on milestone1/issue-60-react-virtualized into ** on master**.

saminzadeh · 2017-08-31T18:32:56Z

src/components/TracePage/TraceTimelineViewer/ListView/Positions.js

+  /**
+   * Indicates how far past the explicitly required height or y-values should
+   * checked.
+   * @type {number}


Just an FYI, I think flow types will give you this for free so you don't need to write it.

Thanks. Turns out jsDoc is dead to me, long live documentation.js. It's got a ton of problems, but it looks like you get a lot for free 👍

saminzadeh · 2017-08-31T18:37:14Z

src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianKeyValues.js

 };

-function KeyValuesSummary(props: KeyValuesSummaryProps) {
-  const { data } = props;
+function getKVSummary(data: { key: string, value: any }[]) {


Any advantage in changing this? Why not just make this a React function component? I'm still a fan of being explicit with variable names aka KeyValue instead of KV. Although I know you have some preference of short names that are declared within close scope.

<KVSummary data={..} />

Well, it's a lot more performant as a regular function which is invoked, but that's kind of minor considering these are relatively rare (requiring the details to be expanded).

I've reverted the change.

saminzadeh · 2017-08-31T18:40:10Z

src/components/TracePage/TraceTimelineViewer/SpanDetail/DetailState.js

+
+import type { Log } from '../../../../types';
+
+// DetailState {


Remove comments?

saminzadeh · 2017-08-31T18:44:13Z

src/components/TracePage/TraceTimelineViewer/SpanDetailRow.js

              style={{ borderColor: color }}
            />
          </span>
        </div>
      </TimelineRow.Left>
      <TimelineRow.Right>
        <div className="p2 detail-info-wrapper" style={{ borderTopColor: color }}>
-          <SpanDetail span={span} trace={trace} />
+          <SpanDetail


I think a higher level API that could be composed to these props is.

<SpanDetail trace={$Trace} spanID={$spanID} />

which can call this component you have

That way its a similar data model

Only thing needed from the trace was the start time, so I changed the param to traceStartTime and kept the rest as-is. Most of them (*Toggle) trigger actions.

saminzadeh · 2017-08-31T18:47:27Z

src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.js

+  detailToggle: string => void,
+  findMatchesIDs: Set<string>,
+  ticks: number[],
+  trace?: XformedTrace,


should this be required?

No, if the trace needs to be loaded it is undefined until it is loaded.

saminzadeh · 2017-08-31T18:49:17Z

src/components/TracePage/TraceTimelineViewer/duck.js

+// }
+//
+
+export function newInitialState(trace = null) {


I'm a fan of this ducks approach

saminzadeh · 2017-08-31T18:50:24Z

src/components/TracePage/TraceTimelineViewer/index.js

-    this.toggleSpanCollapse = this.toggleSpanCollapse.bind(this);
-    this.toggleSpanSelect = this.toggleSpanSelect.bind(this);
+
+    this.store = createStore(reducer, newInitialState(xformedTrace));


Still a little iffy on this. I would just reuse the store and create a new namespace

Thanks. I've folded the state management into the top-level redux store.

saminzadeh · 2017-08-31T18:51:31Z

src/components/TracePage/TraceTimelineViewer/index.js

-    if (textFilter === this.props.textFilter && xformedTrace === this.props.xformedTrace) {
-      return;
+    if (xformedTrace !== this.props.xformedTrace) {
+      throw new Error('Component does not support changing the trace');


Why should this matter?

- JSDoc tweaks - documentation.js can infer types automatically - Move trace transform - Move trace tranform out of src/components/TracePage/TraceTimelineViewer/transforms and into src/model/transform-trace-data.js - Apply trace transform to data from HTTP response so the trace data stored in the state is always the enriched form

coveralls · 2017-09-05T20:19:39Z

Changes Unknown when pulling 6059fed on milestone1/issue-60-react-virtualized into ** on master**.

* Update prettier and wrap at 110 instead of 80 * WIP commit for refactoring trace detail view TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` SpanGraph - Fix #49 - Span position in graph doesn not match its position in the detail - Ticks in span graph made to match trace detail (in number and formatting) - Span graph refactored to trim down files and DOM elements TracePageHeader - `trace` prop removed - Added props for various title values instead of deriving them from `trace` Trace Detail - Several components split out into separate files - `transformTrace` to use alread span tree to determine span depth Span Detail - Fix uber/jaeger jaegertracing#326: extraneous scrollbars in trace views - Fix: Some tags were not being rendered due to clashing keys (observed in a log message) - Tall content scrolls via entire table instead of single table cell - Horizontal scrolling for wide content (e.g. long log values) - Full width of the header is clickable for tags, process, and logs headers (instead of header text, only) - Service and endpoint are shown on mouseover anywhere in span bar row Misc - Several TraceTimelineViewer / utils removed - `TreeNode` `.walk()` method can now be used to calculate the depth, avoiding use of less efficient `.getPath()` - Removed several `console.error` warnings caused by React key issues * WIP commit for refactoring trace detail view TODO - Test are currently in a bad state - Finish shifting to transformed trace, exclusively, instead of raw trace - Misc cleanup (unused / ordering of vars, imports, etc) is outstanding while changes are WIP - Continued refinement of trace-detail components, selectors and utils - Many styles will be moved to CSS - Will likely remove `model/trace-viewer.js` Span Bar / Detail - Label on span bars no longer off-screen - Clip or hide span bars when zoomed in (insted of flush left) - Add shadow to left / right boundary when span bar view is clipped - Darkened span name column to differentiate from span bar section - Span detail left column color coded by service - Clicking span detail left column collapses detail - Clicking anywhere left of parent span name toggles children visibility * WIP refactor trace detail, split out css, start fixing tests * WIP refactor trace detail, tests working again * WIP refactor trace, yarn upgrade Sub-page scrolling used for trace detail (TODO: revert this). This lays the ground work for using react-virtualized, but unfortunately has major perf issues, hence the TODO: revert. yarn upgrade --latest. Notable changes: - Removed react-sticky - react-router v4 - react-vis CSS needed to be included via a sym-link Misc tweaks: - Styling adjusted on trace mini-map - Scatterplot dots are sized based on number of spans - Scatterplot dots mouseover shows trace name * WIP refactor trace, test maintenance * remove unused import * add license to css files * Revert sub-page scrolling for trace detail * Support URL prefix via homepage in package.json * Prevent collision of logs in log entries table * Add comments to new utils * WIP Use react-virtualized in trace detail view * Fix jaegertracing#59 - "Span Name" to "Service & Operation" * Fix unreleased regression - ellipsis on span name Add back the styling that adds an ellipsis to truncated span name text. * Address PR comment on search results scatter plot https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013 * Misc cleanup from PR comment https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020 * PR comment - Remove ms and use nano seconds https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418 * PR comment - Adjust export, relates to recompose https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188 * Reorganize span detail components * PR comment - Comment getTraceSpanIdsAsTree() https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990 * PR comment - Add "SpanID:" label https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675 * WIP react-virtualized layout progress Outstanding: - Window scroller used https://bvaughn.github.io/react-virtualized/#/components/WindowScroller - Expanding span details - Collapsing / expanding children * WIP react-virtualized layout progress Switching to use a props oriented ui state management for span details. This is necessary because the virtualized scroller removes and adds span details as it scrolls. The span detail components cannot retain their own state (e.g. tags table is expanded / collapsed, etc.) because they are not long-lived. * PR comment - Flow typing for props https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924 * WIP Switching react-virtualized to custom solution * fix file-header licenses * WIP Switching react-virtualized to custom solution * Add flow to ListView, comment ListView, Positions * Positions tests, fix edge-case bug in Positions * Fix comment issue from prettier * Unit tests for ListView, remove unused propType/* * Minor changes to a comment and a test case description * Unit tests for TraceTimelineViewer/duck * remove react-virtualized from package.json * PR feedback - JSDoc, move trace transform - JSDoc tweaks - documentation.js can infer types automatically - Move trace transform - Move trace tranform out of src/components/TracePage/TraceTimelineViewer/transforms and into src/model/transform-trace-data.js - Apply trace transform to data from HTTP response so the trace data stored in the state is always the enriched form * Use top-level redux store for traceTimeline state Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

tiffon added 30 commits August 11, 2017 01:21

Update prettier and wrap at 110 instead of 80

fe35bbd

WIP refactor trace detail, split out css, start fixing tests

93a6fb7

WIP refactor trace detail, tests working again

b8945ab

WIP refactor trace, test maintenance

c34bbd3

remove unused import

c041353

add license to css files

9f54454

Revert sub-page scrolling for trace detail

c657cf1

Support URL prefix via homepage in package.json

2226744

Prevent collision of logs in log entries table

f555e8b

Add comments to new utils

bef7f5b

WIP Use react-virtualized in trace detail view

8101efd

Fix #59 - "Span Name" to "Service & Operation"

fa6f016

Fix unreleased regression - ellipsis on span name

3797f5b

Add back the styling that adds an ellipsis to truncated span name text.

Address PR comment on search results scatter plot

0fd43f5

https://github.com/uber/jaeger-ui/pull/53#discussion_r134313013

Misc cleanup from PR comment

542c1ba

https://github.com/uber/jaeger-ui/pull/53#discussion_r134314020

PR comment - Remove ms and use nano seconds

42736e0

https://github.com/uber/jaeger-ui/pull/53#discussion_r134316418

PR comment - Adjust export, relates to recompose

d16097b

https://github.com/uber/jaeger-ui/pull/53#discussion_r134318188

Reorganize span detail components

83b28d5

PR comment - Comment getTraceSpanIdsAsTree()

bdd6d7e

https://github.com/uber/jaeger-ui/pull/53#discussion_r134321990

PR comment - Add "SpanID:" label

7204684

https://github.com/uber/jaeger-ui/pull/64#issuecomment-324080675

Merge branch 'refactor-trace-detail-selectors' into span-detail-reorg

9bdf8b6

Merge branch 'refactor-trace-detail-selectors' into milestone1/issue-…

da7bbb2

…60-react-virtualized

Merge branch 'span-detail-reorg' into milestone1/issue-60-react-virtu…

762b1b1

…alized

WIP react-virtualized layout progress

0ed6fde

Outstanding: - Window scroller used https://bvaughn.github.io/react-virtualized/#/components/WindowScroller - Expanding span details - Collapsing / expanding children

Merge branch 'master' into span-detail-reorg

0113985

PR comment - Flow typing for props

5c7f896

https://github.com/uber/jaeger-ui/pull/64#discussion_r134792924

tiffon added 2 commits August 28, 2017 18:15

WIP Switching react-virtualized to custom solution

3eeec40

Merge branch 'master' into milestone1/issue-60-react-virtualized

c749135

tiffon requested a review from a team August 28, 2017 22:37

tiffon added 3 commits August 29, 2017 14:22

Add flow to ListView, comment ListView, Positions

11fa23e

Positions tests, fix edge-case bug in Positions

c88e723

Fix comment issue from prettier

f8dd6b8

Unit tests for ListView, remove unused propType/*

cd45e62

tiffon commented Aug 30, 2017

View reviewed changes

Minor changes to a comment and a test case description

f45d360

tiffon changed the title ~~[WIP] Change to virtualized scrolling for trace detail view~~ Change to virtualized scrolling for trace detail view Aug 30, 2017

tiffon changed the title ~~Change to virtualized scrolling for trace detail view~~ Virtualized scrolling for trace detail view Aug 30, 2017

tiffon assigned saminzadeh Aug 31, 2017

Unit tests for TraceTimelineViewer/duck

ff2ae49

saminzadeh reviewed Aug 31, 2017

View reviewed changes

remove react-virtualized from package.json

3c8057e

saminzadeh approved these changes Sep 5, 2017

View reviewed changes

Use top-level redux store for traceTimeline state

ff779db

tiffon merged commit 2258205 into master Sep 8, 2017

tiffon mentioned this pull request Sep 9, 2017

Use react-virtualized in trace detail view #60

Closed

yurishkuro unassigned saminzadeh Oct 23, 2017

tiffon mentioned this pull request Jan 25, 2018

Long list optimization ant-design/ant-design#3789

Closed

8 tasks

yurishkuro deleted the milestone1/issue-60-react-virtualized branch January 29, 2020 15:05

mszabo-wikia mentioned this pull request Mar 18, 2023

Remove unused Redux selectors #1284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Virtualized scrolling for trace detail view #68

Virtualized scrolling for trace detail view #68

tiffon commented Aug 28, 2017 •

edited

Loading

coveralls commented Aug 29, 2017

coveralls commented Aug 30, 2017

tiffon Aug 30, 2017

tiffon Aug 30, 2017

tiffon Aug 30, 2017

tiffon Aug 30, 2017

coveralls commented Aug 30, 2017

coveralls commented Aug 31, 2017

saminzadeh Aug 31, 2017

tiffon Sep 2, 2017

saminzadeh Aug 31, 2017

tiffon Sep 2, 2017

saminzadeh Aug 31, 2017

tiffon Sep 2, 2017

saminzadeh Aug 31, 2017

tiffon Sep 2, 2017

saminzadeh Aug 31, 2017

tiffon Sep 2, 2017

saminzadeh Aug 31, 2017

saminzadeh Aug 31, 2017

tiffon Sep 8, 2017

saminzadeh Aug 31, 2017

coveralls commented Sep 5, 2017


		import type { Log } from '../../../../types';

		// DetailState {

Virtualized scrolling for trace detail view #68

Virtualized scrolling for trace detail view #68

Conversation

tiffon commented Aug 28, 2017 • edited Loading

TODO

Description

Performance

Flashes of un-rendered content

Performance on a 5,000 span trace

Performance on a 42,000 span trace

coveralls commented Aug 29, 2017

coveralls commented Aug 30, 2017

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

coveralls commented Aug 30, 2017

coveralls commented Aug 31, 2017

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

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

coveralls commented Sep 5, 2017

tiffon commented Aug 28, 2017 •

edited

Loading