fix: Prepend formatted timestamps to unstructured log events (fixes #201). #209
Conversation
WalkthroughThe pull request updates two components. First, it modifies the regular expression responsible for matching date strings in a custom log language in the Monaco editor to enhance flexibility, allowing for optional time components and timezone offsets. Second, it introduces a new static method in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This PR is submitted to unblock a formal release of KV-Pair IR logs. #188 will be merged later as an improvement / extension to the timestamp formatting ability in the current PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/decoders/ClpIrDecoder/index.ts (1)
88-96: Consider defensive coding practices for timestamp handlingThe current implementation directly modifies the input array and doesn't handle potential null or invalid timestamps. Consider adding validation checks:
static #formatUnstructuredResults = (results: DecodeResult[]): DecodeResult[] => { for (const r of results) { const [ message, timestamp, ] = r; + // Skip formatting if timestamp is invalid + if (timestamp === null || timestamp === undefined) { + continue; + } const dayJsTimestamp: Dayjs = convertToDayjsTimestamp(timestamp); r[0] = dayJsTimestamp.format("YYYY-MM-DDTHH:mm:ss.SSSZ") + message; } return results; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Editor/MonacoInstance/language.ts(1 hunks)src/services/decoders/ClpIrDecoder/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/components/Editor/MonacoInstance/language.tssrc/services/decoders/ClpIrDecoder/index.ts
🔇 Additional comments (3)
src/components/Editor/MonacoInstance/language.ts (1)
36-36: Consider supporting both timezone formats in the date regexThe updated regex now supports timestamps with timezone offsets (
[+-]HH:MM) but no longer supports UTC timestamps with the 'Z' suffix. This might cause regression issues if logs contain timestamps with 'Z' notation.Consider using a more inclusive pattern like:
-/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}[+-]\d{2}:\d{2}/ +/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}(?:[+-]\d{2}:\d{2}|Z)/This would support both timezone offset notation and the 'Z' suffix for UTC time.
src/services/decoders/ClpIrDecoder/index.ts (2)
82-99: New method effectively formats timestamps for unstructured logsThe implementation correctly prepends formatted timestamps to unstructured log messages, addressing the issue mentioned in the PR (#201).
152-152: The modified control flow effectively applies formatting to unstructured logsGood change - this ensures all unstructured log events are now formatted with timestamps, meeting the PR's objective.
Co-authored-by: kirkrodrigues <[email protected]>
… for structured logs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/decoders/ClpIrDecoder/index.ts (1)
82-99: New static method added to format unstructured log eventsThe implementation looks good overall. A few observations:
The method modifies the input array in-place rather than creating a copy. This is efficient but could cause issues if the caller expects the original array to remain unchanged.
There's no space between the timestamp and the message in line 95, which might affect readability. Consider adding a space:
- r[0] = dayJsTimestamp.format("YYYY-MM-DDTHH:mm:ss.SSSZ") + message; + r[0] = dayJsTimestamp.format("YYYY-MM-DDTHH:mm:ss.SSSZ") + " " + message;
- The timestamp format is hardcoded. In the future, you might want to make this configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/decoders/ClpIrDecoder/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/decoders/ClpIrDecoder/index.ts
🔇 Additional comments (3)
src/services/decoders/ClpIrDecoder/index.ts (3)
166-166: Code style suggestion for boolean comparisonAccording to the coding guidelines, prefer
false == <expression>rather than!<expression>.- if (false === isJsonObject(fields)) { + if (false === isJsonObject(fields)) {The code already follows the preferred style, so this is just confirming compliance with the coding guidelines.
141-141: Code style - boolean comparison follow coding guidelinesThe code is using the preferred style
null === resultsrather thanresults === null, which follows the coding guidelines for boolean comparisons.
152-152:❓ Verification inconclusive
Updated return statement to format unstructured logs
This change ensures unstructured log events now have timestamps prepended, which aligns with the PR objective.
However, there's no explicit check to verify that
this.#streamTypeisCLP_IR_STREAM_TYPE.UNSTRUCTUREDbefore applying the formatting. This block currently handles both the case where formatting is not set for structured logs (which triggers an error) and unstructured logs (which should be formatted).Consider making the intention clearer:
if (null === this.#formatter) { if (this.#streamType === CLP_IR_STREAM_TYPE.STRUCTURED) { // eslint-disable-next-line no-warning-comments // TODO: Revisit when we allow displaying structured logs without a formatter. console.error("Formatter is not set for structured logs."); + throw new Error("Formatter must be set for structured logs"); } - return ClpIrDecoder.#formatUnstructuredResults(results); + // For unstructured logs, prepend timestamps when no formatter is set + return ClpIrDecoder.#formatUnstructuredResults(results); }This change ensures that the code fails explicitly when a formatter is required but not provided. Without this change, the code would continue execution with a warning, potentially leading to unexpected behavior. Does this approach align with how other error conditions are handled in the codebase?
Clarify and Explicitly Distinguish Log Handling
The updated return statement to format unstructured logs is a step forward, as it prepends timestamps in line with the PR’s objective. However, there is still a potential ambiguity: the code relies on falling through the condition to assume the logs are unstructured when no formatter is set. It might be clearer to explicitly verify that the stream type is indeed unstructured before calling
ClpIrDecoder.#formatUnstructuredResults(results).A suggested refactor would be:
if (null === this.#formatter) { if (this.#streamType === CLP_IR_STREAM_TYPE.STRUCTURED) { // eslint-disable-next-line no-warning-comments // TODO: Revisit when we allow displaying structured logs without a formatter. console.error("Formatter is not set for structured logs."); + throw new Error("Formatter must be set for structured logs"); } else if (this.#streamType === CLP_IR_STREAM_TYPE.UNSTRUCTURED) { // For unstructured logs, prepend timestamps when no formatter is set. return ClpIrDecoder.#formatUnstructuredResults(results); } }This explicit branch ensures that:
- Structured logs: an error is thrown immediately when a required formatter is missing.
- Unstructured logs: the logs are formatted as intended.
Please verify that this approach aligns with how other error conditions are handled in the codebase, ensuring consistency across modules.
junhaoliao
changed the title
Description
Checklist
breaking change.
Validation performed
year,zero-padded month,zero-padded day,T,zero-padded 24-h hour,zero-padded minute,zero-padded secondandzero-padded 3-digit millisecondsparts matched. For the timezone,+00:00is shown to represent UTC, instead ofZor `` (not present) depending on the timestamp format string in the IR file's metadata.Summary by CodeRabbit