-
Notifications
You must be signed in to change notification settings - Fork 249
feat: denoise search results #751
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
Changes from all commits
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 |
---|---|---|
|
@@ -503,6 +503,18 @@ function DBSearchPage() { | |
} | ||
}, [analysisMode, setIsLive]); | ||
|
||
const [denoiseResults, _setDenoiseResults] = useQueryState( | ||
'denoise', | ||
parseAsBoolean.withDefault(false), | ||
); | ||
const setDenoiseResults = useCallback( | ||
(value: boolean) => { | ||
setIsLive(false); | ||
_setDenoiseResults(value); | ||
}, | ||
[setIsLive, _setDenoiseResults], | ||
); | ||
|
||
const { | ||
control, | ||
watch, | ||
|
@@ -719,11 +731,11 @@ function DBSearchPage() { | |
const onTableScroll = useCallback( | ||
(scrollTop: number) => { | ||
// If the user scrolls a bit down, kick out of live mode | ||
if (scrollTop > 16) { | ||
if (scrollTop > 16 && isLive) { | ||
setIsLive(false); | ||
} | ||
}, | ||
[setIsLive], | ||
[isLive, setIsLive], | ||
); | ||
|
||
const onRowExpandClick = useCallback( | ||
|
@@ -1247,6 +1259,8 @@ function DBSearchPage() { | |
> | ||
<ErrorBoundary message="Unable to render search filters"> | ||
<DBSearchPageFilters | ||
denoiseResults={denoiseResults} | ||
setDenoiseResults={setDenoiseResults} | ||
isLive={isLive} | ||
analysisMode={analysisMode} | ||
setAnalysisMode={setAnalysisMode} | ||
|
@@ -1534,25 +1548,31 @@ function DBSearchPage() { | |
</> | ||
) : ( | ||
<> | ||
{shouldShowLiveModeHint && analysisMode === 'results' && ( | ||
<div | ||
className="d-flex justify-content-center" | ||
style={{ height: 0 }} | ||
> | ||
{shouldShowLiveModeHint && | ||
analysisMode === 'results' && | ||
denoiseResults != true && ( | ||
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: 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. I think a bool comparison here is good |
||
<div | ||
style={{ position: 'relative', top: -20, zIndex: 2 }} | ||
className="d-flex justify-content-center" | ||
style={{ height: 0 }} | ||
> | ||
<Button | ||
size="compact-xs" | ||
variant="outline" | ||
onClick={handleResumeLiveTail} | ||
<div | ||
style={{ | ||
position: 'relative', | ||
top: -20, | ||
zIndex: 2, | ||
}} | ||
> | ||
<i className="bi text-success bi-lightning-charge-fill me-2" /> | ||
Resume Live Tail | ||
</Button> | ||
<Button | ||
size="compact-xs" | ||
variant="outline" | ||
onClick={handleResumeLiveTail} | ||
> | ||
<i className="bi text-success bi-lightning-charge-fill me-2" /> | ||
Resume Live Tail | ||
</Button> | ||
</div> | ||
</div> | ||
</div> | ||
)} | ||
)} | ||
{chartConfig && | ||
dbSqlRowTableConfig && | ||
analysisMode === 'results' && ( | ||
|
@@ -1565,6 +1585,7 @@ function DBSearchPage() { | |
queryKeyPrefix={QUERY_KEY_PREFIX} | ||
onScroll={onTableScroll} | ||
onError={handleTableError} | ||
denoiseResults={denoiseResults} | ||
/> | ||
)} | ||
</> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,11 @@ import { | |
} from '@hyperdx/common-utils/dist/types'; | ||
import { splitAndTrimWithBracket } from '@hyperdx/common-utils/dist/utils'; | ||
import { Box, Code, Flex, Text } from '@mantine/core'; | ||
import { FetchNextPageOptions } from '@tanstack/react-query'; | ||
import { | ||
FetchNextPageOptions, | ||
useQuery, | ||
useQueryClient, | ||
} from '@tanstack/react-query'; | ||
import { | ||
ColumnDef, | ||
ColumnResizeMode, | ||
|
@@ -41,6 +45,7 @@ import { useVirtualizer } from '@tanstack/react-virtual'; | |
|
||
import { useTableMetadata } from '@/hooks/useMetadata'; | ||
import useOffsetPaginatedQuery from '@/hooks/useOffsetPaginatedQuery'; | ||
import { useGroupedPatterns } from '@/hooks/usePatterns'; | ||
import useRowWhere from '@/hooks/useRowWhere'; | ||
import { UNDEFINED_WIDTH } from '@/tableUtils'; | ||
import { FormatTime } from '@/useFormatTime'; | ||
|
@@ -900,6 +905,7 @@ export function DBSqlRowTable({ | |
isLive = false, | ||
queryKeyPrefix, | ||
onScroll, | ||
denoiseResults = false, | ||
}: { | ||
config: ChartConfigWithDateRange; | ||
onRowExpandClick?: (where: string) => void; | ||
|
@@ -909,6 +915,7 @@ export function DBSqlRowTable({ | |
isLive?: boolean; | ||
onScroll?: (scrollTop: number) => void; | ||
onError?: (error: Error | ClickHouseQueryError) => void; | ||
denoiseResults?: boolean; | ||
}) { | ||
const mergedConfig = useConfigWithPrimaryAndPartitionKey(config); | ||
|
||
|
@@ -964,7 +971,7 @@ export function DBSqlRowTable({ | |
}); | ||
return newRow; | ||
}); | ||
}, [data, objectTypeColumns]); | ||
}, [data, objectTypeColumns, columnMap]); | ||
|
||
const aliasMap = chSqlToAliasMap(data?.chSql ?? { sql: '', params: {} }); | ||
|
||
|
@@ -983,23 +990,117 @@ export function DBSqlRowTable({ | |
} | ||
}, [isError, onError, error]); | ||
|
||
const patternColumn = columns[columns.length - 1]; | ||
const groupedPatterns = useGroupedPatterns({ | ||
config, | ||
samples: 10_000, | ||
bodyValueExpression: patternColumn ?? '', | ||
totalCount: undefined, | ||
enabled: denoiseResults, | ||
}); | ||
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. Just a comment for later: If we notice performance issues here, we could move this to a webworker 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. there's a few other issues we likely want to fix, I think it re-imports packages at times for some reason but I'm not too worried since none of these behaviors come around unless the user is activating the denoise |
||
const noisyPatterns = useQuery({ | ||
queryKey: ['noisy-patterns', config], | ||
queryFn: async () => { | ||
return Object.values(groupedPatterns.data).filter( | ||
p => p.count / (groupedPatterns.sampledRowCount ?? 1) > 0.1, | ||
); | ||
}, | ||
enabled: | ||
denoiseResults && | ||
groupedPatterns.data != null && | ||
Object.values(groupedPatterns.data).length > 0 && | ||
groupedPatterns.miner != null, | ||
}); | ||
const noisyPatternIds = useMemo(() => { | ||
return noisyPatterns.data?.map(p => p.id) ?? []; | ||
}, [noisyPatterns.data]); | ||
|
||
const queryClient = useQueryClient(); | ||
|
||
const denoisedRows = useQuery({ | ||
queryKey: [ | ||
'denoised-rows', | ||
config, | ||
processedRows, | ||
noisyPatternIds, | ||
patternColumn, | ||
], | ||
queryFn: async () => { | ||
// No noisy patterns, so no need to denoise | ||
if (noisyPatternIds.length === 0) { | ||
return processedRows; | ||
} | ||
|
||
const matchedLogs = await groupedPatterns.miner?.matchLogs( | ||
processedRows.map(row => row[patternColumn]), | ||
); | ||
return processedRows.filter((row, i) => { | ||
const match = matchedLogs?.[i]; | ||
return !noisyPatternIds.includes(`${match}`); | ||
}); | ||
}, | ||
placeholderData: (previousData, previousQuery) => { | ||
// If it's the same search, but new data, return the previous data while we load | ||
if ( | ||
previousQuery?.queryKey?.[0] === 'denoised-rows' && | ||
previousQuery?.queryKey?.[1] === config | ||
) { | ||
return previousData; | ||
} | ||
return undefined; | ||
}, | ||
enabled: | ||
denoiseResults && | ||
noisyPatterns.isSuccess && | ||
processedRows.length > 0 && | ||
groupedPatterns.miner != null, | ||
}); | ||
|
||
const isLoading = denoiseResults | ||
? isFetching || | ||
denoisedRows.isFetching || | ||
noisyPatterns.isFetching || | ||
groupedPatterns.isLoading | ||
: isFetching; | ||
|
||
return ( | ||
<RawLogTable | ||
isLive={isLive} | ||
wrapLines={false} | ||
displayedColumns={columns} | ||
highlightedLineId={highlightedLineId} | ||
rows={processedRows} | ||
isLoading={isFetching} | ||
fetchNextPage={fetchNextPage} | ||
// onPropertySearchClick={onPropertySearchClick} | ||
hasNextPage={hasNextPage} | ||
onRowExpandClick={_onRowExpandClick} | ||
onScroll={onScroll} | ||
generateRowId={getRowWhere} | ||
isError={isError} | ||
error={error ?? undefined} | ||
columnTypeMap={columnMap} | ||
/> | ||
<> | ||
{denoiseResults && ( | ||
<Box mb="xxs" px="sm" mt="-24px"> | ||
<Text fw="bold" fz="xs" mb="xxs"> | ||
Removed Noisy Event Patterns | ||
</Text> | ||
<Box mah={100} style={{ overflow: 'auto' }}> | ||
{noisyPatterns.data?.map(p => ( | ||
<Text c="gray.3" fz="xs" key={p.id}> | ||
{p.pattern} | ||
</Text> | ||
))} | ||
{noisyPatternIds.length === 0 && ( | ||
<Text c="gray.3" fz="xs"> | ||
No noisy patterns found | ||
</Text> | ||
)} | ||
</Box> | ||
</Box> | ||
)} | ||
<RawLogTable | ||
isLive={isLive} | ||
wrapLines={false} | ||
displayedColumns={columns} | ||
highlightedLineId={highlightedLineId} | ||
rows={denoiseResults ? (denoisedRows?.data ?? []) : processedRows} | ||
isLoading={isLoading} | ||
fetchNextPage={fetchNextPage} | ||
// onPropertySearchClick={onPropertySearchClick} | ||
hasNextPage={hasNextPage} | ||
onRowExpandClick={_onRowExpandClick} | ||
onScroll={onScroll} | ||
generateRowId={getRowWhere} | ||
isError={isError} | ||
error={error ?? undefined} | ||
columnTypeMap={columnMap} | ||
/> | ||
</> | ||
); | ||
} |
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.
This just an optimization or does it fix a bug?
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.
good q, I don't actually recall why this diff is here since the commit was a while back, at best vaguely recalling it's likely trying to fix an excess re-render issue.