-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(logs): Split flex time page params from existing page params #101176
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
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 |
---|---|---|
|
@@ -2,6 +2,7 @@ import {useCallback, useEffect, useMemo} from 'react'; | |
import {logger} from '@sentry/react'; | ||
|
||
import {type ApiResult} from 'sentry/api'; | ||
import {defined} from 'sentry/utils'; | ||
import {encodeSort, type EventsMetaType} from 'sentry/utils/discover/eventView'; | ||
import type {Sort} from 'sentry/utils/discover/fields'; | ||
import {DiscoverDatasets} from 'sentry/utils/discover/types'; | ||
|
@@ -285,12 +286,43 @@ function getPageParam( | |
pageParam: LogPageParam | ||
): LogPageParam => { | ||
const [pageData, _statusText, response] = result; | ||
|
||
const sortBy = getTimeBasedSortBy(sortBys); | ||
|
||
if (!sortBy) { | ||
// Only sort by timestamp precise is supported for infinite queries. | ||
return null; | ||
} | ||
|
||
const isDescending = sortBy.kind === 'desc'; | ||
let querySortDirection: Sort | undefined = undefined; | ||
const reverseSortDirection = isDescending ? 'asc' : 'desc'; | ||
|
||
if (isGetPreviousPage) { | ||
// Previous pages have to have the sort order reversed in order to start at the limit from the initial page. | ||
querySortDirection = { | ||
field: OurLogKnownFieldKey.TIMESTAMP, | ||
kind: reverseSortDirection, | ||
}; | ||
} | ||
|
||
if (highFidelity || isFlexTimePageParam(pageParam)) { | ||
const pageLinkHeader = response?.getResponseHeader('Link') ?? null; | ||
const links = parseLinkHeader(pageLinkHeader); | ||
const link = isGetPreviousPage ? links.previous : links.next; | ||
if (!link?.results) { | ||
return undefined; | ||
} | ||
|
||
const pageParamResult: FlexTimePageParam = { | ||
querySortDirection, | ||
sortByDirection: sortBy.kind, | ||
autoRefresh, | ||
cursor: link.cursor ?? undefined, | ||
}; | ||
|
||
return pageParamResult; | ||
} | ||
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: Flexible Pagination Overlaps With Cursor-BasedThe |
||
|
||
const firstRow = pageData.data?.[0]; | ||
const lastRow = pageData.data?.[pageData.data.length - 1]; | ||
if (!firstRow || !lastRow) { | ||
|
@@ -319,45 +351,19 @@ function getPageParam( | |
const logId = isGetPreviousPage | ||
? firstRow[OurLogKnownFieldKey.ID] | ||
: lastRow[OurLogKnownFieldKey.ID]; | ||
const isDescending = sortBy.kind === 'desc'; | ||
const timestampPrecise = isGetPreviousPage ? firstTimestamp : lastTimestamp; | ||
let querySortDirection: Sort | undefined = undefined; | ||
const reverseSortDirection = isDescending ? 'asc' : 'desc'; | ||
|
||
if (isGetPreviousPage) { | ||
// Previous pages have to have the sort order reversed in order to start at the limit from the initial page. | ||
querySortDirection = { | ||
field: OurLogKnownFieldKey.TIMESTAMP, | ||
kind: reverseSortDirection, | ||
}; | ||
} | ||
|
||
const indexFromInitialPage = isGetPreviousPage | ||
? (pageParam?.indexFromInitialPage ?? 0) - 1 | ||
: (pageParam?.indexFromInitialPage ?? 0) + 1; | ||
|
||
let cursor: PageParam['cursor'] = undefined; | ||
if (isDescending && !autoRefresh && highFidelity) { | ||
const pageLinkHeader = response?.getResponseHeader('Link') ?? null; | ||
const links = parseLinkHeader(pageLinkHeader); | ||
// the flex time sampling strategy has no previous page cursor and only | ||
// a next page cursor because it only works in timestamp desc order | ||
const link = isGetPreviousPage ? links.previous : links.next; | ||
// return `undefined` to indicate that there are no results in this direction | ||
if (!link?.results) { | ||
return undefined; | ||
} | ||
cursor = link.cursor ?? undefined; | ||
} | ||
|
||
const pageParamResult: LogPageParam = { | ||
const pageParamResult: InfiniteScrollPageParam = { | ||
logId, | ||
timestampPrecise, | ||
querySortDirection, | ||
sortByDirection: sortBy.kind, | ||
indexFromInitialPage, | ||
autoRefresh, | ||
cursor, | ||
}; | ||
|
||
return pageParamResult; | ||
|
@@ -417,7 +423,7 @@ function getParamBasedQuery( | |
return query; | ||
} | ||
|
||
if (pageParam.cursor) { | ||
if (isFlexTimePageParam(pageParam)) { | ||
return { | ||
...query, | ||
cursor: pageParam.cursor, | ||
|
@@ -448,22 +454,32 @@ function getParamBasedQuery( | |
}; | ||
} | ||
|
||
interface PageParam { | ||
interface BaseLogsPageParams { | ||
// Whether the page param is for auto refresh mode. | ||
autoRefresh: boolean; | ||
// The original sort direction of the query. | ||
sortByDirection: Sort['kind']; | ||
// When scrolling is happening towards current time, or during auto refresh, we flip the sort direction passed to the query to get X more rows in the future starting from the last seen row. | ||
querySortDirection?: Sort; | ||
} | ||
|
||
interface FlexTimePageParam extends BaseLogsPageParams { | ||
cursor?: string; | ||
} | ||
|
||
interface InfiniteScrollPageParam extends BaseLogsPageParams { | ||
// The index of the page from the initial page. Useful for debugging and testing. | ||
indexFromInitialPage: number; | ||
// The id of the log row matching timestampPrecise. We use this to exclude the row from the query to avoid duplicates right on the nanosecond boundary. | ||
logId: string; | ||
// The original sort direction of the query. | ||
sortByDirection: Sort['kind']; | ||
timestampPrecise: bigint | null; | ||
cursor?: string; | ||
// When scrolling is happening towards current time, or during auto refresh, we flip the sort direction passed to the query to get X more rows in the future starting from the last seen row. | ||
querySortDirection?: Sort; | ||
} | ||
|
||
export type LogPageParam = PageParam | null | undefined; | ||
export type LogPageParam = FlexTimePageParam | InfiniteScrollPageParam | null | undefined; | ||
|
||
function isFlexTimePageParam(pageParam: LogPageParam): pageParam is FlexTimePageParam { | ||
return defined(pageParam) && 'cursor' in pageParam; | ||
} | ||
|
||
type QueryKey = [url: string, endpointOptions: QueryKeyEndpointOptions, 'infinite']; | ||
|
||
|
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.
Potential bug: Backward pagination is broken in 'flex time' mode due to incorrect handling of forward-only logic.
Description: In 'flex time' mode, which is strictly forward-only, the code attempts to handle previous page requests. This causes the pagination logic to return
undefined
because the API correctly reports no previous page is available, leading to broken backward pagination functionality.Suggested fix: The code should explicitly handle the case where a previous page is requested in 'flex time' mode, either by preventing the request or by providing a clear state to the UI, instead of returning
undefined
and causing a silent failure.severity: 3.0, confidence: 5.0
Did we get this right? 👍 / 👎 to inform future reviews.