new-log-viewer: Define interface for filtering by log level and add support (internally) to JsonlDecoder; Move timestamp field extraction from LogbackFormatter to JsonlDecoder. #79
Conversation
WalkthroughThe changes involve significant modifications to the log processing system, including the introduction of new methods for log level filtering, renaming of existing methods to clarify their purpose, and the addition of a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (14)
new-log-viewer/src/typings/logs.ts (1)
14-14: LGTM: New LogLevelFilter type aliasThe
LogLevelFiltertype alias is well-defined and aligns with the PR objective of supporting future log filtering. It allows for a nullable array of log levels, which is suitable for optional filtering scenarios.Consider adding a brief JSDoc comment to explain the purpose of this type alias, for example:
/** Represents an optional filter for log levels. Null means no filtering. */ type LogLevelFilter = Nullable<LOG_LEVEL[]>;new-log-viewer/src/typings/formatters.ts (1)
Line range hint
1-38: Summary: Changes align well with PR objectives and improve code structure.The modifications in this file, including the import change, simplification of
LogbackFormatterOptionsType, and updates to theFormatterinterface, all contribute to the PR's goal of restructuring the jsonl decoder. These changes should facilitate future log level filtering and streamline the log processing workflow.The removal of the
TimestampAndMessageTypeinterface and the shift towards usingJsonLogEventindicate a more structured approach to log event handling. This aligns with the PR's objective of relocating timestamp and log level parsing from the decoding phase to the building phase.Overall, these changes appear to be well-thought-out and consistent with the PR's objectives. However, it's crucial to ensure that these modifications don't negatively impact existing functionality. Please run the suggested verification scripts and thoroughly test the updated log processing pipeline to confirm that all components work as expected with these new type definitions.
new-log-viewer/src/services/decoders/ClpIrDecoder.ts (4)
37-42: LGTM: Placeholder method for future filtering functionality.The
getFilteredLogEventMapmethod is correctly implemented as a placeholder for future log level filtering. The TODO comment provides clear guidance for future development.Consider adding a JIRA ticket reference to the TODO comment for better traceability. For example:
// TODO: Update this after log level filtering is implemented in clp-ffi-js (JIRA-123)
44-49: LGTM: Placeholder method for setting log level filter.The
setLogLevelFiltermethod is correctly implemented as a placeholder for future log level filtering functionality. The TODO comment provides clear guidance for future development.Consider adding a JIRA ticket reference to the TODO comment for better traceability. For example:
// TODO: Update this after log level filtering is implemented in clp-ffi-js (JIRA-123)
59-61: LGTM: Method renamed to reflect its purpose.The renaming of
setDecoderOptionstosetFormatterOptionsis consistent with the restructuring objectives. The implementation remains unchanged, which is appropriate for this PR.Consider adding a TODO comment to indicate that this method might need further updates in the future. For example:
setFormatterOptions(): boolean { // TODO: Implement formatter options when they are defined (JIRA-XXX) return true; }
64-71: LGTM: Method updated for future filtering support.The renaming of
decodetodecodeRangeand the addition of theuseFilterparameter align well with the restructuring objectives. The implementation remains appropriate for this PR.Consider adding a TODO comment to indicate that the
useFilterparameter will be used in future implementations. For example:decodeRange( beginIdx: number, endIdx: number, // eslint-disable-next-line @typescript-eslint/no-unused-vars useFilter: boolean ): Nullable<DecodeResultType[]> { // TODO: Implement filtering logic when log level filtering is added (JIRA-XXX) return this.#streamReader.decodeRange(beginIdx, endIdx); }new-log-viewer/src/services/MainWorker.ts (2)
88-88: Approve the change and suggest a minor improvementThe modification from
setDecoderOptionstosetFormatterOptionsis consistent with the earlier change and aligns with the PR objectives.For improved code consistency, consider updating the variable name
decoderOptionstoformatterOptionsthroughout the file. This would make the code more self-explanatory and align with the new method name. Here's a suggested change:-if ("undefined" !== typeof args.decoderOptions) { - LOG_FILE_MANAGER.setFormatterOptions(args.decoderOptions); +if ("undefined" !== typeof args.formatterOptions) { + LOG_FILE_MANAGER.setFormatterOptions(args.formatterOptions);This change should be applied consistently throughout the file, including in the WORKER_REQ_CODE.EXPORT_LOG case.
Line range hint
1-114: Overall assessment of changes in MainWorker.tsThe modifications in this file are consistent and well-aligned with the PR objectives. The changes from
setDecoderOptionstosetFormatterOptionsreflect the restructuring of the jsonl decoder and the shift of parsing responsibilities. These focused updates maintain the existing functionality while preparing the groundwork for future enhancements in log filtering.As you continue to develop this feature, consider the following architectural advice:
- Ensure that the
LogFileManagerclass is updated to handle the new formatter options consistently across all its methods.- If not already done, create unit tests for the new formatting logic to ensure its correctness and prevent regressions.
- Consider adding documentation or comments explaining the new formatting process, especially if it differs significantly from the previous decoding process.
- As you implement the future log filtering capabilities, ensure that the
MainWorker.tsfile remains focused on coordination and delegation, with the core filtering logic residing in appropriate service classes.new-log-viewer/src/services/LogFileManager.ts (3)
76-79: Approved changes with a minor suggestion for error loggingThe update from
buildIdx()tobuild()aligns well with the PR objective of restructuring the jsonl decoder. The error handling has been appropriately updated to reflect this change.Consider enhancing the error logging by including more context:
- console.error("Invalid events found in decoder.buildIdx():", buildResult); + console.error(`Invalid events found in decoder.build(): ${buildResult.numInvalidEvents} out of ${this.#numEvents} total events`, buildResult);This change provides more immediate context about the scale of invalid events.
147-152: Approved changes with a suggestion for documentationThe renaming of
setDecoderOptionstosetFormatterOptionsis consistent with the PR objectives and the overall restructuring of the log processing system. This change clarifies that the method is specifically for setting formatting options.Consider updating the method documentation to reflect the focus on formatting:
/** - * Sets formatting options for the decoder. + * Sets options for formatting log events. * - * @param options + * @param options Formatting options to be applied to log events */This change provides more specific information about the purpose and impact of the method.
Line range hint
1-268: Overall changes align well with PR objectivesThe modifications in this file successfully restructure the jsonl decoder interface, shifting focus towards formatting options and preparing for future log filtering capabilities. The changes are consistent throughout the class and align well with the stated PR objectives.
As the groundwork for future log filtering is now in place, consider the following next steps:
- Document the new decoder interface thoroughly, especially the purpose of the new boolean parameter in
decodeRange.- Update any relevant test cases to cover the new decoder behaviour.
- Plan for the implementation of the log filtering methods mentioned in the PR objectives, ensuring they integrate smoothly with this restructured decoder.
These steps will help maintain code clarity and facilitate the smooth introduction of filtering capabilities in future updates.
new-log-viewer/src/typings/decoders.ts (2)
36-41: Consider renamingFilteredLogEventMapfor clarity.The type
FilteredLogEventMapis defined asNullable<number[]>, which is an array mapping filtered indices to unfiltered indices. Since it's an array, naming it withMapmight be misleading. Consider renaming it toFilteredLogEventIndicesor similar for clarity.
52-54: Enhance the documentation forgetFilteredLogEventMap().The
@returndescription is brief. Please provide more detail to clarify that the method returns a mapping from filtered log event indices to unfiltered log event indices.Apply this diff to enhance the comment:
* @return Indices of the filtered events. +* @return Mapping from filtered log event indices to unfiltered log event indices.new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1)
34-36: Consider default values forlogLevelKeyandtimestampKey.If these keys are standardised across most JSONL logs, providing default values can enhance usability by reducing the need for explicit specification each time.
Apply this diff to set default values:
#logLevelKey: string; #timestampKey: string; constructor (dataArray: Uint8Array, decoderOptions: JsonlDecoderOptionsType) { + const defaultLogLevelKey = "level"; + const defaultTimestampKey = "timestamp"; this.#dataArray = dataArray; - this.#logLevelKey = decoderOptions.logLevelKey; - this.#timestampKey = decoderOptions.timestampKey; + this.#logLevelKey = decoderOptions.logLevelKey || defaultLogLevelKey; + this.#timestampKey = decoderOptions.timestampKey || defaultTimestampKey; // Existing logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- new-log-viewer/src/services/LogFileManager.ts (4 hunks)
- new-log-viewer/src/services/MainWorker.ts (2 hunks)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder.ts (0 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts (1 hunks)
- new-log-viewer/src/services/formatters/LogbackFormatter.ts (2 hunks)
- new-log-viewer/src/typings/decoders.ts (3 hunks)
- new-log-viewer/src/typings/formatters.ts (2 hunks)
- new-log-viewer/src/typings/logs.ts (2 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- new-log-viewer/src/services/decoders/JsonlDecoder.ts
🔇 Additional comments (16)
new-log-viewer/src/typings/logs.ts (3)
1-1: LGTM: Import statement for Nullable typeThe import statement for the
Nullabletype from the localcommonmodule is appropriate and necessary for the newLogLevelFiltertype alias.
18-18: LGTM: Export of LogLevelFilter typeThe export of the
LogLevelFiltertype is appropriate and necessary for making this type available to other modules that will implement log filtering functionality.
Line range hint
1-21: Summary: Good foundation for log level filteringThe changes to this file provide a solid foundation for implementing log level filtering functionality. The new
LogLevelFiltertype, along with its import dependencies and export, are well-structured and align with the PR objectives. These modifications will enable other parts of the application to implement more efficient log processing and filtering in the future.new-log-viewer/src/typings/formatters.ts (2)
1-1: LGTM: Import change aligns with PR objectives.The change from importing
JsonObjecttoJsonLogEventis consistent with the PR's goal of restructuring the jsonl decoder. This modification suggests a move towards a more specific log event structure, which should facilitate future log level filtering.
32-32: Approved: Formatter interface update streamlines log processing.The changes to the
formatLogEventmethod signature in theFormatterinterface align well with the PR's objectives. UsingJsonLogEventas input and returning astringsimplifies the log formatting process and supports the restructuring of the jsonl decoder.To ensure these changes don't negatively impact downstream code, please run the following verification:
#!/bin/bash # Description: Check for uses of TimestampAndMessageType and formatLogEvent # Test 1: Search for uses of TimestampAndMessageType. Expect: No results or only in test files. echo "Checking for TimestampAndMessageType usage:" rg --type typescript 'TimestampAndMessageType' # Test 2: Search for uses of formatLogEvent. Expect: Updated method signatures. echo "Checking for formatLogEvent usage:" rg --type typescript 'formatLogEvent'new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2)
7-8: LGTM: Import statements are consistent with class changes.The new imports for
FilteredLogEventMap,LOG_EVENT_FILE_END_IDX, andLogLevelFilteralign well with the modifications made to theClpIrDecoderclass. These additions support the restructuring for future log filtering capabilities.Also applies to: 11-11
51-56: LGTM with a performance consideration.The
buildmethod has been updated to process all events up toLOG_EVENT_FILE_END_IDX, which aligns with the restructuring objectives. However, this change may have performance implications for large log files.Consider the performance impact of processing all events. You may want to add logging or metrics to monitor the execution time of this method. For example:
build(): LogEventCount { const startTime = performance.now(); const result = { numInvalidEvents: 0, numValidEvents: this.#streamReader.deserializeRange(0, LOG_EVENT_FILE_END_IDX), }; const endTime = performance.now(); console.log(`Build method execution time: ${endTime - startTime} milliseconds`); return result; }This will help track the performance impact of processing all events.
new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts (3)
1-15: LGTM: Imports and dayjs extension are appropriate.The imports are well-organized and include all necessary modules and types. Extending dayjs with the UTC plugin is a good practice for consistent timestamp handling across different timezones.
17-24: LGTM: JsonLogEvent interface is well-defined.The JsonLogEvent interface provides a clear and type-safe structure for log events. Using Dayjs for the timestamp and LOG_LEVEL enum for the level ensures consistency and type safety throughout the application.
95-100: LGTM: Exports are appropriate and consistent.The file correctly exports the three utility functions (convertToDayjsTimestamp, convertToLogLevelValue, isJsonObject) and the JsonLogEvent type. This aligns well with the file's purpose and content.
new-log-viewer/src/services/MainWorker.ts (1)
53-53: Approve the change from setDecoderOptions to setFormatterOptionsThe modification aligns with the PR's objective of restructuring the jsonl decoder. This change reflects the shift from decoding to formatting, which is consistent with moving parsing to the building phase.
To ensure this change doesn't introduce unintended side effects, please run the following verification script:
✅ Verification successful
Change from setDecoderOptions to setFormatterOptions Verified
The
setFormatterOptionsmethod is consistently used across the codebase, andsetDecoderOptionsis no longer present. No issues were found related to this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing setDecoderOptions to setFormatterOptions # Test: Check if setFormatterOptions is consistently used across the codebase rg --type typescript "setFormatterOptions" -C 3 # Test: Ensure setDecoderOptions is not used anymore rg --type typescript "setDecoderOptions" # Test: Look for any TODOs or FIXMEs related to this change rg --type typescript -i "todo|fixme" -C 3 | rg -i "decoder|formatter"Length of output: 265
Script:
#!/bin/bash # Description: Verify the impact of changing setDecoderOptions to setFormatterOptions # Test: Check if setFormatterOptions is consistently used across the codebase rg "setFormatterOptions" --glob "*.ts" -C 3 # Test: Ensure setDecoderOptions is not used anymore rg "setDecoderOptions" --glob "*.ts" # Test: Look for any TODOs or FIXMEs related to this change rg -i "todo|fixme" --glob "*.ts" -C 3 | rg -i "decoder|formatter"Length of output: 6403
new-log-viewer/src/services/LogFileManager.ts (2)
168-171: Approved changes with a request for clarificationThe update to the
decodeRangemethod call, including the addition of a new boolean parameter, is consistent with the PR objectives of restructuring the jsonl decoder.Could you please clarify the purpose of the new boolean parameter (set to
false) in thedecodeRangemethod? Adding a comment explaining its significance would improve code readability and maintainability.For example:
const results = this.#decoder.decodeRange( beginLogEventIdx, endLogEventIdx, - false, + false, // Set to false to [explain the purpose] );
203-203: Consistent change notedThe update to the
decodeRangemethod call inloadPageis consistent with the change observed in theloadChunkmethod. This systematic change across methods reflects the restructuring of the decoder interface.Please ensure that the clarification requested for the new boolean parameter in the
loadChunkmethod (see previous comment) is also applicable here. Consider adding a similar explanatory comment in this method for consistency and clarity.new-log-viewer/src/services/formatters/LogbackFormatter.ts (2)
9-9: Import statement is appropriateThe import of
JsonLogEventis necessary and correctly specified.
60-65: Ensure all usages offormatLogEventare updated to the new signatureThe
formatLogEventmethod signature has changed from acceptingJsonObjectand returning aTimestampAndMessageTypeto acceptingJsonLogEventand returning astring. Please verify that all calls toformatLogEventin the codebase have been updated accordingly to prevent any runtime errors.You can run the following script to find all usages of
formatLogEvent:Review the results to ensure each usage passes a
JsonLogEventand correctly handles thestringreturn type.✅ Verification successful
Verification Successful: All usages of
formatLogEventare updated to the new signature.
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `formatLogEvent` and check their arguments. rg --type ts 'formatLogEvent\s*\(' -A 2 -B 2Length of output: 1077
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1)
150-156: Confirm the structure ofDecodeResultTypematches the returned array.In
#getDecodeResult, the method returns an array with[message, timestamp, logLevel, lineNumber]. Ensure that this aligns with theDecodeResultTypedefinition to prevent type inconsistencies.Please verify that
DecodeResultTypeis defined as[string, number, LOG_LEVEL, number]. If it differs, adjust the return structure accordingly.
| /** | ||
| * Narrow JSON value to JSON object if compatible. | ||
| * Reference: https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates | ||
| * | ||
| * @param fields | ||
| * @return Whether type is JsonObject. | ||
| */ | ||
| const isJsonObject = (fields: JsonValue): fields is JsonObject => { | ||
| return "object" === typeof fields; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling null values in isJsonObject function.
The isJsonObject function is well-implemented with a correct type predicate. However, it could be more robust by explicitly handling null values.
Consider updating the function to handle null values:
const isJsonObject = (fields: JsonValue): fields is JsonObject => {
- return "object" === typeof fields;
+ return typeof fields === "object" && fields !== null;
};This change ensures that null values are not incorrectly identified as JSON objects.
📝 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.
| /** | |
| * Narrow JSON value to JSON object if compatible. | |
| * Reference: https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates | |
| * | |
| * @param fields | |
| * @return Whether type is JsonObject. | |
| */ | |
| const isJsonObject = (fields: JsonValue): fields is JsonObject => { | |
| return "object" === typeof fields; | |
| }; | |
| /** | |
| * Narrow JSON value to JSON object if compatible. | |
| * Reference: https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates | |
| * | |
| * @param fields | |
| * @return Whether type is JsonObject. | |
| */ | |
| const isJsonObject = (fields: JsonValue): fields is JsonObject => { | |
| return typeof fields === "object" && fields !== null; | |
| }; |
davemarco
changed the title
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/src/services/formatters/LogbackFormatter.ts (1)
Line range hint
1-1: Approve removal of #parseTimestamp with a suggestionThe removal of the
#parseTimestampmethod is consistent with the new implementation and simplifies the class. This change is appropriate given that the timestamp is now expected to be pre-parsed in theJsonLogEvent.Consider adding a comment or updating the class-level JSDoc to specify the expected format of the timestamp in the
JsonLogEvent. This will help maintainers and users of the class understand the requirements for the input data.For example:
/** * A formatter that uses a Logback-like format string to format log events into a string. * * @remarks * The `timestamp` in the `JsonLogEvent` is expected to be a valid dayjs.Dayjs object. * It's the responsibility of the caller to ensure the timestamp is correctly parsed before passing it to this formatter. */ class LogbackFormatter implements Formatter { // ... existing code ... }new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1)
227-227: Add a blank line before the return statement for better readabilityAs indicated by the static analysis hint, adding a blank line before the
return;statement enhances code readability and adheres to the project's style guidelines.🧰 Tools
🪛 GitHub Check: lint-check
[warning] 227-227:
Expected blank line before this statement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- new-log-viewer/src/services/LogFileManager.ts (4 hunks)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts (1 hunks)
- new-log-viewer/src/services/formatters/LogbackFormatter.ts (2 hunks)
- new-log-viewer/src/typings/decoders.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- new-log-viewer/src/services/LogFileManager.ts
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts
- new-log-viewer/src/typings/decoders.ts
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts
[warning] 227-227:
Expected blank line before this statement
🔇 Additional comments (4)
new-log-viewer/src/services/formatters/LogbackFormatter.ts (2)
9-9: LGTM: New import for JsonLogEvent typeThe addition of the
JsonLogEventimport is appropriate and aligns with the changes in theformatLogEventmethod signature. This import enhances type safety and improves code clarity.
58-58: LGTM: Updated JSDoc commentThe JSDoc comment has been appropriately updated to reflect the new return type of the
formatLogEventmethod. This change maintains consistency between the documentation and the implementation.new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (2)
71-80: Efficient memory management in thebuildmethodThe
buildmethod correctly invokes#deserialize()and frees the data array afterward, promoting efficient use of memory.
129-156: Proper handling of valid and invalid log events in#getDecodeResultThe method effectively distinguishes between valid and invalid log events, ensuring robust and reliable decoding results.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (2)
74-74: Simplify the calculation ofnumInvalidEventsusingMap.sizeYou can use the
sizeproperty of theMapobject to get the number of invalid events directly, which improves performance and readability.Apply this diff:
- const numInvalidEvents = Array.from(this.#invalidLogEventIdxToRawLine.keys()).length; + const numInvalidEvents = this.#invalidLogEventIdxToRawLine.size;
228-228: Add a blank line before thereturnstatement for readabilityThe linter suggests adding a blank line before the
returnstatement to adhere to the project's coding style and improve readability.Apply this diff:
if (null === logLevelFilter) { this.#filteredLogEventMap = null; + return; }🧰 Tools
🪛 GitHub Check: lint-check
[warning] 228-228:
Expected blank line before this statement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1 hunks)
- new-log-viewer/src/typings/decoders.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/typings/decoders.ts
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts
[warning] 228-228:
Expected blank line before this statement
🔇 Additional comments (1)
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1)
57-59:getEstimatedNumEventsmethod implementation is correctThe method accurately returns the total number of log events stored.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
new-log-viewer/src/typings/logs.ts (3)
1-7: Simplify and standardize import statementsThe new imports are necessary for the added type definitions. However, the import style for
JsonObjectis inconsistent with the others and flagged by the linter. Consider simplifying it to a single-line import for consistency and to resolve linter warnings.Apply this change to standardize the imports:
import {Nullable} from "./common"; import {Dayjs} from "dayjs"; -import { - JsonObject, -} from "./js"; +import {JsonObject} from "./js";🧰 Tools
🪛 GitHub Check: lint-check
[warning] 1-1:
Run autofix to sort these imports!
[failure] 5-5:
Imports must not be broken into multiple lines if there are 1 or less elements
[failure] 5-5:
Unexpected line break after this opening brace
[failure] 7-7:
Unexpected line break before this closing brace
[failure] 7-7:
Expected 2 empty lines after import statement not followed by another import
29-31: Simplify export statementWhile it's correct to export the new
JsonLogEventandLogLevelFiltertypes, the multi-line format is inconsistent with the single-line export below. To improve consistency and resolve the linter warning, consider simplifying this to a single-line export.Apply this change to standardize the exports:
-export type { - JsonLogEvent, - LogLevelFilter}; +export type {JsonLogEvent, LogLevelFilter};🧰 Tools
🪛 GitHub Check: lint-check
[failure] 31-31:
Expected a line break before this closing brace
Line range hint
1-36: Run linter autofix for remaining issuesThe overall structure of the file is good, with clear separation of imports, type definitions, and exports. However, there are several linting warnings about line breaks and spacing. To improve code consistency and readability, it's recommended to run the linter's autofix feature.
Please run the linter's autofix command to resolve the remaining stylistic issues, such as:
- Sorting imports
- Adjusting line breaks after import statements
- Ensuring consistent spacing
This will help maintain a consistent code style across the project.
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 31-31:
Expected a line break before this closing bracenew-log-viewer/src/typings/formatters.ts (1)
1-1: Style: Add empty lines after import statementTo adhere to the project's style guide and resolve the linting issue, please add two empty lines after the import statement.
Apply this diff to fix the linting issue:
import {JsonLogEvent} from "./logs"; + + /** * Options for the LogbackFormatter.🧰 Tools
🪛 GitHub Check: lint-check
[failure] 1-1:
Expected 2 empty lines after import statement not followed by another importnew-log-viewer/src/services/formatters/LogbackFormatter.ts (1)
Line range hint
122-134: Add null check in #formatTimestamp methodThe
#formatTimestampmethod doesn't handle null timestamps, which could lead to runtime errors. Consider adding a null check:#formatTimestamp (timestamp: dayjs.Dayjs, formatString: string): string { - if (null === this.#datePattern) { + if (null === this.#datePattern || null === timestamp) { return formatString; } const formattedDate = timestamp.format(this.#dateFormat); formatString = formatString.replace(this.#datePattern, formattedDate); return formatString; }This change will prevent potential runtime errors when dealing with null timestamps.
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (3)
1-23: Consider sorting import statements to improve code readabilityThe static analysis tool indicates that the import statements are not sorted. Sorting imports enhances readability and maintains consistency across the codebase.
Apply the suggested autofix from the linter to sort the imports.
🧰 Tools
🪛 GitHub Check: lint-check
[warning] 1-1:
Run autofix to sort these imports!
28-28: Correct capitalization of 'JSON Lines' in the documentationIn the comment, 'JSON lines' should be capitalized as 'JSON Lines' to accurately reference the file format.
Apply this diff to correct the capitalization:
- * A decoder for JSONL (JSON lines) files that contain log events. + * A decoder for JSONL (JSON Lines) files that contain log events.
193-193: Add a blank line beforereturn;to adhere to code style guidelinesThe static analysis tool indicates that a blank line is expected before the
return;statement. Adding a blank line improves code readability and follows the project's coding conventions.Apply this diff:
this.#filteredLogEventMap = null; + return; return;🧰 Tools
🪛 GitHub Check: lint-check
[warning] 193-193:
Expected blank line before this statement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- new-log-viewer/src/services/LogFileManager.ts (4 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts (1 hunks)
- new-log-viewer/src/services/formatters/LogbackFormatter.ts (2 hunks)
- new-log-viewer/src/typings/formatters.ts (2 hunks)
- new-log-viewer/src/typings/logs.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- new-log-viewer/src/services/LogFileManager.ts
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/services/decoders/JsonlDecoder/index.ts
[warning] 1-1:
Run autofix to sort these imports!
[warning] 193-193:
Expected blank line before this statementnew-log-viewer/src/typings/formatters.ts
[failure] 1-1:
Expected 2 empty lines after import statement not followed by another importnew-log-viewer/src/typings/logs.ts
[warning] 1-1:
Run autofix to sort these imports!
[failure] 5-5:
Imports must not be broken into multiple lines if there are 1 or less elements
[failure] 5-5:
Unexpected line break after this opening brace
[failure] 7-7:
Unexpected line break before this closing brace
[failure] 7-7:
Expected 2 empty lines after import statement not followed by another import
[failure] 31-31:
Expected a line break before this closing brace
🔇 Additional comments (7)
new-log-viewer/src/typings/logs.ts (2)
19-19: LGTM: New LogLevelFilter typeThe new
LogLevelFiltertype alias is well-defined and provides a clear way to represent an optional array of log levels. This will be useful for implementing log filtering functionality in the future.
23-27: LGTM: Well-structured JsonLogEvent interfaceThe new
JsonLogEventinterface is well-designed and provides a clear structure for JSON log events. It makes good use of theDayjstype for timestamps, theLOG_LEVELenum for log levels, and theJsonObjecttype for flexible field data. This structure will enhance type safety and consistency in log event handling throughout the application.new-log-viewer/src/typings/formatters.ts (3)
22-24: LGTM: Simplified LogbackFormatterOptionsTypeThe removal of the
timestampKeyproperty fromLogbackFormatterOptionsTypeis consistent with the PR's objective of relocating timestamp parsing. This simplification suggests a standardized approach to timestamp handling.As mentioned in a previous review, please ensure that all references to
timestampKeyin other files have been addressed.
31-31: LGTM: Updated Formatter interfaceThe changes to the
formatLogEventmethod signature in theFormatterinterface align well with the PR objectives. The use ofJsonLogEventinstead ofJsonObjectimproves type safety, and the simplified return type (string) suggests a streamlined approach to log formatting.To ensure this change doesn't break existing code, please run the following verification:
#!/bin/bash # Description: Check for any remaining uses of TimestampAndMessageType in the codebase # Test: Search for TimestampAndMessageType. Expect: No results or only in test files. rg --type typescript 'TimestampAndMessageType' # Test: Search for formatLogEvent method calls. Expect: Updated method calls in relevant files. rg --type typescript 'formatLogEvent'
1-1: LGTM: Import statement updated to use JsonLogEventThe change from
JsonObjecttoJsonLogEventaligns with the PR objectives and suggests improved type safety for log events.To ensure this change is consistently applied, please run the following verification:
✅ Verification successful
Verification Successful: Import Statement Update Confirmed
All instances of
JsonLogEventare correctly implemented across the codebase, and no remainingJsonObjectreferences related to log events were detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of JsonLogEvent across the codebase # Test: Search for JsonLogEvent usage. Expect: Multiple occurrences in relevant files. rg --type typescript 'JsonLogEvent' # Test: Search for any remaining JsonObject usage related to log events. Expect: No results or only in test files. rg --type typescript 'JsonObject.*log'Length of output: 154
Script:
#!/bin/bash # Description: Verify the usage of JsonLogEvent across the codebase # Test: Search for JsonLogEvent usage. Expect: Multiple occurrences in relevant files. rg 'JsonLogEvent' # Test: Search for any remaining JsonObject usage related to log events. Expect: No results or only in test files. rg 'JsonObject.*log'Length of output: 855
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 1-1:
Expected 2 empty lines after import statement not followed by another importnew-log-viewer/src/services/formatters/LogbackFormatter.ts (2)
9-9: Clarify handling of invalid timestampsWith the removal of the
#parseTimestampmethod and the#timestampKeyproperty, it's not clear how invalid timestamps are being handled. Could you please clarify:
- How are invalid timestamps being managed now?
- What are the implications of removing the
#parseTimestampmethod?- Is the
JsonLogEventtype ensuring that timestamps are always valid?This information will help ensure that the log processing system remains robust in handling various timestamp formats and potential errors.
60-65:⚠️ Potential issueAdd null check for timestamp and consider return type change impact
The refactored
formatLogEventmethod is more concise, which is good. However, there are a few points to consider:
- The method doesn't handle null timestamps. Consider adding a null check:
formatLogEvent (logEvent: JsonLogEvent): string { const {fields, timestamp} = logEvent; + if (timestamp === null) { + console.warn('Received null timestamp in logEvent'); + return this.#formatVariables(this.#formatString, fields); + } const formatStringWithTimestamp: string = this.#formatTimestamp(timestamp, this.#formatString); return this.#formatVariables(formatStringWithTimestamp, fields); }
The return type has changed from an object to a string. Please verify that this change doesn't break any code that relies on the previous return type (
TimestampAndMessageType).The removal of timestamp parsing logic might affect how invalid timestamps are handled. How are invalid timestamps being managed now?
To verify the impact of the return type change, please run the following script:
| getFilteredLogEventMap (): FilteredLogEventMap { | ||
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Update this after log level filtering is implemented in clp-ffi-js | ||
| return null; |
There was a problem hiding this comment.
Would it be better if we make the log-level filtering a no-op rather than completely disabling the decoder?
| return null; | |
| console.error("Not implemented."); | |
| return Array.from({length: this.#streamReader.getNumEventsBuffered()}, (_, i) => i); |
There was a problem hiding this comment.
I put in the console error but not the return value. If the value is null, the new plan is it will just not use the filter, so it is okay to return null.
…en manually chaning log event
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- new-log-viewer/src/contexts/StateContextProvider.tsx (5 hunks)
- new-log-viewer/src/services/LogFileManager/index.ts (4 hunks)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/index.ts (1 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts (1 hunks)
- new-log-viewer/src/services/formatters/LogbackFormatter.ts (2 hunks)
- new-log-viewer/src/typings/formatters.ts (2 hunks)
- new-log-viewer/src/typings/logs.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts
- new-log-viewer/src/services/decoders/JsonlDecoder/index.ts
- new-log-viewer/src/typings/logs.ts
🧰 Additional context used
🪛 GitHub Check: lint-check
new-log-viewer/src/contexts/StateContextProvider.tsx
[failure] 224-224:
Missing semicolon
🔇 Additional comments (14)
new-log-viewer/src/typings/formatters.ts (3)
23-25: Reminder: EnsuretimestampKeyremoval is addressed across the codebase.The removal of the
timestampKeyproperty fromLogbackFormatterOptionsTypealigns with the PR's goal of relocating timestamp parsing. However, a previous review comment indicated that this property might still be referenced in other files.Please ensure that all references to
timestampKeyhave been addressed in the following files:
new-log-viewer/src/utils/config.tsnew-log-viewer/src/typings/decoders.tsnew-log-viewer/src/typings/config.tsnew-log-viewer/src/services/decoders/JsonlDecoder/index.tsnew-log-viewer/src/components/modals/SettingsModal/SettingsDialog.tsxLet's verify the current status:
#!/bin/bash # Description: Check for any remaining uses of timestampKey in the specified files # Test: Search for 'timestampKey' in the specified files rg 'timestampKey' new-log-viewer/src/utils/config.ts new-log-viewer/src/typings/decoders.ts new-log-viewer/src/typings/config.ts new-log-viewer/src/services/decoders/JsonlDecoder/index.ts new-log-viewer/src/components/modals/SettingsModal/SettingsDialog.tsx
32-32: LGTM: Formatter interface update streamlines log event processing.The update to the
formatLogEventmethod signature in theFormatterinterface is consistent with the overall restructuring. Changing the parameter type toLogEventand simplifying the return type tostringsuggests a more streamlined formatting process.Let's verify the impact on implementations of the Formatter interface:
#!/bin/bash # Description: Check for implementations of the Formatter interface # Test: Search for classes implementing the Formatter interface rg --type typescript 'class.*implements.*Formatter' # Test: Search for formatLogEvent method implementations rg --type typescript 'formatLogEvent.*\(.*\).*{' -g '!*test*'
1-1: LGTM: Import change aligns with restructuring goals.The change from
JsonObjecttoLogEventis consistent with the PR's objective of restructuring the log decoder. This suggests a move towards a more specific and structured log event type.Let's verify the usage of
LogEventacross the codebase:✅ Verification successful
Verification Successful:
LogEventis consistently used across the codebase.The change from
JsonObjecttoLogEventhas been properly applied in multiple relevant files, aligning with the PR's objective of restructuring the log decoder.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of LogEvent type across the codebase # Test: Search for LogEvent usage. Expect: Multiple occurrences in relevant files. rg --type typescript 'LogEvent'Length of output: 461
Script:
#!/bin/bash # Description: Verify the usage of LogEvent type across the codebase # Test: Search for LogEvent usage in TypeScript files. Expect: Multiple occurrences in relevant files. rg --type ts 'LogEvent'Length of output: 14453
new-log-viewer/src/services/formatters/LogbackFormatter.ts (3)
9-9: LGTM: Import statement addition is appropriate.The addition of the
LogEventimport aligns well with the changes in method signatures, particularly in theformatLogEventmethod. This change enhances type safety and improves code readability.
Line range hint
1-156: Verify impact of removing #parseTimestamp method.The removal of the
#parseTimestampmethod aligns with the restructuring of the log processing system. However, it's important to ensure that timestamp parsing and error handling for invalid timestamp values are adequately addressed elsewhere in the codebase.Please run the following script to verify that timestamp parsing is handled appropriately:
#!/bin/bash # Description: Verify timestamp parsing handling in the codebase # Test 1: Search for timestamp parsing logic echo "Searching for timestamp parsing logic:" rg --type typescript -A 5 $'parseTimestamp|parse.*timestamp' # Test 2: Search for dayjs usage related to timestamps echo "Searching for dayjs usage related to timestamps:" rg --type typescript -A 5 $'dayjs\s*\('Review the results to ensure that timestamp parsing and error handling are implemented correctly in the appropriate location within the codebase.
60-60: Approve signature change and verify usage across codebase.The updated method signature improves type safety by using the more specific
LogEventtype instead ofJsonObject. The change in return type tostringsimplifies the method's output. These changes align well with the refactoring objectives.Please run the following script to verify that all calls to
formatLogEventhave been updated accordingly:✅ Verification successful
Update the shell script to use the correct file type identifier 'ts' instead of 'typescript'.
Please run the following script to verify that all calls to
formatLogEventhave been updated accordingly:
Signature change verified and all usages are consistent.
The
formatLogEventmethod signature has been successfully updated, and all calls to this method across the codebase are consistent with the new signature. No issues were found during the verification process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to formatLogEvent match the new signature # Test: Search for function calls to formatLogEvent. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'formatLogEvent\s*\('Length of output: 366
Script:
#!/bin/bash # Description: Verify all calls to formatLogEvent match the new signature # Test: Search for function calls to formatLogEvent. Expect: Only occurrences of the new signature. rg --type ts -A 5 'formatLogEvent\s*\('Length of output: 1205
new-log-viewer/src/contexts/StateContextProvider.tsx (5)
189-189: Excellent use of React state for page number management.The introduction of the
pageNumstate variable usinguseStateis a great improvement. This approach aligns better with React's state management paradigm compared to the previous use ofuseRef. It ensures that changes to the page number will trigger re-renders, which is crucial for keeping the UI in sync with the current page.
198-198: Removal ofpageNumRefimproves code clarity and state management.The removal of
pageNumRefis a positive change that complements the introduction of thepageNumstate variable. This simplification eliminates potential synchronization issues between the ref and the rendered state, making the code more maintainable and less prone to errors related to stale ref values.
293-293: Correct usage ofpageNumstate inloadPageByAction.The update to use
pageNuminstead ofpageNumRef.currentin theloadPageByActionfunction is correct and consistent with the new state-based approach. This change ensures that the most current page number is always used when navigating between pages, maintaining synchronization between the UI state and the navigation logic.
385-385: Correct update of context value withpageNumstate.The change to use
pageNuminstead ofpageNumRef.currentin the StateContext.Provider value is correct and consistent with the new state-based approach. This ensures that any components consuming this context will receive updates and re-render when the page number changes, maintaining consistency throughout the application.
Line range hint
1-391: Overall excellent refactoring of page number management.The changes in this file represent a significant improvement in state management for the
StateContextProvidercomponent. By replacingpageNumRefwith apageNumstate variable, the code now better aligns with React's state management paradigm. This refactoring ensures that:
- Changes to the page number will trigger appropriate re-renders.
- The risk of stale values due to ref usage is eliminated.
- The code is more maintainable and easier to reason about.
These changes contribute to a more robust and reactive log viewer application. Great job on this refactoring!
new-log-viewer/src/services/LogFileManager/index.ts (3)
144-144: Verify the correctness of parameters passed todecodeRangeEnsure that the parameters
beginLogEventIdxandendLogEventIdxpassed todecodeRangeaccurately represent the intended range of log events. It's important to confirm that these indices align with the decoder's expectations to avoid potential off-by-one errors.If needed, consult the
decodeRangemethod's documentation or implementation details to ensure proper usage.Also applies to: 146-147
186-191:⚠️ Potential issueDouble-check index adjustments to prevent off-by-one errors
Subtracting 1 from
pageBeginLogEventNumandpageEndLogEventNummay introduce off-by-one errors if not handled correctly. Please verify that this adjustment is necessary and aligns with zero-based indexing used by the decoder.Consider reviewing the index calculations or applying the following diff if an adjustment is required:
- pageBeginLogEventNum - 1, - pageEndLogEventNum - 1, + pageBeginLogEventNum, + pageEndLogEventNum,To confirm the correct indices, you can perform tests or review decoder expectations.
124-128: Ensure consistent method naming across the codebaseThe method
setDecoderOptionshas been renamed tosetFormatterOptions, which clarifies its purpose. Please verify that all references to this method have been updated accordingly throughout the codebase and in any related documentation.To confirm consistency, you can run the following script:
✅ Verification successful
Consistent method naming confirmed
All references to
setDecoderOptionshave been appropriately renamed tosetFormatterOptions. No remaining references to the old method name were found, ensuring consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old method name. # Test: Search for occurrences of 'setDecoderOptions'. Expect: No matches. rg 'setDecoderOptions' # Verify that 'setFormatterOptions' is used consistently. rg 'setFormatterOptions'Length of output: 823
| @@ -220,7 +221,7 @@ | |||
| break; | |||
| case WORKER_RESP_CODE.PAGE_DATA: { | |||
| setLogData(args.logs); | |||
| pageNumRef.current = args.pageNum; | |||
| setPageNum(args.pageNum) | |||
There was a problem hiding this comment.
Update to use setPageNum is correct, but a semicolon is missing.
The change to use setPageNum(args.pageNum) is correct and consistent with the new state-based approach for managing the page number. This ensures that the component will re-render when the page number changes.
However, there's a minor syntax issue:
Add a semicolon at the end of the line for consistency with the coding style:
- setPageNum(args.pageNum)
+ setPageNum(args.pageNum);📝 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.
| setPageNum(args.pageNum) | |
| setPageNum(args.pageNum); |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 224-224:
Missing semicolon
| const isJsonObject = (value: JsonValue): value is JsonObject => { | ||
| return "object" === typeof value && null !== value; | ||
| }; |
There was a problem hiding this comment.
Ensure value is not an array in isJsonObject
In JavaScript, arrays are of type "object", so the current check may incorrectly identify arrays as JsonObject. This could lead to unexpected behaviour when processing logs.
Apply the following change to exclude arrays:
return "object" === typeof value && null !== value
+ && !Array.isArray(value);This ensures that only non-null objects that are not arrays are considered as JsonObject.
📝 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.
| const isJsonObject = (value: JsonValue): value is JsonObject => { | |
| return "object" === typeof value && null !== value; | |
| }; | |
| const isJsonObject = (value: JsonValue): value is JsonObject => { | |
| return "object" === typeof value && null !== value | |
| && !Array.isArray(value); | |
| }; |
There was a problem hiding this comment.
The Rabbit is right about this one.
There was a problem hiding this comment.
@kirkrodrigues, glad to hear that!
(\╹‿╹)/
| // Build index for the entire file. | ||
| const buildResult = decoder.build(); | ||
| if (0 < buildResult.numInvalidEvents) { | ||
| console.error("Invalid events found in decoder.build():", buildResult); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling for invalid events
While logging invalid events using console.error aids in debugging, it might be beneficial to implement a more robust error-handling mechanism. This could include counting invalid events, aggregating them for reporting, or providing feedback to the user if a significant number are encountered.
|
I did notice a small bug that the page number sometimes does not update when manually setting the log event number. I am 95% sure this is due to pageNum being a ref. However, I will fix in another PR, and not here. This PR is ready for final review, then merge. |
| // Json stringify covers edge case where the field is an object with more than one key, e.g., | ||
| // `field = { "name": "INFO", "value": 20 }`. | ||
| const logLevelName = "object" === typeof field ? | ||
| JSON.stringify(field) : |
There was a problem hiding this comment.
@junhaoliao, if it's an object, shouldn't we just return early? A stringified JSON object won't match any of our hard-coded log levels, right?
There was a problem hiding this comment.
If we are to support stringified JSON objects, the code below should be written as
const uppercaseLogLevelName = logLevelName.toUpperCase();
logLevelValue = LOG_LEVEL_NAMES.splice(1).find((name) =>
uppercaseLogLevelName.includes(name)
) ?? LOG_LEVEL.NONE;
I vaguely remember seeing log levels being stored nested as an object in some logs though I can't find the logs now. If you believe the code has too much overhead introduced, let's do early returns, and we can come back to this for a more properly implementation when we see such requirements.
There was a problem hiding this comment.
Gotcha. Iirc, the MongoDB log dataset has the level nested in key. That said, I'm not sure we should bother stringifying the field if it's an object since:
- It introduces overhead.
- It's a heuristic to search for the log level string in the stringified object.
- It hides the issue from the user which is really that they should be specifying the nested key as the level field.
What do you think?
There was a problem hiding this comment.
Sound good. Let's do early return here and defer the requirement of handling such nested objects.
Just curious, how does clp-s handle timestamps from nested objects? e.g., how should the user specify a timestamp key if the log looks like this:
{
"t":
{
"timestamp.value": 123
}
}There was a problem hiding this comment.
Theoretically, the user would escape any periods that are part of a key. So in the example: t.timestamp\.value. That said, clp-s doesn't support querying keys with escaped characters yet.
There was a problem hiding this comment.
I see. In the log viewer, we don't treat periods as children access specifiers at the moment so accessing nested nodes are impossible. Let me create an issue for this.
There was a problem hiding this comment.
Added early return for object. Used generic object so will early return for null & array as well.
| const isJsonObject = (value: JsonValue): value is JsonObject => { | ||
| return "object" === typeof value && null !== value; | ||
| }; |
There was a problem hiding this comment.
The Rabbit is right about this one.
| // If the field is an object , e.g. `field = { "name": "INFO", "value": 20 }`. | ||
| // Then the user should specify a nested key. |
There was a problem hiding this comment.
I'm not sure this comment is necessary since the if below basically reads: If it's undefined or an object, ignore it, which I think is pretty clear.
|
|
||
| // If the field is an object , e.g. `field = { "name": "INFO", "value": 20 }`. | ||
| // Then the user should specify a nested key. | ||
| if ("undefined" === typeof field || "object" === typeof field) { |
There was a problem hiding this comment.
Shall we use isJsonObject? Otherwise it's possible that field is an array or null.
| * @return A TypeScript type predicate indicating whether `value` is a `JsonObject`. | ||
| */ | ||
| const isJsonObject = (value: JsonValue): value is JsonObject => { | ||
| return "object" === typeof value && null !== value && !Array.isArray(value); |
There was a problem hiding this comment.
| return "object" === typeof value && null !== value && !Array.isArray(value); | |
| return "object" === typeof value && null !== value && false === Array.isArray(value); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
new-log-viewer/src/services/decoders/ClpIrDecoder.ts (3)
37-44: LGTM: Method stub added for future implementation.The
getFilteredLogEventMapmethod has been correctly added as a placeholder for future implementation. The TODO comment provides clear guidance for developers.Consider adding a JSDoc comment to describe the intended functionality of this method, which would be helpful for other developers working on this class.
46-53: LGTM: Method stub added for future implementation.The
setLogLevelFiltermethod has been correctly added as a placeholder for future implementation. The TODO comment provides clear guidance for developers.Consider adding a JSDoc comment to describe the intended functionality of this method, including the expected behaviour when the filter is successfully set.
67-73: LGTM: Method renamed and updated for future filtering support.The
decodemethod has been appropriately renamed todecodeRange, which better describes its functionality. The addition of theuseFilterparameter aligns with the PR objectives for future log level filtering.Consider adding a TODO comment explaining the intended use of the
useFilterparameter in the future implementation. This would complement the existing eslint-disable comment and provide clarity for future development.new-log-viewer/src/typings/decoders.ts (1)
52-54: LGTM: getFilteredLogEventMap method added with a minor suggestion.The getFilteredLogEventMap method is correctly defined and returns the appropriate type. However, the comment could be more descriptive.
Consider expanding the comment to provide more context:
- * @return The filtered log events map. + * @return The filtered log events map, which maps indices in the filtered collection to indices in the unfiltered collection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2 hunks)
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts (1 hunks)
- new-log-viewer/src/typings/decoders.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- new-log-viewer/src/services/decoders/JsonlDecoder/utils.ts
🔇 Additional comments (9)
new-log-viewer/src/services/decoders/ClpIrDecoder.ts (2)
7-8: LGTM: New imports added correctly.The new imports for
FilteredLogEventMapandLOG_EVENT_FILE_END_IDXhave been added appropriately. These additions align with the new methods and changes in the class.
55-59: LGTM: Build method updated to align with restructuring objectives.The
buildmethod has been successfully updated to useLOG_EVENT_FILE_END_IDX, ensuring consistent behaviour across the class. The return typeLogEventCountprovides more structured information about the build process, which is a good improvement.new-log-viewer/src/typings/decoders.ts (7)
2-2: LGTM: Import statement for LogLevelFilter added.The import of LogLevelFilter is correctly placed and necessary for the new log level filtering functionality.
36-40: LGTM: FilteredLogEventMap type added.The FilteredLogEventMap type is well-defined and properly documented. The use of Nullable<number[]> is appropriate for cases where the map might not exist.
56-63: LGTM: setLogLevelFilter method added.The setLogLevelFilter method is well-defined with appropriate parameters and return type. The comment provides a clear description of the method's purpose and return value.
64-70: LGTM: build method updated.The build method has been simplified by removing parameters and now returns a more specific LogEventCount type. The updated comment accurately reflects these changes.
73-78: LGTM: setFormatterOptions method renamed and updated.The method has been appropriately renamed from setDecoderOptions to setFormatterOptions, clarifying its specific purpose. The updated comment accurately reflects this change while maintaining the correct parameter and return types.
81-94: LGTM: decodeRange method updated with new parameter.The method has been appropriately renamed from decode to decodeRange, better reflecting its functionality. The addition of the useFilter parameter enhances flexibility in decoding. The updated comment provides a clear and comprehensive explanation of the method's purpose and parameters.
108-108: LGTM: FilteredLogEventMap added to export statement.The FilteredLogEventMap type has been correctly added to the export statement, making it accessible to other modules that may need to use this new type.
davemarco
changed the title
References
new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62 #63 #66 #67 #68 #69 #70 #71 #72 #73 #74 #76
Description
Restructures jsonl decoder which will eventually enable log level filtering. Most of the code from this PR is inspired by #63, however, I did a proper review and made many improvements. PR is effectively a heavy refactor of a portion of #63. PR does not add any new functionality to log viewer, it just sets up future PRs. Note the PR is not as big as it looks, I moved some files and github thinks they are completely new.
Specifically made the following changes:
Parsing timestamp and log level moved from decode to build.
jsonl decoder now parses the log level and timestamp during building rather than decoding, this allows us to filter logs without decoding all the events. This also affected the formatter as now it has less work to do since the timestamp is already converted to a dayjs object during build. Effectively it moves more work to the build stage to simplify filtering algorithms.
Add new filtering methods
Added a few new methods that will be required when filtering is enabled. See
decoders.tsfor a breakdown of interface changes / new methods. Most importantly, added a method to actually filter the jsonl logs. Note none of the new methods are actually being called in this PR, it just sets up future filtering PRs.Validation performed
I did tests on jsonl and ir files, and everything seemed okay. I did notice the page skipping was a little buggy, but i tested main branch and it had the same bugs. Hopefully #76 unintentionally fixes these bugs.
Summary by CodeRabbit
Release Notes
New Features
JsonlDecoderclass for processing JSONL formatted log files.Improvements
Bug Fixes
Documentation