Skip to content
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 @@ -25,7 +25,6 @@ import {SectionKey} from 'sentry/views/issueDetails/streamline/context';
import {InterimSection} from 'sentry/views/issueDetails/streamline/interimSection';
import {useIssuesTraceTree} from 'sentry/views/performance/newTraceDetails/traceApi/useIssuesTraceTree';
import {useTrace} from 'sentry/views/performance/newTraceDetails/traceApi/useTrace';
import {isEAPTraceOccurrence} from 'sentry/views/performance/newTraceDetails/traceGuards';
import useTraceStateAnalytics from 'sentry/views/performance/newTraceDetails/useTraceStateAnalytics';

enum AnrRootCauseAllowlist {
Expand Down Expand Up @@ -143,12 +142,9 @@ export function AnrRootCause({event, organization}: Props) {
>
{potentialAnrRootCause?.map(occurence => {
const project = projects.find(p => p.id === occurence.project_id.toString());
const title = isEAPTraceOccurrence(occurence)
? occurence.description
: occurence.title;
const shortId = isEAPTraceOccurrence(occurence)
? occurence.short_id
: occurence.issue_short_id;
const isEAPOccurence = 'description' in occurence;
const title = isEAPOccurence ? occurence.description : occurence.title;
const shortId = isEAPOccurence ? occurence.short_id : occurence.issue_short_id;
return (
<IssueSummary key={occurence.issue_id}>
<Title>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {decodeScalar} from 'sentry/utils/queryString';
import type RequestError from 'sentry/utils/requestError/requestError';
import useOrganization from 'sentry/utils/useOrganization';
import usePageFilters from 'sentry/utils/usePageFilters';
import {isValidEventUUID} from 'sentry/views/performance/newTraceDetails/traceApi/utils';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import {useIsEAPTraceEnabled} from 'sentry/views/performance/newTraceDetails/useIsEAPTraceEnabled';

Expand Down Expand Up @@ -291,3 +290,9 @@ export function useTrace(

return isDemoMode ? demoTrace : isEAPEnabled ? eapTraceQuery : traceQuery;
}

const isValidEventUUID = (id: string): boolean => {
const uuidRegex =
/^[0-9a-f]{8}[0-9a-f]{4}[1-5][0-9a-f]{3}[89ab][0-9a-f]{3}[0-9a-f]{12}$/i;
return uuidRegex.test(id);
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,7 @@ import {
type OurLogsResponseItem,
} from 'sentry/views/explore/logs/types';
import {TraceItemDataset} from 'sentry/views/explore/types';
import {getRepresentativeTraceEvent} from 'sentry/views/performance/newTraceDetails/traceApi/utils';
import {
isEAPError,
isTraceError,
} from 'sentry/views/performance/newTraceDetails/traceGuards';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import {useIsEAPTraceEnabled} from 'sentry/views/performance/newTraceDetails/useIsEAPTraceEnabled';

type Params = {
logs: OurLogsResponseItem[] | undefined;
Expand All @@ -34,28 +28,20 @@ export function useTraceRootEvent({
logs,
traceId,
}: Params): TraceRootEventQueryResults {
const rep = getRepresentativeTraceEvent(tree, logs);
const rep = tree.findRepresentativeTraceNode({logs});
const organization = useOrganization();

// TODO: This is a bit of a mess, we won't need all of this once we switch to EAP only
const treeIsLoading = tree.type === 'loading';
const hasOnlyLogs = !!(tree.type === 'empty' && logs && logs.length > 0);
const enabledBase =
!treeIsLoading && (tree.type === 'trace' || hasOnlyLogs) && !!rep?.event && !!traceId;

const isRepEventError =
rep.event && OurLogKnownFieldKey.PROJECT_ID in rep.event
? false
: isTraceError(rep.event) || isEAPError(rep.event);
const enabledBase = !treeIsLoading && !!rep?.event;

const isEAPTraceEnabled = useIsEAPTraceEnabled();
const isEAPQueryEnabled =
!isRepEventError && // Errors are not supported in EAP yet
(isEAPTraceEnabled || (!treeIsLoading && hasOnlyLogs));
const isRepLog = rep?.dataset === TraceItemDataset.LOGS;
const isEAPQueryEnabled = !!(isRepLog || rep?.event?.isEAPEvent);

const projectSlug = rep?.event?.projectSlug;
const legacyRootEvent = useApiQuery<EventTransaction>(
[
`/organizations/${organization.slug}/events/${rep?.event?.project_slug}:${rep?.event?.event_id}/`,
`/organizations/${organization.slug}/events/${projectSlug}:${rep?.event?.id}/`,
{
query: {
referrer: 'trace-details-summary',
Expand All @@ -65,32 +51,27 @@ export function useTraceRootEvent({
{
// 10 minutes
staleTime: 1000 * 60 * 10,
enabled: enabledBase && !isEAPQueryEnabled,
enabled: enabledBase && !isEAPQueryEnabled && !!projectSlug && !!rep?.event?.id,
}
);

const projectId = rep.event
const projectId = rep?.event
? OurLogKnownFieldKey.PROJECT_ID in rep.event
? rep.event[OurLogKnownFieldKey.PROJECT_ID]
: rep.event.project_id
: rep.event.projectId
: '';
const eventId = rep.event
? OurLogKnownFieldKey.ID in rep.event
const eventId = rep?.event
? OurLogKnownFieldKey.PROJECT_ID in rep.event
? rep.event[OurLogKnownFieldKey.ID]
: rep.event.event_id
: rep.event.id
: '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Wrong property checked in eventId condition

The condition for extracting eventId checks for OurLogKnownFieldKey.PROJECT_ID in the event, but then accesses OurLogKnownFieldKey.ID. The original code checked OurLogKnownFieldKey.ID in rep.event before accessing the ID field. While this works in practice because logs always contain both fields, it's checking the wrong property and could fail if the data contract changes. The condition should check for the property that's actually being accessed.

Fix in Cursor Fix in Web


const itemTypes = {
log: TraceItemDataset.LOGS,
span: TraceItemDataset.SPANS,
uptime_check: TraceItemDataset.UPTIME_RESULTS,
};
const dataset = rep?.dataset ?? TraceItemDataset.SPANS;

const rootEvent = useTraceItemDetails({
traceItemId: String(eventId),
projectId: String(projectId),
traceId,
traceItemType: itemTypes[rep.type],
traceItemType: dataset,
referrer: 'api.explore.log-item-details',
enabled: enabledBase && isEAPQueryEnabled,
});
Expand Down
126 changes: 16 additions & 110 deletions static/app/views/performance/newTraceDetails/traceApi/utils.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
import type {TraceItemDetailsResponse} from 'sentry/views/explore/hooks/useTraceItemDetails';
import type {OurLogsResponseItem} from 'sentry/views/explore/logs/types';
import type {TraceSplitResults} from 'sentry/views/performance/newTraceDetails/traceApi/types';
import type {TraceRootEventQueryResults} from 'sentry/views/performance/newTraceDetails/traceApi/useTraceRootEvent';
import {
isEAPTraceNode,
isRootEvent,
isTraceNode,
isTraceSplitResult,
isUptimeCheckNode,
} from 'sentry/views/performance/newTraceDetails/traceGuards';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import type {BaseNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/baseNode';

export function isBrowserRequestNode(node: BaseNode): boolean {
return (
// Adjust for SDK changes in https://github.com/getsentry/sentry-javascript/pull/13527
node.op === 'browser.request' ||
(node.op === 'browser' && node.description === 'request')
);
}

export function isTraceSplitResult(
result: TraceTree.Trace
): result is TraceSplitResults<TraceTree.Transaction> {
return 'transactions' in result && 'orphan_errors' in result;
}

export function isEmptyTrace(trace: TraceTree.Trace): boolean {
if (isTraceSplitResult(trace)) {
Expand All @@ -18,110 +26,8 @@ export function isEmptyTrace(trace: TraceTree.Trace): boolean {
return trace.length === 0;
}

const CANDIDATE_TRACE_TITLE_OPS = ['pageload', 'navigation', 'ui.load'];

export type RepresentativeTraceEvent = {
event: TraceTree.TraceEvent | OurLogsResponseItem | null;
type: 'span' | 'log' | 'uptime_check';
};

export const getRepresentativeTraceEvent = (
tree: TraceTree,
logs: OurLogsResponseItem[] | undefined
): RepresentativeTraceEvent => {
const hasLogs = logs && logs.length > 0;
if (tree.type === 'empty' && hasLogs) {
return {
event: logs[0]!,
type: 'log',
};
}

const traceNode = tree.root.children[0];

if (!traceNode) {
return {
event: null,
type: 'span',
};
}

if (!isTraceNode(traceNode) && !isEAPTraceNode(traceNode)) {
throw new TypeError('Not trace node');
}

const traceChild = traceNode.children[0];

if (traceChild && isUptimeCheckNode(traceChild)) {
return {type: 'uptime_check', event: traceChild.value};
}

let preferredRootEvent: TraceTree.TraceEvent | null = null;
let firstRootEvent: TraceTree.TraceEvent | null = null;
let candidateEvent: TraceTree.TraceEvent | null = null;
let firstEvent: TraceTree.TraceEvent | null = null;

const isEAP = isEAPTraceNode(traceNode);
const events = isEAP
? traceNode.value
: [...traceNode.value.transactions, ...traceNode.value.orphan_errors];
for (const event of events) {
if (isRootEvent(event)) {
if (!firstRootEvent) {
firstRootEvent = event;
}

if (hasPreferredOp(event)) {
preferredRootEvent = event;
break;
}
// Otherwise we keep looking for a root eap transaction. If we don't find one, we use other roots, like standalone spans.
continue;
} else if (
// If we haven't found a root transaction, but we found a candidate transaction
// with an op that we care about, we can use it for the title. We keep looking for
// a root.
!candidateEvent &&
hasPreferredOp(event)
) {
candidateEvent = event;
continue;
} else if (!firstEvent) {
// If we haven't found a root or candidate transaction, we can use the first transaction
// in the trace for the title.
firstEvent = event;
}
}

return {
event: preferredRootEvent ?? firstRootEvent ?? candidateEvent ?? firstEvent,
type: 'span',
};
};

export const isTraceItemDetailsResponse = (
data: TraceRootEventQueryResults['data']
): data is TraceItemDetailsResponse => {
return data !== undefined && 'attributes' in data;
};

export const isValidEventUUID = (id: string): boolean => {
const uuidRegex =
/^[0-9a-f]{8}[0-9a-f]{4}[1-5][0-9a-f]{3}[89ab][0-9a-f]{3}[0-9a-f]{12}$/i;
return uuidRegex.test(id);
};

/**
* Prefer "special" root events over generic root events when generating a title
* for the waterfall view. Picking these improves contextual navigation for linked
* traces, resulting in more meaningful waterfall titles.
*/
function hasPreferredOp(event: TraceTree.TraceEvent): boolean {
const op =
'op' in event
? event.op
: 'transaction.op' in event
? event['transaction.op']
: undefined;
return !!op && CANDIDATE_TRACE_TITLE_OPS.includes(op);
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
import {SectionDivider} from 'sentry/views/issueDetails/streamline/foldSection';
import type {TraceRootEventQueryResults} from 'sentry/views/performance/newTraceDetails/traceApi/useTraceRootEvent';
import {isTraceItemDetailsResponse} from 'sentry/views/performance/newTraceDetails/traceApi/utils';
import {isEAPTraceNode} from 'sentry/views/performance/newTraceDetails/traceGuards';
import type {TraceTree} from 'sentry/views/performance/newTraceDetails/traceModels/traceTree';
import {
TRACE_VIEW_MOBILE_VITALS,
Expand Down Expand Up @@ -52,9 +51,8 @@ export function TraceContextVitals({rootEventResults, tree, containerWidth}: Pro
? TRACE_VIEW_WEB_VITALS
: TRACE_VIEW_MOBILE_VITALS;

const isEAPTrace = isEAPTraceNode(traceNode);
const collectedVitals =
isEAPTrace && tree.vital_types.has('mobile')
traceNode.isEAPEvent && tree.vital_types.has('mobile')
? getMobileVitalsFromRootEventResults(rootEventResults.data)
: Array.from(tree.vitals.values()).flat();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import {OrganizationFixture} from 'sentry-fixture/organization';

import {render, screen} from 'sentry-test/reactTestingLibrary';

import type {TraceTreeNodeExtra} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/baseNode';
import {EapSpanNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/eapSpanNode';
import {SiblingAutogroupNode} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeNode/siblingAutogroupNode';
import {
makeEAPSpan,
makeSiblingAutogroup,
} from 'sentry/views/performance/newTraceDetails/traceModels/traceTreeTestUtils';
import {DEFAULT_TRACE_VIEW_PREFERENCES} from 'sentry/views/performance/newTraceDetails/traceState/tracePreferences';
import {TraceStateProvider} from 'sentry/views/performance/newTraceDetails/traceState/traceStateProvider';

import {AutogroupNodeDetails} from './index';

const createMockExtra = (
overrides: Partial<TraceTreeNodeExtra> = {}
): TraceTreeNodeExtra => ({
organization: OrganizationFixture(),
...overrides,
});

describe('AutogroupNodeDetails', () => {
it('renders autogroup details with title and description', () => {
const organization = OrganizationFixture();
const extra = createMockExtra({organization});
const autogroupValue = makeSiblingAutogroup({
span_id: 'test-span-id',
autogrouped_by: {
op: 'db.query',
description: 'SELECT * FROM users',
},
});

const childSpanValue = makeEAPSpan({event_id: 'child-span-1'});
const childNode = new EapSpanNode(null, childSpanValue, extra);
const node = new SiblingAutogroupNode(null, autogroupValue, extra);
node.children = [childNode];

render(
<TraceStateProvider initialPreferences={DEFAULT_TRACE_VIEW_PREFERENCES}>
<AutogroupNodeDetails
node={node as any}
organization={organization}
onTabScrollToNode={jest.fn()}
onParentClick={jest.fn()}
manager={null}
replay={null}
traceId="test-trace-id"
/>
</TraceStateProvider>
);

expect(screen.getByText('Autogroup')).toBeInTheDocument();

expect(screen.getByText(/ID: test-span-id/)).toBeInTheDocument();

expect(
screen.getByText(/This block represents autogrouped spans/)
).toBeInTheDocument();

expect(
screen.getByText('5 or more siblings with the same operation and description')
).toBeInTheDocument();
expect(
screen.getByText('2 or more descendants with the same operation')
).toBeInTheDocument();

expect(
screen.getByText(/You can either open this autogroup using the chevron/)
).toBeInTheDocument();
});
});
Loading
Loading