-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(groupingInfo): Refactor grouping info to avoid storing redundant data #101135
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
base: master
Are you sure you want to change the base?
Changes from all commits
451bd05
ad35eec
be9c508
fdc486e
e393d05
84b8782
ed5435b
ed5de2e
bf44c8c
19d8a97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,37 @@ import type {Group} from 'sentry/types/group'; | |
import {useApiQuery} from 'sentry/utils/queryClient'; | ||
import useOrganization from 'sentry/utils/useOrganization'; | ||
|
||
type EventGroupingInfoResponse = Record<string, EventGroupVariant>; | ||
type EventGroupingInfoResponseOld = Record<string, EventGroupVariant>; | ||
type EventGroupingInfoResponse = { | ||
grouping_config: string | null; | ||
variants: Record<string, EventGroupVariant>; | ||
}; | ||
|
||
// temporary function to convert the old response structure to the new one | ||
function eventGroupingInfoResponseOldToNew( | ||
old: EventGroupingInfoResponseOld | null | ||
): EventGroupingInfoResponse | null { | ||
const grouping_config = old | ||
? ( | ||
Object.values(old).find( | ||
variant => 'config' in variant && variant.config?.id | ||
) as any | ||
)?.config?.id | ||
: null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Config Extraction Fails Without ID CheckThe |
||
return old | ||
? { | ||
grouping_config, | ||
variants: old, | ||
} | ||
: null; | ||
Comment on lines
+21
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, totally not worth changing in this temporary function, but just an option to keep in mind. If you do an early out ( |
||
} | ||
|
||
// temporary function to check if the respinse is old type | ||
function isOld( | ||
data: EventGroupingInfoResponseOld | EventGroupingInfoResponse | null | ||
): data is EventGroupingInfoResponseOld { | ||
return data ? !('grouping_config' in data) : false; | ||
} | ||
|
||
function generatePerformanceGroupInfo({ | ||
event, | ||
|
@@ -27,21 +57,24 @@ function generatePerformanceGroupInfo({ | |
|
||
return group | ||
? { | ||
[group.issueType]: { | ||
contributes: true, | ||
description: t('performance problem'), | ||
hash: event.occurrence?.fingerprint[0] || '', | ||
hashMismatch: false, | ||
hint: null, | ||
key: group.issueType, | ||
type: EventGroupVariantType.PERFORMANCE_PROBLEM, | ||
evidence: { | ||
op: evidenceData?.op, | ||
parent_span_ids: evidenceData?.parentSpanIds, | ||
cause_span_ids: evidenceData?.causeSpanIds, | ||
offender_span_ids: evidenceData?.offenderSpanIds, | ||
desc: t('performance problem'), | ||
fingerprint: hash, | ||
grouping_config: null, | ||
variants: { | ||
[group.issueType]: { | ||
contributes: true, | ||
description: t('performance problem'), | ||
hash: event.occurrence?.fingerprint[0] || '', | ||
hashMismatch: false, | ||
hint: null, | ||
key: group.issueType, | ||
type: EventGroupVariantType.PERFORMANCE_PROBLEM, | ||
evidence: { | ||
op: evidenceData?.op, | ||
parent_span_ids: evidenceData?.parentSpanIds, | ||
cause_span_ids: evidenceData?.causeSpanIds, | ||
offender_span_ids: evidenceData?.offenderSpanIds, | ||
desc: t('performance problem'), | ||
fingerprint: hash, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
@@ -61,14 +94,26 @@ export function useEventGroupingInfo({ | |
|
||
const hasPerformanceGrouping = event.occurrence && event.type === 'transaction'; | ||
|
||
const {data, isPending, isError, isSuccess} = useApiQuery<EventGroupingInfoResponse>( | ||
[`/projects/${organization.slug}/${projectSlug}/events/${event.id}/grouping-info/`], | ||
{enabled: !hasPerformanceGrouping, staleTime: Infinity} | ||
); | ||
const {data, isPending, isError, isSuccess} = useApiQuery< | ||
EventGroupingInfoResponseOld | EventGroupingInfoResponse | ||
>([`/projects/${organization.slug}/${projectSlug}/events/${event.id}/grouping-info/`], { | ||
enabled: !hasPerformanceGrouping, | ||
staleTime: Infinity, | ||
}); | ||
|
||
const groupInfo = hasPerformanceGrouping | ||
const groupInfoRaw = hasPerformanceGrouping | ||
? generatePerformanceGroupInfo({group, event}) | ||
: (data ?? null); | ||
|
||
return {groupInfo, isPending, isError, isSuccess, hasPerformanceGrouping}; | ||
const groupInfo = isOld(groupInfoRaw) | ||
? eventGroupingInfoResponseOldToNew(groupInfoRaw) | ||
: groupInfoRaw; | ||
|
||
return { | ||
groupInfo, | ||
isPending, | ||
isError, | ||
isSuccess, | ||
hasPerformanceGrouping, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Unnecessary API Mock in Local Data Test
The test "gets performance new grouping info from group/event data" mocks an API response for grouping info, then asserts the API call isn't made. Since performance grouping data is derived locally, this mock is unnecessary and creates a confusing contradiction in the test.