Skip to content

fix(decoder): Auto-detect timestamp resolution in JSONL decoder #405

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(decoder): Auto-detect timestamp resolution in JSONL decoder #405

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97a3d1c
fix(decoder): Add auto-detection for timestamp resolution in JSONL de…
jackluo923 Oct 30, 2025
92a67f0
fix(decoder): Address code review feedback on timestamp auto-detection
jackluo923 Oct 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion src/services/decoders/JsonlDecoder/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@
} from "../../../typings/logs";


// Timestamp resolution detection thresholds
const TIMESTAMP_THRESHOLD_SECONDS = 1e11;
const TIMESTAMP_THRESHOLD_MILLISECONDS = 1e14;
const TIMESTAMP_THRESHOLD_MICROSECONDS = 1e17;

// Conversion factors to milliseconds
const MILLISECONDS_PER_SECOND = 1000;
const MILLISECONDS_PER_MICROSECOND = 1000;
const MILLISECONDS_PER_NANOSECOND = 1e6;
const NANOSECONDS_PER_MILLISECOND = 1000000n;


/**
* Determines whether the given value is a `JsonObject` and applies a TypeScript narrowing
* conversion if so.
Expand Down Expand Up @@ -69,7 +81,39 @@
field = INVALID_TIMESTAMP_VALUE;
}

let dayjsTimestamp: Dayjs = dayjs.utc(field);
let timestampInMs: number | string = field;

// Auto-detect timestamp resolution and convert to milliseconds
if ("number" === typeof field || "bigint" === typeof field) {
const numValue = "bigint" === typeof field
? Number(field)

Check failure on line 89 in src/services/decoders/JsonlDecoder/utils.ts

View workflow job for this annotation

GitHub Actions / lint-check 

'?' should be placed at the end of the line
: field;

Check failure on line 90 in src/services/decoders/JsonlDecoder/utils.ts

View workflow job for this annotation

GitHub Actions / lint-check 

':' should be placed at the end of the line
Comment on lines +88 to +90
Copy link

@coderabbitai coderabbitai bot Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Risk of precision loss for large microsecond timestamps when converting bigint to Number.

Converting bigint to Number before the magnitude check causes precision loss for microsecond timestamps (1e14–1e17) that exceed Number.MAX_SAFE_INTEGER (≈9.007e15). The nanoseconds case correctly handles this by performing bigint division before conversion (lines 109-111), but the microseconds case doesn't.

Consider handling bigint for microseconds similarly to nanoseconds. Apply this approach:

         } else if (numValue < TIMESTAMP_THRESHOLD_MICROSECONDS) {
             // Microseconds -> convert to milliseconds
-            timestampInMs = numValue / MILLISECONDS_PER_MICROSECOND;
+            timestampInMs = "bigint" === typeof field ?
+                Number(field / 1000n) :
+                numValue / MILLISECONDS_PER_MICROSECOND;
         } else {
🧰 Tools
🪛 GitHub Actions: lint

[error] 89-89: ESLint: '?' should be placed at the end of the line. (operator-linebreak). Command: 'eslint . --max-warnings 0 --concurrency=auto'.

🪛 GitHub Check: lint-check

[failure] 90-90:
':' should be placed at the end of the line

[failure] 89-89:
'?' should be placed at the end of the line

🤖 Prompt for AI Agents 
In src/services/decoders/JsonlDecoder/utils.ts around lines 88 to 90, the code
converts a bigint microsecond timestamp to Number before checking magnitude,
risking precision loss for values > Number.MAX_SAFE_INTEGER; to fix, treat
bigint microsecond values the same as nanoseconds: perform bigint arithmetic
(e.g., divide by 1000n to get milliseconds or compare against BigInt thresholds)
while still in bigint form, then convert to Number only after reducing magnitude
or confirming it's within safe range, and ensure the subsequent magnitude checks
use the converted/adjusted numeric value.


// Detection based on timestamp magnitude:
// - Seconds: < 10^11 (covers dates up to year 5138)
// - Milliseconds: 10^11 <= t < 10^14
// - Microseconds: 10^14 <= t < 10^17
// - Nanoseconds: >= 10^17
if (numValue < TIMESTAMP_THRESHOLD_SECONDS && numValue >= 0) {

Check failure on line 97 in src/services/decoders/JsonlDecoder/utils.ts

View workflow job for this annotation

GitHub Actions / lint-check 

Expected literal to be on the left side of >=
// Seconds -> convert to milliseconds
timestampInMs = numValue * MILLISECONDS_PER_SECOND;
Comment on lines +97 to +99
Copy link

@coderabbitai coderabbitai bot Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Negative timestamps in seconds are incorrectly handled.

The check numValue >= 0 at line 97 causes negative timestamps in seconds (e.g., -1000 representing a date before epoch) to skip the seconds-to-milliseconds conversion. They would fall through to the milliseconds case and be used as-is, which is incorrect.

For example, -1000 seconds (which represents 1000 seconds before epoch) would be treated as -1000 milliseconds (only 1 second before epoch).

Apply this diff to handle negative timestamps correctly:

-        if (numValue < TIMESTAMP_THRESHOLD_SECONDS && numValue >= 0) {
+        if (Math.abs(numValue) < TIMESTAMP_THRESHOLD_SECONDS) {
             // Seconds -> convert to milliseconds
             timestampInMs = numValue * MILLISECONDS_PER_SECOND;
🧰 Tools
🪛 GitHub Check: lint-check

[failure] 97-97:
Expected literal to be on the left side of >=

🤖 Prompt for AI Agents 
In src/services/decoders/JsonlDecoder/utils.ts around lines 97 to 99, the
current check prevents negative second-based timestamps from being converted to
milliseconds; change the condition to detect seconds by magnitude (e.g., use
Math.abs(numValue) < TIMESTAMP_THRESHOLD_SECONDS) instead of numValue >= 0 so
negative values like -1000 are multiplied by MILLISECONDS_PER_SECOND and
converted correctly; update the comment to reflect that both positive and
negative seconds are handled.

} else if (numValue < TIMESTAMP_THRESHOLD_MILLISECONDS) {
// Milliseconds -> use as-is
timestampInMs = numValue;
} else if (numValue < TIMESTAMP_THRESHOLD_MICROSECONDS) {
// Microseconds -> convert to milliseconds
timestampInMs = numValue / MILLISECONDS_PER_MICROSECOND;
} else {
// Nanoseconds -> convert to milliseconds
// For bigint, perform division before converting to avoid precision loss
timestampInMs = "bigint" === typeof field
? Number(field / NANOSECONDS_PER_MILLISECOND)

Check failure on line 110 in src/services/decoders/JsonlDecoder/utils.ts

View workflow job for this annotation

GitHub Actions / lint-check 

'?' should be placed at the end of the line
: numValue / MILLISECONDS_PER_NANOSECOND;

Check failure on line 111 in src/services/decoders/JsonlDecoder/utils.ts

View workflow job for this annotation

GitHub Actions / lint-check 

':' should be placed at the end of the line
Comment on lines +109 to +111
Copy link

@coderabbitai coderabbitai bot Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix the multiline ternary operator formatting to resolve ESLint violation.

The pipeline is failing because the ? and : operators should be placed at the end of the line, not at the beginning.

Apply this diff to fix the formatting:

-            timestampInMs = "bigint" === typeof field
-                ? Number(field / NANOSECONDS_PER_MILLISECOND)
-                : numValue / MILLISECONDS_PER_NANOSECOND;
+            timestampInMs = "bigint" === typeof field ?
+                Number(field / NANOSECONDS_PER_MILLISECOND) :
+                numValue / MILLISECONDS_PER_NANOSECOND;
🧰 Tools
🪛 GitHub Check: lint-check

[failure] 111-111:
':' should be placed at the end of the line

[failure] 110-110:
'?' should be placed at the end of the line

🤖 Prompt for AI Agents 
In src/services/decoders/JsonlDecoder/utils.ts around lines 109 to 111, the
multiline ternary is formatted with the `?` and `:` at the start of lines
causing an ESLint violation; rewrite the assignment so the `?` appears at the
end of the condition line and the `:` at the end of the true-expression line
(i.e., put `?` after `"bigint" === typeof field` and `:` after the Number(...)
expression) so the ternary spans three lines with operators at line ends.

}
}

// dayjs.utc() expects millisecond input
let dayjsTimestamp: Dayjs = dayjs.utc(timestampInMs);

// Sanitize invalid (e.g., "deadbeef") timestamps to `INVALID_TIMESTAMP_VALUE`; otherwise
// they'll show up in UI as "Invalid Date".
Expand Down
Loading