Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(trace) issues only collapse trace #81171

Open
wants to merge 2 commits into
base: jb/issues/trace-view
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ function EventTraceViewInner({event, organization}: EventTraceViewInnerProps) {
const meta = useTraceMeta([{traceSlug: traceId, timestamp: undefined}]);
const tree = useIssuesTraceTree({trace, meta, replay: null});

const hasNoTransactions = meta.data?.transactions === 0;
const shouldLoadTraceRoot = !trace.isPending && trace.data && !hasNoTransactions;
const shouldLoadTraceRoot = !trace.isPending && trace.data;

const rootEvent = useTraceRootEvent(shouldLoadTraceRoot ? trace.data! : null);

Expand All @@ -79,7 +78,7 @@ function EventTraceViewInner({event, organization}: EventTraceViewInnerProps) {
return {eventId: firstTransactionEventId};
}, [trace.data]);

if (trace.isPending || rootEvent.isPending || !rootEvent.data || hasNoTransactions) {
if (trace.isPending || rootEvent.isPending || !rootEvent.data) {
return null;
}

Expand Down Expand Up @@ -151,7 +150,7 @@ const TraceContentWrapper = styled('div')`

const ROW_HEIGHT = 24;
const MIN_ROW_COUNT = 1;
const MAX_HEIGHT = 500;
const MAX_HEIGHT = 12 * ROW_HEIGHT;
const MAX_ROW_COUNT = Math.floor(MAX_HEIGHT / ROW_HEIGHT);
const HEADER_HEIGHT = 26;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ export function IssuesTraceWaterfall(props: IssuesTraceWaterfallProps) {
(eventId && TraceTree.FindByID(props.tree.root, eventId)) ||
null;

// @TODO Scrolling to a node is not yet supported for issues, so always pin to the top
const index = -1;

if (index === -1 || !node) {
Expand Down Expand Up @@ -239,18 +238,20 @@ export function IssuesTraceWaterfall(props: IssuesTraceWaterfallProps) {
organization={organization}
/>
<IssuesTraceGrid layout={traceState.preferences.layout}>
<Trace
trace={props.tree}
rerender={rerender}
trace_id={props.traceSlug}
onRowClick={onRowClick}
onTraceSearch={noopTraceSearch}
previouslyFocusedNodeRef={previouslyFocusedNodeRef}
manager={viewManager}
scheduler={traceScheduler}
forceRerender={forceRender}
isLoading={props.tree.type === 'loading' || onLoadScrollStatus === 'pending'}
/>
<IssuesPointerDisabled>
<Trace
trace={props.tree}
rerender={rerender}
trace_id={props.traceSlug}
onRowClick={onRowClick}
onTraceSearch={noopTraceSearch}
previouslyFocusedNodeRef={previouslyFocusedNodeRef}
manager={viewManager}
scheduler={traceScheduler}
forceRerender={forceRender}
isLoading={props.tree.type === 'loading' || onLoadScrollStatus === 'pending'}
/>
</IssuesPointerDisabled>

{props.tree.type === 'loading' || onLoadScrollStatus === 'pending' ? (
<TraceWaterfallState.Loading />
Expand All @@ -264,6 +265,12 @@ export function IssuesTraceWaterfall(props: IssuesTraceWaterfallProps) {
);
}

const IssuesPointerDisabled = styled('div')`
position: relative;
height: 100%;
width: 100%;
`;

const IssuesTraceGrid = styled(TraceGrid)<{
layout: 'drawer bottom' | 'drawer left' | 'drawer right';
}>`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,32 @@ trace root
"
`;

exports[`IssuesTraceTree collapses sibling collapsed nodes 1`] = `
"
trace root
transaction 1 - transaction.op
collapsed
transaction 2 - transaction.op
collapsed
"
`;

exports[`IssuesTraceTree errors only 1`] = `
"
trace root
error
error
error
error
error
error
error
error
error
collapsed
"
`;

exports[`IssuesTraceTree preserves path to child error 1`] = `
"
trace root
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,27 @@ const traceWithChildError = makeTrace({
],
});

const errorsOnlyTrace = makeTrace({
transactions: [],
orphan_errors: new Array(20).fill(null).map(() => makeTraceError({})),
});

const traceWithSiblingCollapsedNodes = makeTrace({
transactions: [
makeTransaction({
transaction: 'transaction 1',
children: [
makeTransaction({}),
makeTransaction({}),
makeTransaction({transaction: 'transaction 2', errors: [makeTraceError({})]}),
makeTransaction({}),
],
}),
makeTransaction({transaction: 'transaction 3'}),
makeTransaction({transaction: 'transaction 4'}),
],
});

describe('IssuesTraceTree', () => {
it('collapsed nodes without errors', () => {
const tree = IssuesTraceTree.FromTrace(traceWithErrorInMiddle, {
Expand All @@ -40,4 +61,23 @@ describe('IssuesTraceTree', () => {

expect(tree.build().serialize()).toMatchSnapshot();
});

it('errors only', () => {
// has +100 issues at the end
const tree = IssuesTraceTree.FromTrace(errorsOnlyTrace, {
meta: null,
replay: null,
});

expect(tree.build().serialize()).toMatchSnapshot();
});

it('collapses sibling collapsed nodes', () => {
const tree = IssuesTraceTree.FromTrace(traceWithSiblingCollapsedNodes, {
meta: null,
replay: null,
});

expect(tree.build().serialize()).toMatchSnapshot();
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import {isTraceErrorNode} from 'sentry/views/performance/newTraceDetails/traceGuards';
import {
isCollapsedNode,
isTraceErrorNode,
} from 'sentry/views/performance/newTraceDetails/traceGuards';
import {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import type {ReplayRecord} from 'sentry/views/replays/types';

Expand All @@ -7,6 +10,8 @@ import {CollapsedNode} from '../traceModels/traceCollapsedNode';

import type {TraceTreeNode} from './traceTreeNode';

const MAX_ISSUES = 10;

export class IssuesTraceTree extends TraceTree {
static FromTrace(
trace: TraceTree.Trace,
Expand All @@ -24,13 +29,13 @@ export class IssuesTraceTree extends TraceTree {
const errorNodes = TraceTree.FindAll(
tree.root,
node => node.hasErrors || isTraceErrorNode(node)
);
).slice(0, MAX_ISSUES);

const preservePaths = new Set<TraceTreeNode>();
const preserveNodes = new Set<TraceTreeNode>();
for (const node of errorNodes) {
let current: TraceTreeNode | null = node;
while (current) {
preservePaths.add(current);
preserveNodes.add(current);
current = current.parent;
}
}
Expand All @@ -52,11 +57,7 @@ export class IssuesTraceTree extends TraceTree {
while (index < node.children.length) {
const start = index;

while (
node.children[index] &&
!node.children[index]?.hasErrors &&
!preservePaths.has(node.children[index])
) {
while (node.children[index] && !preserveNodes.has(node.children[index])) {
index++;
}

Expand Down Expand Up @@ -91,6 +92,34 @@ export class IssuesTraceTree extends TraceTree {

build() {
super.build();

// Since we only collapsed sibling nodes, it means that it is possible for the list to contain
// sibling collapsed nodes. We'll do a second pass to flatten these nodes and replace them with
// a single fake collapsed node.
for (let i = 0; i < this.list.length; i++) {
if (!isCollapsedNode(this.list[i])) {
continue;
}

const start = i;
while (i < this.list.length && isCollapsedNode(this.list[i])) {
i++;
}

if (i - start > 0) {
const newNode = new CollapsedNode(
this.list[start].parent!,
{type: 'collapsed'},
this.list[start].metadata
);

const removed = this.list.splice(start, i - start, newNode);

for (const node of removed) {
newNode.children = newNode.children.concat(node.children);
}
}
}
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1149,8 +1149,8 @@ export class TraceTree extends TraceTreeEventDispatcher {
if (isParentAutogroupedNode(next)) {
queue.push(next.head);
} else {
for (const child of next.children) {
queue.push(child);
for (let i = next.children.length - 1; i >= 0; i--) {
queue.push(next.children[i]);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,39 @@
import {useMemo} from 'react';

import {t} from 'sentry/locale';
import {isTraceErrorNode} from 'sentry/views/performance/newTraceDetails/traceGuards';

import type {CollapsedNode} from '../traceModels/traceCollapsedNode';
import {TraceTree} from '../traceModels/traceTree';
import type {TraceRowProps} from '../traceRow/traceRow';

export function TraceCollapsedRow(props: TraceRowProps<CollapsedNode>) {
const collapsedChildrenCount = useMemo(() => {
const collapsedNodeType = useMemo(() => {
let type: 'issues only' | '' = 'issues only';
let count = 1;
TraceTree.ForEachChild(props.node, () => count++);
return count;

TraceTree.ForEachChild(props.node, c => {
count++;
if (!isTraceErrorNode(c)) {
type = '';
}
});
return {count, type};
}, [props.node]);

return (
<div
key={props.index}
tabIndex={props.tabIndex}
className="Collapsed TraceRow"
className={`Collapsed ${collapsedNodeType.type === 'issues only' ? 'IssuesOnly' : ''} TraceRow`}
onPointerDown={props.onRowClick}
onKeyDown={props.onRowKeyDown}
style={props.style}
>
<div className="TraceLeftColumn" ref={props.registerListColumnRef}>
<div className="TraceLeftColumnInner" style={props.listColumnStyle}>
{collapsedChildrenCount}{' '}
{collapsedChildrenCount === 1 ? t('hidden span') : t('hidden spans')}
{collapsedNodeType.count}{' '}
{collapsedNodeType.count === 1 ? t('hidden span') : t('hidden spans')}
</div>
</div>
</div>
Expand Down
Loading