From 9913ac73fb1c797170e2669fcd1c691042994a6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Bedi?= Date: Tue, 1 Dec 2020 17:43:56 +0100 Subject: [PATCH] Bug: trace viewer doesn't show more than 300 spans (#29377) * Fix: trace viewer scroll issue * Fix lazy loading * Listview test fixes * Move scrollElement to props --- .../ListView/__snapshots__/index.test.js.snap | 3 -- .../ListView/index.test.js | 8 ++--- .../TraceTimelineViewer/ListView/index.tsx | 30 ++++++++++++++----- .../VirtualizedTraceView.tsx | 9 ++++-- .../src/TraceTimelineViewer/index.tsx | 1 + .../features/explore/TraceView/TraceView.tsx | 2 ++ 6 files changed, 33 insertions(+), 20 deletions(-) diff --git a/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/__snapshots__/index.test.js.snap b/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/__snapshots__/index.test.js.snap index baf54ebe67e..3c12d1d9a8f 100644 --- a/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/__snapshots__/index.test.js.snap +++ b/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/__snapshots__/index.test.js.snap @@ -2,11 +2,8 @@ exports[` shallow tests matches a snapshot 1`] = `
', () => { itemsWrapperClassName: 'SomeClassName', viewBuffer: 10, viewBufferMin: 5, - windowScroller: false, + windowScroller: true, }; describe('shallow tests', () => { @@ -152,10 +152,6 @@ describe('', () => { instance = wrapper.instance(); }); - it('getViewHeight() returns the viewHeight', () => { - expect(instance.getViewHeight()).toBe(clientHeight); - }); - it('getBottomVisibleIndex() returns a number', () => { const n = instance.getBottomVisibleIndex(); expect(Number.isNaN(n)).toBe(false); @@ -233,9 +229,9 @@ describe('', () => { }, }); const hasChanged = instance._isViewChanged(); + expect(hasChanged).toBe(true); expect(spyFns.clientHeight).toHaveBeenCalled(); expect(spyFns.scrollTop).toHaveBeenCalled(); - expect(hasChanged).toBe(true); }); }); }); diff --git a/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/index.tsx b/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/index.tsx index 55bd4dbf86d..22bc0aeb421 100644 --- a/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/index.tsx +++ b/packages/jaeger-ui-components/src/TraceTimelineViewer/ListView/index.tsx @@ -67,7 +67,7 @@ type TListViewProps = { itemsWrapperClassName?: string; /** * When adding new items to the DOM, this is the number of items to add above - * and below the current view. E.g. if list is 100 items and is srcolled + * and below the current view. E.g. if list is 100 items and is scrolled * halfway down (so items [46, 55] are in view), then when a new range of * items is rendered, it will render items `46 - viewBuffer` to * `55 + viewBuffer`. @@ -89,15 +89,20 @@ type TListViewProps = { * - Ref:https://github.com/bvaughn/react-virtualized/blob/497e2a1942529560681d65a9ef9f5e9c9c9a49ba/docs/WindowScroller.md */ windowScroller?: boolean; + /** + * You need to pass in scrollElement when windowScroller is set to false. + * This element is responsible for tracking scrolling for lazy loading. + */ + scrollElement?: Element; }; -const DEFAULT_INITIAL_DRAW = 300; +const DEFAULT_INITIAL_DRAW = 100; /** * Virtualized list view component, for the most part, only renders the window * of items that are in-view with some buffer before and after. Listens for * scroll events and updates which items are rendered. See react-virtualized - * for a suite of components with similar, but generalized, functinality. + * for a suite of components with similar, but generalized, functionality. * https://github.com/bvaughn/react-virtualized * * Note: Presently, ListView cannot be a PureComponent. This is because ListView @@ -157,9 +162,9 @@ export default class ListView extends React.Component { _windowScrollListenerAdded: boolean; _htmlElm: HTMLElement; /** - * HTMLElement holding the scroller. + * Element holding the scroller. */ - _wrapperElm: HTMLElement | TNil; + _wrapperElm: Element | TNil; /** * HTMLElement holding the rendered items. */ @@ -202,6 +207,10 @@ export default class ListView extends React.Component { } window.addEventListener('scroll', this._onScroll); this._windowScrollListenerAdded = true; + } else { + // The wrapper element should be the one that handles the scrolling. Once we are not using scroll-canvas we can remove this. + this._wrapperElm = this.props.scrollElement; + this._wrapperElm?.addEventListener('scroll', this._onScroll); } } @@ -214,6 +223,8 @@ export default class ListView extends React.Component { componentWillUnmount() { if (this._windowScrollListenerAdded) { window.removeEventListener('scroll', this._onScroll); + } else { + this._wrapperElm?.removeEventListener('scroll', this._onScroll); } } @@ -308,8 +319,11 @@ export default class ListView extends React.Component { }; _initWrapper = (elm: HTMLElement | TNil) => { + if (!this.props.windowScroller) { + return; + } this._wrapperElm = elm; - if (!this.props.windowScroller && elm) { + if (elm) { this._viewHeight = elm.clientHeight; } }; @@ -374,8 +388,8 @@ export default class ListView extends React.Component { }; /** - * Get the height of the element at index `i`; first check the known heigths, - * fallbck to `.props.itemHeightGetter(...)`. + * Get the height of the element at index `i`; first check the known heights, + * fallback to `.props.itemHeightGetter(...)`. */ _getHeight = (i: number) => { const key = this.props.getKeyFromIndex(i); diff --git a/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.tsx b/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.tsx index 8b5a549a3c0..6e2e0b94f47 100644 --- a/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.tsx +++ b/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.tsx @@ -82,6 +82,7 @@ type TVirtualizedTraceViewOwnProps = { createSpanLink?: ( span: TraceSpan ) => { href: string; onClick?: (e: React.MouseEvent) => void; content: React.ReactNode }; + scrollElement?: Element; }; type VirtualizedTraceViewProps = TVirtualizedTraceViewOwnProps & TExtractUiFindFromStateReturn & TTraceTimeline; @@ -445,6 +446,7 @@ export class UnthemedVirtualizedTraceView extends React.Component
); diff --git a/packages/jaeger-ui-components/src/TraceTimelineViewer/index.tsx b/packages/jaeger-ui-components/src/TraceTimelineViewer/index.tsx index be668aaa066..c2d229004bc 100644 --- a/packages/jaeger-ui-components/src/TraceTimelineViewer/index.tsx +++ b/packages/jaeger-ui-components/src/TraceTimelineViewer/index.tsx @@ -102,6 +102,7 @@ type TProps = TExtractUiFindFromStateReturn & { createSpanLink?: ( span: TraceSpan ) => { href: string; onClick?: (e: React.MouseEvent) => void; content: React.ReactNode }; + scrollElement?: Element; }; type State = { diff --git a/public/app/features/explore/TraceView/TraceView.tsx b/public/app/features/explore/TraceView/TraceView.tsx index 23e1db961ac..b7e7dcd8083 100644 --- a/public/app/features/explore/TraceView/TraceView.tsx +++ b/public/app/features/explore/TraceView/TraceView.tsx @@ -83,6 +83,7 @@ export function TraceView(props: Props) { ); const createSpanLink = useMemo(() => createSpanLinkFactory(props.splitOpenFn), [props.splitOpenFn]); + const scrollElement = document.getElementsByClassName('scroll-canvas')[0]; if (!traceProp) { return null; @@ -148,6 +149,7 @@ export function TraceView(props: Props) { )} uiFind={search} createSpanLink={createSpanLink} + scrollElement={scrollElement} />