feat: Support custom timestamp formatting for Unstructured IR logs (fixes #201). #188
Conversation
## Walkthrough
This pull request introduces enhancements for timestamp formatting in log decoding and configuration management. In the `ClpIrDecoder` class, a new private property and method are added to format timestamps using the dayjs library. Configuration interfaces, enums, and default settings are updated to include the new `timestampFormatString` property. Additionally, a new form field for specifying the timestamp format is incorporated into the settings panel, and tests are adjusted to validate the new functionality.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------|
| `src/services/decoders/ClpIrDecoder/index.ts` | Added private property `#timestampFormatString` and new instance method `#formatUnstructuredResults` to format timestamps using dayjs. Updated `decodeRange` to use the new instance method instead of the removed static method. |
| `src/typings/config.ts`, `src/typings/decoders.ts` | Added `DECODER_OPTIONS_TIMESTAMP_FORMAT_STRING` enum entry and `timestampFormatString` property in `DecoderOptions` interface. |
| `src/utils/config.ts`, `test/utils/config.test.ts` | Added `timestampFormatString` to default config, validation, local storage get/set operations, and updated tests to include validation error and storage assertion. |
| `src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx` | Added new form field for timestamp format string with helper text, initial value, unique local storage key, and updated form submission to include this property in configuration. |
## Possibly related PRs
- **y-scope/yscope-log-viewer#209:** Refactored `ClpIrDecoder` timestamp formatting method from static to instance and introduced configurable timestamp format string.
## Suggested reviewers
- kirkrodriguesWarning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
🧬 Code Graph Analysis (1)src/services/decoders/ClpIrDecoder/index.ts (3)
🔇 Additional comments (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/modals/SettingsModal/SettingsDialog.tsx (1)
79-85: Type inconsistency with other form fieldsThe type property is set to "string", while other similar form fields in this component use "text". Consider changing to "text" for consistency.
- type: "string", + type: "text",Additionally, consider enhancing the helper text with a link to Day.js documentation, similar to how the format string field includes documentation links.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/modals/SettingsModal/SettingsDialog.tsx(2 hunks)src/services/decoders/ClpIrDecoder/index.ts(5 hunks)src/typings/config.ts(1 hunks)src/typings/decoders.ts(1 hunks)src/utils/config.ts(4 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/typings/decoders.tssrc/typings/config.tssrc/utils/config.tssrc/components/modals/SettingsModal/SettingsDialog.tsxsrc/services/decoders/ClpIrDecoder/index.ts
🧠 Learnings (1)
src/services/decoders/ClpIrDecoder/index.ts (1)
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#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.
🔇 Additional comments (13)
src/typings/decoders.ts (1)
19-19: Looks good - Clean interface extension.This addition to the
DecoderOptionsinterface properly aligns with the existing properties and enables the new timestamp formatting functionality.src/typings/config.ts (1)
23-23: Enum addition follows consistent naming pattern.The new storage key follows the established pattern and properly uses template literals to combine the base key with the specific property name.
src/services/decoders/ClpIrDecoder/index.ts (4)
5-5: Appropriate import for the new functionality.The dayjs import is necessary for the new timestamp formatting feature.
40-40: Private field follows class style conventions.The private field declaration uses the modern private field syntax with the # prefix, consistent with other class members.
53-53: Property initialization is clean and straightforward.The timestamp format string is properly initialized from the decoder options.
147-147: Method call correctly added to return path.The formatResults method is now properly called when no formatter is set, ensuring consistent timestamp formatting.
src/utils/config.ts (4)
31-31: Good default timestamp format choice.The ISO 8601 format "YYYY-MM-DDTHH:mm:ssZ" is a sensible default for timestamp formatting.
55-56: Appropriate validation for required field.The validation ensures that the timestamp format string is not empty, which is important for proper functionality.
108-111: Storage implementation follows established pattern.The implementation for storing the timestamp format string in local storage follows the same pattern as other configuration properties.
154-156: Retrieval implementation follows established pattern.The implementation for retrieving the timestamp format string from local storage follows the same pattern as other configuration properties.
src/components/modals/SettingsModal/SettingsDialog.tsx (3)
123-125: Looks good - correct extraction of the new timestamp formatThe implementation correctly extracts the timestamp format string from the form data.
131-131: Looks good - proper configuration updateThe code correctly includes the new timestampFormatString in the configuration object.
138-138: Consider using Yoda condition styleBased on coding guidelines, prefer
false == <expression>rather than!<expression>. In this case, consider changingnull !== errortonull !== error.Oh wait, this is already using the preferred style! This code follows the guidelines correctly.
| formatResults (results: DecodeResult[]): Nullable<DecodeResult[]> { | ||
| for (const r of results) { | ||
| const [ | ||
| message, timestamp, | ||
| ] = r; | ||
|
|
||
| const formattedTimestamp = dayjs(timestamp).format(this.#timestampFormatString); | ||
| r[0] = formattedTimestamp + message; | ||
| } | ||
|
|
||
| return results; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with timestamp duplication in formatted messages.
The current implementation prepends the formatted timestamp to the message without checking if the message already includes a timestamp. This could lead to duplicate timestamps in the output.
Additionally, the method directly mutates the input array, which might cause unexpected side effects if the caller expects the original array to remain unchanged.
Consider this alternative implementation:
formatResults (results: DecodeResult[]): Nullable<DecodeResult[]> {
- for (const r of results) {
- const [
- message, timestamp,
- ] = r;
-
- const formattedTimestamp = dayjs(timestamp).format(this.#timestampFormatString);
- r[0] = formattedTimestamp + message;
- }
-
- return results;
+ // Create a shallow copy to avoid mutating the input array
+ const formattedResults = [...results];
+
+ for (const r of formattedResults) {
+ const [message, timestamp] = r;
+ const formattedTimestamp = dayjs(timestamp).format(this.#timestampFormatString);
+
+ // Replace the message with the timestamp-formatted message
+ r[0] = formattedTimestamp + ' ' + message;
+ }
+
+ return formattedResults;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| formatResults (results: DecodeResult[]): Nullable<DecodeResult[]> { | |
| for (const r of results) { | |
| const [ | |
| message, timestamp, | |
| ] = r; | |
| const formattedTimestamp = dayjs(timestamp).format(this.#timestampFormatString); | |
| r[0] = formattedTimestamp + message; | |
| } | |
| return results; | |
| } | |
| formatResults (results: DecodeResult[]): Nullable<DecodeResult[]> { | |
| // Create a shallow copy to avoid mutating the input array | |
| const formattedResults = [...results]; | |
| for (const r of formattedResults) { | |
| const [message, timestamp] = r; | |
| const formattedTimestamp = dayjs(timestamp).format(this.#timestampFormatString); | |
| // Replace the message with the timestamp-formatted message | |
| r[0] = formattedTimestamp + ' ' + message; | |
| } | |
| return formattedResults; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utils/config.test.ts (1)
154-158: Code style inconsistency with other assertionsThe formatting of this localStorage assertion spans multiple lines, while the previous assertions on lines 145-153 use a single line format. Consider keeping the same style for consistency.
- expect(localStorage.getItem( - LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_FORMAT_STRING - )).toBe( - VALID_DECODER_OPTIONS.timestampFormatString - ); + expect(localStorage.getItem(LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_FORMAT_STRING)).toBe( + VALID_DECODER_OPTIONS.timestampFormatString + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/utils/config.test.ts(3 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>.
test/utils/config.test.ts
🔇 Additional comments (2)
test/utils/config.test.ts (2)
30-30: LGTM: New configuration property added correctlyThe addition of the
timestampFormatStringproperty toVALID_DECODER_OPTIONSproperly supports the new feature for custom timestamp rendering using DayJS.
61-61: LGTM: Appropriate validation error message addedThe error message for empty timestamp format string is consistent with the validation pattern for other configuration properties.
There was a problem hiding this comment.
mostly just nit-picking
will be great if we can get some insights for the settings UI text from @LinZhihao-723 / @kirkrodrigues
| type: "text", | ||
| }, | ||
| { | ||
| helperText: "Day.js format string for timestamp", |
There was a problem hiding this comment.
how about
| helperText: "Day.js format string for timestamp", | |
| helperText: "[Unstructured-IR] Format string for timestamps in CLP unstructured IR logs, in Day.js format.", |
There was a problem hiding this comment.
@LinZhihao-723 / @kirkrodrigues any thoughts?
|
for the PR title, how about: I consider this as a breaking change because now we format the logs in the user browser's timezone, instead of UTC as previously done. Until #207 is implemented, there is no way to format timestamps in UTC. |
AVMatthews
changed the title
|
@AVMatthews let me know when this is ready for review. in case you haven't already, let's merge |
|
As the web development team discussed offline, we may want to separate the settings into different tabs. I created a feature request to track this: #215 and we can implement that in a separate PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/decoders/ClpIrDecoder/index.ts (1)
182-196: 🛠️ Refactor suggestionAddress input mutation and formatting spacing.
The current implementation has two concerns:
- Input mutation: The method directly modifies the input array, which could cause unexpected side effects if the caller expects the original array to remain unchanged.
- Missing separator: The formatted timestamp is concatenated directly with the message without a space separator, which may make the output less readable.
Consider this improved implementation:
#formatUnstructuredResults (logEvents: DecodeResult[]): Nullable<DecodeResult[]> { + // Create a shallow copy to avoid mutating the input array + const formattedResults = [...logEvents]; + - for (const r of logEvents) { + for (const r of formattedResults) { const [ message, timestamp, ] = r; const formattedTimestamp = convertToDayjsTimestamp(timestamp).format( this.#timestampFormatString ); - r[0] = formattedTimestamp + message; + r[0] = formattedTimestamp + " " + message; } - return logEvents; + return formattedResults; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx(2 hunks)src/services/decoders/ClpIrDecoder/index.ts(4 hunks)src/utils/config.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/decoders/ClpIrDecoder/index.tssrc/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsxsrc/utils/config.ts
🧬 Code Graph Analysis (2)
src/services/decoders/ClpIrDecoder/index.ts (3)
src/typings/decoders.ts (1)
DecodeResult(108-108)src/typings/common.ts (1)
Nullable(10-10)src/services/decoders/JsonlDecoder/utils.ts (1)
convertToDayjsTimestamp(81-81)
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx (3)
src/utils/config.ts (2)
getConfig(192-192)setConfig(195-195)src/typings/config.ts (2)
CONFIG_KEY(45-45)LOCAL_STORAGE_KEY(46-46)src/typings/common.ts (1)
Nullable(10-10)
🔇 Additional comments (6)
src/utils/config.ts (3)
30-30: Good default timestamp format string choice.The default format
"YYYY-MM-DDTHH:mm:ss.SSSZ"follows ISO 8601 standard with milliseconds and timezone, which is appropriate for log timestamps.
53-56: Validation logic is consistent and well-placed.The validation for
timestampFormatStringfollows the same pattern as other configuration validations and maintains alphabetical order in the validation checks.
104-107: Local storage operations follow established patterns.The implementation correctly handles persistence and retrieval of the timestamp format string, maintaining consistency with other decoder options.
Also applies to: 151-153
src/services/decoders/ClpIrDecoder/index.ts (1)
40-40: Good integration of timestamp format configuration.The private property is properly initialized from decoder options, following the established pattern for configuration injection.
Also applies to: 54-54
src/components/CentralContainer/Sidebar/SidebarTabs/SettingsTabPanel/index.tsx (2)
112-118: Well-structured form field addition.The new timestamp format string field follows the established pattern and includes helpful context in the helper text about Day.js format and its specific application to unstructured IR logs.
154-156: Proper integration with form submission logic.The form data extraction and configuration update correctly includes the new timestamp format string field, maintaining consistency with the existing configuration management pattern.
Also applies to: 163-163
|
For the PR title, how about removing the The new config option value, retrieved from a newly added key in |
Co-authored-by: Junhao Liao <[email protected]>
AVMatthews
changed the title
Description
Added a field to the settings dialog which allows users to enter a format for the timestamp using DayJS. Used the entered string to format the timestamp displayed in the message.
Checklist
breaking change.
Validation performed
Tested using different format strings and some sample logs to see that the timestamp was correctly formatted.
Summary by CodeRabbit