fix: add fallback to numeric comparator to respect natural sorting

jgbernalp · jgbernalp · commit 13706d2afcf7 · 2026-05-21T13:52:45.000+02:00
Signed-off-by: Gabriel Bernal &lt;gbernal@redhat.com&gt;
diff --git a/web/cypress/e2e/integration/logs-sorting.cy.ts 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/src/__tests__/numeric-comparator.spec.ts b/web/src/__tests__/numeric-comparator.spec.ts
@@ -0,0 +1,72 @@
+import { 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),
+    );
+    // Direction multiplier is applied to fallback, so order is reversed
+    expect(sortedDesc.map((l) => l.logIndex)).toEqual([2, 1, 0]);
+  });
+});
diff --git a/web/src/__tests__/observed-timestamp-difference.spec.ts b/web/src/__tests__/observed-timestamp-difference.spec.ts
@@ -0,0 +1,58 @@
+import { observedTimestampDifference } from '../sort-utils';
+
+describe('observedTimestampDifference', () => {
+  it('should return the numeric difference for small bigints', () => {
+    expect(observedTimestampDifference(10n, 3n)).toBe(7);
+    expect(observedTimestampDifference(3n, 10n)).toBe(-7);
+  });
+
+  it('should return 0 when both values are equal', () => {
+    expect(observedTimestampDifference(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(observedTimestampDifference(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(observedTimestampDifference(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(observedTimestampDifference(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(observedTimestampDifference(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(observedTimestampDifference(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(observedTimestampDifference(pastMin, 0n)).toBe(Number.MIN_SAFE_INTEGER);
+  });
+
+  it('should handle realistic nanosecond timestamp differences', () => {
+    const ts1 = 1679000000000000000n;
+    const ts2 = 1679000000000000001n;
+    expect(observedTimestampDifference(ts2, ts1)).toBe(1);
+    expect(observedTimestampDifference(ts1, ts2)).toBe(-1);
+  });
+
+  it('should clamp when nanosecond timestamps are far apart', () => {
+    const ts1 = 1679000000000000000n;
+    const ts2 = 1579000000000000000n;
+    expect(observedTimestampDifference(ts1, ts2)).toBe(Number.MAX_SAFE_INTEGER);
+    expect(observedTimestampDifference(ts2, ts1)).toBe(Number.MIN_SAFE_INTEGER);
+  });
+});
diff --git 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, observedTimestampDifference } 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<Resource>;
-
 const isJSONObject = (value: string): boolean => {
   const trimmedValue = value.trim();
 
@@ -64,7 +63,9 @@ const streamToTableData = (stream: StreamLogData, timezone?: string): Array<LogT
   return values.map((value) => {
     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 = BigInt(stream.stream.observedTimestamp ?? rawTimestamp);
     const time = timestamp / 1e6;
     const formattedTime = dateToFormat(time, DateFormat.Full, timezone);
     const severity = parseName(stream.stream, ResourceLabel.Severity);
@@ -81,6 +82,7 @@ const streamToTableData = (stream: StreamLogData, timezone?: string): Array<LogT
       namespace,
       podName,
       type: 'log',
+      observedTimestamp,
       // index is 0 here to match the type, but it will be recalculated when flattening the array
       logIndex: 0,
     };
@@ -108,13 +110,6 @@ const getSeverityClass = (severity: string) => {
   return severity ? `lv-plugin__table__severity-${severity}` : '';
 };
 
-// sort with an appropriate numeric comparator for big floats
-const numericComparator = <T extends TableCellValue>(
-  a: T,
-  b: T,
-  directionMultiplier: number,
-): number => (a < b ? -1 : a > b ? 1 : 0) * directionMultiplier;
-
 const columns: Array<TableColumn<LogTableData>> = [
   {
     id: 'expand',
@@ -131,7 +126,12 @@ const columns: Array<TableColumn<LogTableData>> = [
     },
     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,
+          observedTimestampDifference(a.observedTimestamp, b.observedTimestamp),
+        ),
       ),
   },
   {
@@ -338,8 +338,15 @@ export const LogsTable: FC<PropsWithChildren<LogsTableProps>> = ({
       }
     }
 
-    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,
+        observedTimestampDifference(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
@@ -119,4 +119,5 @@ export type LogTableData = {
   data: Record<string, string>;
   type: 'log' | 'expand';
   logIndex: number;
+  observedTimestamp: bigint;
 };
diff --git a/web/src/sort-utils.ts b/web/src/sort-utils.ts
@@ -0,0 +1,25 @@
+type SortableValue = string | number;
+
+// sort with an appropriate numeric comparator for big floats
+export const numericComparator = <T extends SortableValue>(
+  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 observedTimestampDifference = (a: bigint, b: bigint): number => {
+  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);
+};
