Skip to content

feat(trace-eap-waterfall): Rendering errors associated with spans #88060

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

Merged
merged 3 commits into from
Apr 1, 2025
Merged
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 @@ -19,6 +19,9 @@ import useOrganization from 'sentry/utils/useOrganization';
import useProjects from 'sentry/utils/useProjects';
import {IssueTraceWaterfallOverlay} from 'sentry/views/performance/newTraceDetails/issuesTraceWaterfallOverlay';
import {
isEAPSpanNode,
isEAPTransactionNode,
isNonTransactionEAPSpanNode,
isSpanNode,
isTraceErrorNode,
isTransactionNode,
Expand Down Expand Up @@ -200,8 +203,9 @@ export function IssuesTraceWaterfall(props: IssuesTraceWaterfallProps) {
}
}
}
if (isSpanNode(n)) {
if (n.value.span_id === props.event.eventID) {
if (isSpanNode(n) || isEAPSpanNode(n)) {
const spanId = 'span_id' in n.value ? n.value.span_id : n.value.event_id;
if (spanId === props.event.eventID) {
return true;
}
for (const e of n.errors) {
Expand All @@ -223,8 +227,8 @@ export function IssuesTraceWaterfall(props: IssuesTraceWaterfallProps) {
// the error may have been attributed to, otherwise we look at the transaction.
const node =
nodes?.find(n => isTraceErrorNode(n)) ||
nodes?.find(n => isSpanNode(n)) ||
nodes?.find(n => isTransactionNode(n));
nodes?.find(n => isSpanNode(n) || isNonTransactionEAPSpanNode(n)) ||
nodes?.find(n => isTransactionNode(n) || isEAPTransactionNode(n));

const index = node ? props.tree.list.indexOf(node) : -1;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ import {t, tct, tn} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {Group} from 'sentry/types/group';
import type {Organization} from 'sentry/types/organization';
import type {
TraceError,
TraceErrorOrIssue,
TracePerformanceIssue,
} from 'sentry/utils/performance/quickTrace/types';
import type {TracePerformanceIssue} from 'sentry/utils/performance/quickTrace/types';
import {useApiQuery} from 'sentry/utils/queryClient';
import type RequestError from 'sentry/utils/requestError/requestError';
import useOrganization from 'sentry/utils/useOrganization';
Expand All @@ -40,7 +36,7 @@ import {TraceDrawerComponents} from '../styles';
import {IssueSummary} from './issueSummary';

type IssueProps = {
issue: TraceErrorOrIssue;
issue: TraceTree.TraceIssue;
organization: Organization;
};

Expand All @@ -63,7 +59,10 @@ const issueOrderPriority: Record<keyof Theme['level'], number> = {
unknown: 6,
};

function sortIssuesByLevel(a: TraceError, b: TraceError): number {
function sortIssuesByLevel(
a: TraceTree.TraceErrorIssue,
b: TraceTree.TraceErrorIssue
): number {
// If the level is not defined in the priority map, default to unknown
const aPriority = issueOrderPriority[a.level] ?? issueOrderPriority.unknown;
const bPriority = issueOrderPriority[b.level] ?? issueOrderPriority.unknown;
Expand Down Expand Up @@ -352,15 +351,15 @@ function LegacyIssue(
) : null;
}
type IssueListProps = {
issues: TraceErrorOrIssue[];
issues: TraceTree.TraceIssue[];
node: TraceTreeNode<TraceTree.NodeValue>;
organization: Organization;
};

export function IssueList({issues, node, organization}: IssueListProps) {
const hasTraceNewUi = useHasTraceNewUi();
const uniqueErrorIssues = useMemo(() => {
const unique: TraceError[] = [];
const unique: TraceTree.TraceErrorIssue[] = [];

const seenIssues: Set<number> = new Set();

Expand Down Expand Up @@ -451,7 +450,7 @@ function IssueListHeader({
errorIssues,
performanceIssues,
}: {
errorIssues: TraceError[];
errorIssues: TraceTree.TraceErrorIssue[];
node: TraceTreeNode<TraceTree.NodeValue>;
performanceIssues: TracePerformanceIssue[];
}) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,10 @@ export function SpanNodeDetails({
analyticsPageSource={LogsAnalyticsPageSource.TRACE_DETAILS}
>
<LogsPageDataProvider>
{issues.length > 0 ? (
<IssueList organization={organization} issues={issues} node={node} />
) : null}
<LogDetails />
<div>TO DO: EAP Span Details</div>
</LogsPageDataProvider>
</LogsPageParamsProvider>
</TraceDrawerComponents.BodyContainer>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {t, tn} from 'sentry/locale';
import type {EventTransaction} from 'sentry/types/event';
import type {Organization} from 'sentry/types/organization';
import getDuration from 'sentry/utils/duration/getDuration';
import type {TraceErrorOrIssue} from 'sentry/utils/performance/quickTrace/types';
import type {UseApiQueryResult} from 'sentry/utils/queryClient';
import type RequestError from 'sentry/utils/requestError/requestError';
import {useParams} from 'sentry/utils/useParams';
Expand Down Expand Up @@ -38,7 +37,7 @@ export function GeneralInfo(props: GeneralInfoProps) {
return [];
}

const unique: TraceErrorOrIssue[] = [];
const unique: TraceTree.TraceErrorIssue[] = [];
const seenIssues: Set<number> = new Set();

for (const issue of traceNode.errors) {
Expand All @@ -57,7 +56,7 @@ export function GeneralInfo(props: GeneralInfoProps) {
return [];
}

const unique: TraceErrorOrIssue[] = [];
const unique: TraceTree.TracePerformanceIssue[] = [];
const seenIssues: Set<number> = new Set();

for (const issue of traceNode.performance_issues) {
Expand Down
23 changes: 22 additions & 1 deletion static/app/views/performance/newTraceDetails/traceGuards.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ export function isEAPSpanNode(
return !!(node.value && 'is_transaction' in node.value);
}

export function isNonTransactionEAPSpanNode(
node: TraceTreeNode<TraceTree.NodeValue>
): node is TraceTreeNode<TraceTree.EAPSpan> {
return isEAPSpanNode(node) && !isEAPTransactionNode(node);
}

export function isTransactionNode(
node: TraceTreeNode<TraceTree.NodeValue>
): node is TraceTreeNode<TraceTree.Transaction> {
Expand All @@ -49,6 +55,21 @@ export function isTransactionNode(
);
}

export function isEAPError(value: TraceTree.NodeValue): value is TraceTree.EAPError {
return !!(
value &&
'event_type' in value &&
value.event_type === 'error' &&
!('message' in value) // a bit gross, but we won't need this soon as we remove the legacy error type
);
}

export function isEAPErrorNode(
node: TraceTreeNode<TraceTree.NodeValue>
): node is TraceTreeNode<TraceTree.EAPError> {
return isEAPError(node.value);
}

export function isParentAutogroupedNode(
node: TraceTreeNode<TraceTree.NodeValue>
): node is ParentAutogroupNode {
Expand Down Expand Up @@ -178,7 +199,7 @@ export function getPageloadTransactionChildCount(
}

export function isTracePerformanceIssue(
issue: TraceTree.TraceError | TraceTree.TracePerformanceIssue
issue: TraceTree.TraceIssue
): issue is TraceTree.TracePerformanceIssue {
return 'suspect_spans' in issue;
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ import {space} from 'sentry/styles/space';
import type {EventTransaction} from 'sentry/types/event';
import type {Organization} from 'sentry/types/organization';
import getDuration from 'sentry/utils/duration/getDuration';
import type {
TraceErrorOrIssue,
TraceMeta,
} from 'sentry/utils/performance/quickTrace/types';
import type {TraceMeta} from 'sentry/utils/performance/quickTrace/types';
import type {UseApiQueryResult} from 'sentry/utils/queryClient';
import type RequestError from 'sentry/utils/requestError/requestError';

Expand Down Expand Up @@ -65,7 +62,7 @@ export function Meta(props: MetaProps) {
return [];
}

const unique: TraceErrorOrIssue[] = [];
const unique: TraceTree.TraceErrorIssue[] = [];
const seenIssues: Set<number> = new Set();

for (const issue of traceNode.errors) {
Expand All @@ -84,7 +81,7 @@ export function Meta(props: MetaProps) {
return [];
}

const unique: TraceErrorOrIssue[] = [];
const unique: TraceTree.TracePerformanceIssue[] = [];
const seenIssues: Set<number> = new Set();

for (const issue of traceNode.performance_issues) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import type {
TraceError,
TracePerformanceIssue,
} from 'sentry/utils/performance/quickTrace/types';

import type {TraceTree} from './traceModels/traceTree';

interface EventTypeIconProps {
event: TracePerformanceIssue | TraceError | TraceTree.Profile;
event: TraceTree.TraceIssue | TraceTree.Profile;
}

function EventTypeIcon(props: EventTypeIconProps) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type {Client} from 'sentry/api';
import type {Organization} from 'sentry/types/organization';
import {
isEAPSpanNode,
isTraceErrorNode,
isTransactionNode,
} from 'sentry/views/performance/newTraceDetails/traceGuards';
Expand Down Expand Up @@ -65,7 +66,7 @@ export class IssuesTraceTree extends TraceTree {
if (isTraceErrorNode(n)) {
return n.value.event_id === eventId;
}
if (isTransactionNode(n)) {
if (isTransactionNode(n) || isEAPSpanNode(n)) {
if (n.value.event_id === eventId) {
return true;
}
Expand All @@ -85,8 +86,14 @@ export class IssuesTraceTree extends TraceTree {
return false;
});

if (node && isTransactionNode(node)) {
return tree.zoom(node, true, options).then(() => {});
if (node) {
if (isTransactionNode(node)) {
return tree.zoom(node, true, options).then(() => {});
}

if (isEAPSpanNode(node)) {
tree.expand(node, true);
}
}

return Promise.resolve();
Expand All @@ -106,7 +113,9 @@ export class IssuesTraceTree extends TraceTree {
const preserveNodes = new Set(preserveLeafNodes);

for (const node of preserveLeafNodes) {
const parentTransaction = TraceTree.ParentTransaction(node);
const parentTransaction = isEAPSpanNode(node)
? TraceTree.ParentEAPTransaction(node)
: TraceTree.ParentTransaction(node);
if (parentTransaction) {
preserveNodes.add(parentTransaction);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {ParentAutogroupNode} from './parentAutogroupNode';
import {TraceTree} from './traceTree';
import {
assertTransactionNode,
makeEAPError,
makeEAPSpan,
makeEAPTrace,
makeEventTransaction,
Expand Down Expand Up @@ -170,6 +171,21 @@ const parentAutogroupSpansWithTailChildren = [
}),
];

const eapTraceWithErrors = makeEAPTrace([
makeEAPSpan({
event_id: 'eap-span-1',
is_transaction: true,
errors: [],
children: [
makeEAPSpan({
event_id: 'eap-span-2',
is_transaction: false,
errors: [makeEAPError()],
}),
],
}),
]);

function findTransactionByEventId(tree: TraceTree, eventId: string) {
return TraceTree.Find(
tree.root,
Expand Down Expand Up @@ -536,6 +552,18 @@ describe('TraceTree', () => {
expect(tree.build().serialize()).toMatchSnapshot();
});

it('adds eap errors to tree nodes', () => {
const tree = TraceTree.FromTrace(eapTraceWithErrors, traceMetadata);

expect(tree.root.children[0]!.errors.size).toBe(1);

const eapTransaction = findEAPSpanByEventId(tree, 'eap-span-1');
const eapSpan = findEAPSpanByEventId(tree, 'eap-span-2');

expect(eapTransaction?.errors.size).toBe(1);
expect(eapSpan?.errors.size).toBe(1);
});

it('initializes expanded based on is_transaction property', () => {
const tree = TraceTree.FromTrace(
makeEAPTrace([
Expand Down
Loading
Loading