From 7d9d7fb59c864fbc9c5760bd94e23aad3c705866 Mon Sep 17 00:00:00 2001 From: Gabriel Bernal Date: Wed, 25 Mar 2026 13:36:06 +0100 Subject: [PATCH] fix: add fallback to numeric comparator to respect natural sorting Signed-off-by: Gabriel Bernal --- .../e2e/integration/logs-dev-page.cy.ts | 2 +- .../e2e/integration/logs-sorting.cy.ts | 118 ++++++++++++++++ web/package.json | 2 +- web/src/__tests__/numeric-comparator.spec.ts | 128 ++++++++++++++++++ web/src/components/logs-table.tsx | 43 ++++-- web/src/logs.types.ts | 2 + web/src/sort-utils.ts | 28 ++++ 7 files changed, 308 insertions(+), 15 deletions(-) create mode 100644 web/cypress/e2e/integration/logs-sorting.cy.ts create mode 100644 web/src/__tests__/numeric-comparator.spec.ts create mode 100644 web/src/sort-utils.ts diff --git a/web/cypress/e2e/integration/logs-dev-page.cy.ts b/web/cypress/e2e/integration/logs-dev-page.cy.ts index f5cc914ed..2f3325e4d 100644 --- a/web/cypress/e2e/integration/logs-dev-page.cy.ts +++ b/web/cypress/e2e/integration/logs-dev-page.cy.ts @@ -431,7 +431,7 @@ describe('Logs Dev Page', () => { ); }); - cy.byTestID(TestIds.ExecuteQueryButton).click(); + cy.byTestID(TestIds.ExecuteQueryButton).click({ force: true }); cy.byTestID(TestIds.LogsMetrics).should('exist'); cy.byTestID(TestIds.ToggleHistogramButton).should('be.disabled'); diff --git a/web/cypress/e2e/integration/logs-sorting.cy.ts b/web/cypress/e2e/integration/logs-sorting.cy.ts new file mode 100644 index 000000000..eed13e0b7 --- /dev/null +++ b/web/cypress/e2e/integration/logs-sorting.cy.ts @@ -0,0 +1,118 @@ +import { TestIds } from '../../../src/test-ids'; + +const LOGS_PAGE_URL = '/monitoring/logs'; +const QUERY_RANGE_STREAMS_URL_MATCH = + '/api/proxy/plugin/logging-view-plugin/backend/api/logs/v1/application/loki/api/v1/query_range?query=%7B*'; +const QUERY_RANGE_MATRIX_URL_MATCH = + '/api/proxy/plugin/logging-view-plugin/backend/api/logs/v1/application/loki/api/v1/query_range?query=sum*'; + +const sameTimestampStreamsResponse = () => { + const msTimestamp = BigInt(Date.now()); + const nanosTimestamp = msTimestamp * 1000000n; + const nanosString = String(nanosTimestamp); + + return { + status: 'success', + data: { + resultType: 'streams', + result: [ + { + stream: { + filename: '/var/log/out.log', + job: 'varlogs', + level: 'info', + observedTimestamp: String(nanosTimestamp + 200n), + }, + values: [[nanosString, 'log-line-B']], + }, + { + stream: { + filename: '/var/log/out.log', + job: 'varlogs', + level: 'info', + observedTimestamp: String(nanosTimestamp + 300n), + }, + values: [[nanosString, 'log-line-C']], + }, + { + stream: { + filename: '/var/log/out.log', + job: 'varlogs', + level: 'info', + observedTimestamp: String(nanosTimestamp + 100n), + }, + values: [[nanosString, 'log-line-A']], + }, + ], + stats: { + summary: { + bytesProcessedPerSecond: 0, + linesProcessedPerSecond: 0, + totalBytesProcessed: 0, + totalLinesProcessed: 0, + execTime: 0, + queueTime: 0, + subqueries: 0, + }, + querier: { + store: { + totalChunksRef: 0, + totalChunksDownloaded: 0, + chunksDownloadTime: 0, + chunk: { + headChunkBytes: 0, + headChunkLines: 0, + decompressedBytes: 0, + decompressedLines: 0, + compressedBytes: 0, + totalDuplicates: 0, + }, + }, + }, + ingester: { + totalReached: 0, + totalChunksMatched: 0, + totalBatches: 0, + totalLinesSent: 0, + store: { + totalChunksRef: 0, + totalChunksDownloaded: 0, + chunksDownloadTime: 0, + chunk: { + headChunkBytes: 0, + headChunkLines: 0, + decompressedBytes: 0, + decompressedLines: 0, + compressedBytes: 0, + totalDuplicates: 0, + }, + }, + }, + }, + }, + }; +}; + +describe('Logs Table Sorting', () => { + it('sorts same-timestamp logs by observedTimestamp', () => { + cy.intercept(QUERY_RANGE_STREAMS_URL_MATCH, sameTimestampStreamsResponse()).as( + 'queryRangeStreams', + ); + cy.intercept(QUERY_RANGE_MATRIX_URL_MATCH, { statusCode: 200, body: {} }); + + cy.visit(LOGS_PAGE_URL); + + cy.wait('@queryRangeStreams'); + + // Default direction is 'backward' (desc) — largest observedTimestamp first + // observedTimestamp order: C(+300) > B(+200) > A(+100) + cy.byTestID(TestIds.LogsTable) + .should('exist') + .within(() => { + cy.get('td[data-label="message"]').should('have.length', 3); + cy.get('td[data-label="message"]').eq(0).should('contain', 'log-line-C'); + cy.get('td[data-label="message"]').eq(1).should('contain', 'log-line-B'); + cy.get('td[data-label="message"]').eq(2).should('contain', 'log-line-A'); + }); + }); +}); diff --git a/web/package.json b/web/package.json index 76000c433..14f168d34 100644 --- a/web/package.json +++ b/web/package.json @@ -21,7 +21,7 @@ "lint:tsc": "tsc --noEmit", "cypress:open": "cypress open", "cypress:run": "cypress run", - "cypress:run:ci": "NO_COLOR=1 npx cypress run --spec cypress/e2e/integration/*.cy.ts --browser electron", + "cypress:run:ci": "NO_COLOR=1 cypress run --spec \"cypress/e2e/integration/*.cy.ts\" --browser electron", "test": "concurrently -n \"unit,e2e\" --kill-others-on-fail \"npm run test:unit\" \"npm run test:e2e\"", "test:unit": "TZ=UTC jest --config jest.config.js", "test:e2e": "./scripts/run-cypress.sh", diff --git a/web/src/__tests__/numeric-comparator.spec.ts b/web/src/__tests__/numeric-comparator.spec.ts new file mode 100644 index 000000000..c1e2e510c --- /dev/null +++ b/web/src/__tests__/numeric-comparator.spec.ts @@ -0,0 +1,128 @@ +import { bigIntDifference, numericComparator } from '../sort-utils'; + +describe('numericComparator', () => { + it('should return -1 when a < b with positive multiplier', () => { + expect(numericComparator(1, 2, 1)).toBe(-1); + }); + + it('should return 1 when a > b with positive multiplier', () => { + expect(numericComparator(2, 1, 1)).toBe(1); + }); + + it('should return 0 when a === b', () => { + expect(numericComparator(1, 1, 1)).toBe(0); + }); + + it('should invert result with negative multiplier (descending sort)', () => { + expect(numericComparator(1, 2, -1)).toBe(1); + expect(numericComparator(2, 1, -1)).toBe(-1); + }); + + it('should use fallback comparison when values are equal', () => { + expect(numericComparator(5, 5, 1, 3)).toBe(3); + expect(numericComparator(5, 5, 1, -2)).toBe(-2); + }); + + it('should apply direction multiplier to fallback comparison', () => { + expect(numericComparator(5, 5, -1, 3)).toBe(-3); + expect(numericComparator(5, 5, -1, -2)).toBe(2); + }); + + it('should ignore fallback when values are not equal', () => { + expect(numericComparator(1, 2, 1, 100)).toBe(-1); + expect(numericComparator(2, 1, 1, 100)).toBe(1); + }); + + it('should work with timestamps', () => { + const timestamp1 = 1679000000000; + const timestamp2 = 1679000000001; + expect(numericComparator(timestamp1, timestamp2, 1)).toBe(-1); + expect(numericComparator(timestamp2, timestamp1, 1)).toBe(1); + expect(numericComparator(timestamp1, timestamp1, 1)).toBe(0); + }); + + it('should use logIndex as tiebreaker for equal timestamps in ascending sort', () => { + const timestamp = 1679000000000; + const logs = [ + { timestamp, logIndex: 0 }, + { timestamp, logIndex: 1 }, + { timestamp, logIndex: 2 }, + ]; + + const sortedAsc = [...logs].sort((a, b) => + numericComparator(a.timestamp, b.timestamp, 1, a.logIndex - b.logIndex), + ); + expect(sortedAsc.map((l) => l.logIndex)).toEqual([0, 1, 2]); + }); + + it('should use logIndex as tiebreaker for equal timestamps in descending sort', () => { + const timestamp = 1679000000000; + const logs = [ + { timestamp, logIndex: 0 }, + { timestamp, logIndex: 1 }, + { timestamp, logIndex: 2 }, + ]; + + const sortedDesc = [...logs].sort((a, b) => + numericComparator(a.timestamp, b.timestamp, -1, a.logIndex - b.logIndex), + ); + expect(sortedDesc.map((l) => l.logIndex)).toEqual([2, 1, 0]); + }); +}); + +describe('observedTimestampDifference', () => { + it('should return the numeric difference for small bigints', () => { + expect(bigIntDifference(10n, 3n)).toBe(7); + expect(bigIntDifference(3n, 10n)).toBe(-7); + }); + + it('should return 0 when both values are equal', () => { + expect(bigIntDifference(42n, 42n)).toBe(0); + }); + + it('should clamp to MIN_SAFE_INTEGER when difference is too negative', () => { + const a = 0n; + const b = BigInt(Number.MAX_SAFE_INTEGER) + 1n; + expect(bigIntDifference(a, b)).toBe(Number.MIN_SAFE_INTEGER); + }); + + it('should clamp to MAX_SAFE_INTEGER when difference is too positive', () => { + const a = BigInt(Number.MAX_SAFE_INTEGER) + 1n; + const b = 0n; + expect(bigIntDifference(a, b)).toBe(Number.MAX_SAFE_INTEGER); + }); + + it('should not clamp when difference is exactly MAX_SAFE_INTEGER', () => { + const maxSafe = BigInt(Number.MAX_SAFE_INTEGER); + expect(bigIntDifference(maxSafe, 0n)).toBe(Number.MAX_SAFE_INTEGER); + }); + + it('should not clamp when difference is exactly MIN_SAFE_INTEGER', () => { + const minSafe = BigInt(Number.MIN_SAFE_INTEGER); + expect(bigIntDifference(minSafe, 0n)).toBe(Number.MIN_SAFE_INTEGER); + }); + + it('should clamp when difference is one past MAX_SAFE_INTEGER', () => { + const pastMax = BigInt(Number.MAX_SAFE_INTEGER) + 1n; + expect(bigIntDifference(pastMax, 0n)).toBe(Number.MAX_SAFE_INTEGER); + }); + + it('should clamp when difference is one past MIN_SAFE_INTEGER', () => { + const pastMin = BigInt(Number.MIN_SAFE_INTEGER) - 1n; + expect(bigIntDifference(pastMin, 0n)).toBe(Number.MIN_SAFE_INTEGER); + }); + + it('should handle realistic nanosecond timestamp differences', () => { + const ts1 = 1679000000000000000n; + const ts2 = 1679000000000000001n; + expect(bigIntDifference(ts2, ts1)).toBe(1); + expect(bigIntDifference(ts1, ts2)).toBe(-1); + }); + + it('should clamp when nanosecond timestamps are far apart', () => { + const ts1 = 1679000000000000000n; + const ts2 = 1579000000000000000n; + expect(bigIntDifference(ts1, ts2)).toBe(Number.MAX_SAFE_INTEGER); + expect(bigIntDifference(ts2, ts1)).toBe(Number.MIN_SAFE_INTEGER); + }); +}); diff --git a/web/src/components/logs-table.tsx b/web/src/components/logs-table.tsx index af400d99d..772120a52 100644 --- a/web/src/components/logs-table.tsx +++ b/web/src/components/logs-table.tsx @@ -27,6 +27,7 @@ import { } from '../logs.types'; import { parseName, parseResources, ResourceLabel } from '../parse-resources'; import { severityFromString } from '../severity'; +import { numericComparator, bigIntDifference } from '../sort-utils'; import { TestIds } from '../test-ids'; import { LogDetail } from './log-detail'; import './logs-table.css'; @@ -50,8 +51,6 @@ interface LogsTableProps { schema: Schema; } -type TableCellValue = string | number | Resource | Array; - const isJSONObject = (value: string): boolean => { const trimmedValue = value.trim(); @@ -64,7 +63,14 @@ const streamToTableData = (stream: StreamLogData, timezone?: string): Array { const logValue = String(value[1]); const message = isJSONObject(logValue) ? stream.stream['message'] || logValue : logValue; - const timestamp = parseFloat(String(value[0])); + const rawTimestamp = String(value[0]); + const timestamp = parseFloat(rawTimestamp); + const observedTimestamp = stream.stream.observedTimestamp + ? BigInt(stream.stream.observedTimestamp) + : undefined; + const openshiftSequence = stream.stream.openshift_sequence + ? BigInt(stream.stream.openshift_sequence) + : undefined; const time = timestamp / 1e6; const formattedTime = dateToFormat(time, DateFormat.Full, timezone); const severity = parseName(stream.stream, ResourceLabel.Severity); @@ -81,6 +87,8 @@ const streamToTableData = (stream: StreamLogData, timezone?: string): Array { return severity ? `lv-plugin__table__severity-${severity}` : ''; }; -// sort with an appropriate numeric comparator for big floats -const numericComparator = ( - a: T, - b: T, - directionMultiplier: number, -): number => (a < b ? -1 : a > b ? 1 : 0) * directionMultiplier; - const columns: Array> = [ { id: 'expand', @@ -131,7 +132,14 @@ const columns: Array> = [ }, sort: (data, sortDirection) => data.sort((a, b) => - numericComparator(a.timestamp, b.timestamp, sortDirection === 'asc' ? 1 : -1), + numericComparator( + a.timestamp, + b.timestamp, + sortDirection === 'asc' ? 1 : -1, + a.openshiftSequence !== undefined && b.openshiftSequence !== undefined + ? bigIntDifference(a.openshiftSequence, b.openshiftSequence) + : bigIntDifference(a.observedTimestamp, b.observedTimestamp), + ), ), }, { @@ -338,8 +346,17 @@ export const LogsTable: FC> = ({ } } - return dataCopy.sort((a, b) => numericComparator(a.timestamp, b.timestamp, -1)); - }, [tableData, sortBy]); + return dataCopy.sort((a, b) => + numericComparator( + a.timestamp, + b.timestamp, + direction === 'backward' ? -1 : 1, + a.openshiftSequence !== undefined && b.openshiftSequence !== undefined + ? bigIntDifference(a.openshiftSequence, b.openshiftSequence) + : bigIntDifference(a.observedTimestamp, b.observedTimestamp), + ), + ); + }, [tableData, sortBy, direction]); const dataIsEmpty = sortedData.length === 0; diff --git a/web/src/logs.types.ts b/web/src/logs.types.ts index 892ec631c..a4f1b412d 100644 --- a/web/src/logs.types.ts +++ b/web/src/logs.types.ts @@ -119,4 +119,6 @@ export type LogTableData = { data: Record; type: 'log' | 'expand'; logIndex: number; + observedTimestamp?: bigint; + openshiftSequence?: bigint; }; diff --git a/web/src/sort-utils.ts b/web/src/sort-utils.ts new file mode 100644 index 000000000..e96488db2 --- /dev/null +++ b/web/src/sort-utils.ts @@ -0,0 +1,28 @@ +type SortableValue = string | number; + +// sort with an appropriate numeric comparator for big floats +export const numericComparator = ( + a: T, + b: T, + directionMultiplier: number, + fallbackComparison?: number, +): number => { + const result = a < b ? -1 : a > b ? 1 : 0; + if (result === 0 && fallbackComparison !== undefined) { + return fallbackComparison * directionMultiplier; + } + return result * directionMultiplier; +}; + +const MIN_SAFE_BIGINT = BigInt(Number.MIN_SAFE_INTEGER); +const MAX_SAFE_BIGINT = BigInt(Number.MAX_SAFE_INTEGER); + +export const bigIntDifference = (a: bigint | undefined, b: bigint | undefined): number => { + if (a === undefined || b === undefined) { + return 0; + } + const difference = a - b; + if (difference < MIN_SAFE_BIGINT) return Number.MIN_SAFE_INTEGER; + if (difference > MAX_SAFE_BIGINT) return Number.MAX_SAFE_INTEGER; + return Number(difference); +};