fix(decoder): Auto-detect timestamp resolution in JSONL decoder #405
Conversation
…coder Automatically detect and convert timestamps in seconds, milliseconds, microseconds, and nanoseconds to milliseconds for dayjs parsing. Previously, only millisecond timestamps were handled correctly.
WalkthroughThe change updates a timestamp parsing utility to auto-detect numeric timestamp resolution (seconds, milliseconds, microseconds, nanoseconds), convert values to milliseconds (with bigint-aware handling), and then call dayjs.utc; existing INVALID_TIMESTAMP_VALUE handling and validity checks remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/services/decoders/JsonlDecoder/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
src/services/decoders/JsonlDecoder/utils.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts:24-36
Timestamp: 2024-11-18T01:34:54.885Z
Learning: In `src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts`, the `convertToDayjsTimestamp` function already includes error handling for invalid inputs, so wrapping it in try-catch in the `TimestampFormatter.formatField` method is unnecessary.
📚 Learning: 2024-11-18T01:34:54.885Z
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts:24-36
Timestamp: 2024-11-18T01:34:54.885Z
Learning: In `src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts`, the `convertToDayjsTimestamp` function already includes error handling for invalid inputs, so wrapping it in try-catch in the `TimestampFormatter.formatField` method is unnecessary.
Applied to files:
src/services/decoders/JsonlDecoder/utils.ts
🪛 GitHub Actions: lint
src/services/decoders/JsonlDecoder/utils.ts
[error] 76-76: ESLint: 'multiline-ternary' rule violation. Expected newline between test and consequent of ternary.
🪛 GitHub Check: lint-check
src/services/decoders/JsonlDecoder/utils.ts
[failure] 91-91:
No magic number: 1000
[failure] 89-89:
No magic number: 1e17
[failure] 89-89:
Expected literal to be on the left side of <
[failure] 86-86:
No magic number: 1e14
[failure] 86-86:
Expected literal to be on the left side of <
[failure] 85-85:
No magic number: 1000
[failure] 83-83:
No magic number: 1e11
[failure] 83-83:
Expected literal to be on the left side of <
[failure] 76-76:
Expected newline between consequent and alternate of ternary expression
[failure] 76-76:
Expected newline between test and consequent of ternary expression
🔇 Additional comments (2)
src/services/decoders/JsonlDecoder/utils.ts (2)
83-95: Consider handling negative timestamps explicitly.The current logic treats all values
< 1e11as seconds, including negative timestamps. Negative Unix timestamps are valid (representing dates before 1970-01-01). However, large negative values (e.g.,-1e15) would incorrectly be treated as seconds rather than microseconds or nanoseconds.Please verify whether negative timestamps are expected in your log data and how they should be handled. If negative timestamps in microseconds or nanoseconds are possible, the magnitude-based detection should use absolute values:
const absValue = Math.abs(numValue); if (absValue < 1e11) { // Seconds -> convert to milliseconds timestampInMs = numValue * 1000; } else if (absValue < 1e14) { // Milliseconds -> use as-is timestampInMs = numValue; } else if (absValue < 1e17) { // Microseconds -> convert to milliseconds timestampInMs = numValue / 1000; } else { // Nanoseconds -> convert to milliseconds timestampInMs = numValue / 1e6; }
98-99: Verify the timestamp conversion logic with test cases.The change correctly passes the normalized
timestampInMstodayjs.utc(). To ensure the auto-detection logic works as expected across all timestamp resolutions, please confirm that comprehensive test cases cover:
- Seconds: e.g.,
1679711026(2023-03-25)- Milliseconds: e.g.,
1679711026000- Microseconds: e.g.,
1679711026000000- Nanoseconds: e.g.,
1679711026000000000- Edge cases: boundary values at each threshold (1e11, 1e14, 1e17)
- Negative timestamps
- String timestamps
- Extract magic numbers into named constants for better maintainability - Fix precision loss for bigint nanosecond timestamps by performing division in bigint space before converting to number - Add negative timestamp validation to seconds threshold check - Improve code readability with multi-line ternary formatting
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/services/decoders/JsonlDecoder/utils.ts (1)
88-90: 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. This same issue was flagged in previous reviews but hasn't been addressed yet.Apply this diff to fix the formatting:
- const numValue = "bigint" === typeof field - ? Number(field) - : field; + const numValue = "bigint" === typeof field ? + Number(field) : + field;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/services/decoders/JsonlDecoder/utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
src/services/decoders/JsonlDecoder/utils.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: davemarco
Repo: y-scope/yscope-log-viewer PR: 123
File: src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts:24-36
Timestamp: 2024-11-18T01:34:54.885Z
Learning: In `src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts`, the `convertToDayjsTimestamp` function already includes error handling for invalid inputs, so wrapping it in try-catch in the `TimestampFormatter.formatField` method is unnecessary.
📚 Learning: 2024-11-18T01:34:54.885Z
Learnt from: davemarco
Repo: y-scope/yscope-log-viewer PR: 123
File: src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts:24-36
Timestamp: 2024-11-18T01:34:54.885Z
Learning: In `src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts`, the `convertToDayjsTimestamp` function already includes error handling for invalid inputs, so wrapping it in try-catch in the `TimestampFormatter.formatField` method is unnecessary.
Applied to files:
src/services/decoders/JsonlDecoder/utils.ts
📚 Learning: 2024-11-18T01:35:37.012Z
Learnt from: davemarco
Repo: y-scope/yscope-log-viewer PR: 123
File: src/services/decoders/ClpIrDecoder.ts:90-90
Timestamp: 2024-11-18T01:35:37.012Z
Learning: Within `ClpIrDecoder.ts`, in the `setFormatterOptions` method, `options.formatString` is already typed as a string, so explicit casting to string is unnecessary.
Applied to files:
src/services/decoders/JsonlDecoder/utils.ts
🪛 GitHub Actions: lint
src/services/decoders/JsonlDecoder/utils.ts
[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
src/services/decoders/JsonlDecoder/utils.ts
[failure] 111-111:
':' should be placed at the end of the line
[failure] 110-110:
'?' should be placed at the end of the line
[failure] 97-97:
Expected literal to be on the left side of >=
[failure] 90-90:
':' should be placed at the end of the line
[failure] 89-89:
'?' should be placed at the end of the line
🔇 Additional comments (2)
src/services/decoders/JsonlDecoder/utils.ts (2)
13-23: Good addition of named constants for timestamp thresholds and conversion factors.These constants improve maintainability and make the detection logic self-documenting, addressing the feedback from previous reviews about extracting magic numbers.
84-116: Approve the auto-detection approach with noted issues to address.The magnitude-based timestamp resolution detection is a sound approach that improves compatibility with diverse log formats. The logic correctly handles the common timestamp units and maintains backward compatibility with string timestamps.
However, please address the critical ESLint violations and major issues flagged in the other comments before merging.
| const numValue = "bigint" === typeof field | ||
| ? Number(field) | ||
| : field; |
There was a problem hiding this comment.
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.
| if (numValue < TIMESTAMP_THRESHOLD_SECONDS && numValue >= 0) { | ||
| // Seconds -> convert to milliseconds | ||
| timestampInMs = numValue * MILLISECONDS_PER_SECOND; |
There was a problem hiding this comment.
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.
| timestampInMs = "bigint" === typeof field | ||
| ? Number(field / NANOSECONDS_PER_MILLISECOND) | ||
| : numValue / MILLISECONDS_PER_NANOSECOND; |
There was a problem hiding this comment.
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.
1 participant
Description
This PR enhances the JSONL decoder’s
convertToDayjsTimestampfunction by adding automatic timestamp resolution detection. Previously, the function assumed millisecond precision, leading to incorrect parsing for logs using seconds, microseconds, or nanoseconds.The updated implementation infers resolution based on magnitude:
This ensures
dayjs.utc()receives a properly scaled millisecond input, improving compatibility across diverse log formats.Checklist
Validation
Before the fix:
{timestamp:timestamp}rendered incorrectly as1970-01-01T00:00:00ZAfter the fix:
{timestamp:timestamp}correctly renders as2023-03-25T02:43:46ZSummary by CodeRabbit