feat(decoder): Add support for Structured CLP IR streams but without log-level filtering. #85
Conversation
WalkthroughThe pull request includes updates to various files within the project, primarily focusing on the type definitions related to decoder options. The 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 (
|
junhaoliao
changed the title
| #streamReader: ClpIrStreamReader; | ||
| #streamReader: ClpStreamReader; | ||
|
|
||
| #formatter: Nullable<LogbackFormatter>; |
There was a problem hiding this comment.
There is a formatter interface might as well use it
| #formatter: Nullable<LogbackFormatter>; | |
| #formatter: Nullable<Formatter>; |
| results.forEach((result) => { | ||
| const [message, timestamp] = result; | ||
| let fields: JsonObject = {}; | ||
| try { | ||
| fields = JSON.parse(message) as JsonObject; | ||
| if (false === isJsonObject(fields)) { | ||
| throw new Error("Unexpected non-object."); | ||
| } else if (null !== this.#logLevelKey) { | ||
| const logLevel = fields[this.#logLevelKey]; | ||
| fields[this.#logLevelKey] = LOG_LEVEL[logLevel as LOG_LEVEL]; | ||
| } | ||
| } catch (e) { | ||
| if (0 === message.length) { | ||
| return; | ||
| } | ||
| console.error(e, message); | ||
| } | ||
|
|
||
| // @ts-expect-error `this.#formatter` is certainly non-null | ||
| result[0] = this.#formatter.formatLogEvent({ | ||
| fields: fields, | ||
| timestamp: convertToDayjsTimestamp(timestamp), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Here is a simpler version. I feel like we dont need to get the log level since it should already be in the result. I also dont think we need to check if the length is 0, since i dont think clp-ffi-js would return length 0. I think worst case it will return {} if the log event is empty.
| results.forEach((result) => { | |
| const [message, timestamp] = result; | |
| let fields: JsonObject = {}; | |
| try { | |
| fields = JSON.parse(message) as JsonObject; | |
| if (false === isJsonObject(fields)) { | |
| throw new Error("Unexpected non-object."); | |
| } else if (null !== this.#logLevelKey) { | |
| const logLevel = fields[this.#logLevelKey]; | |
| fields[this.#logLevelKey] = LOG_LEVEL[logLevel as LOG_LEVEL]; | |
| } | |
| } catch (e) { | |
| if (0 === message.length) { | |
| return; | |
| } | |
| console.error(e, message); | |
| } | |
| // @ts-expect-error `this.#formatter` is certainly non-null | |
| result[0] = this.#formatter.formatLogEvent({ | |
| fields: fields, | |
| timestamp: convertToDayjsTimestamp(timestamp), | |
| }); | |
| }); | |
| for (const result of results) { | |
| const [message, timestamp, level] = result; | |
| let fields: JsonObject = {}; | |
| try { | |
| fields = JSON.parse(message) as JsonObject; | |
| if (false === isJsonObject(fields)) { | |
| throw new Error("Unexpected non-object."); | |
| } | |
| } catch (e) { | |
| console.error(e, message); | |
| } | |
| const dayJsTimestamp: Dayjs = convertToDayjsTimestamp(timestamp); | |
| const logEvent: LogEvent = { | |
| timestamp: dayJsTimestamp, | |
| level: level, | |
| fields: fields | |
| } | |
| result[0] = this.#formatter.formatLogEvent(logEvent); | |
| } |
There was a problem hiding this comment.
I feel like we dont need to get the log level since it should already be in the result.
Right. For now we shall store log level strings as authoritative log levels in the logging library.
I also dont think we need to check if the length is 0, since i dont think clp-ffi-js would return length 0. I think worst case it will return {} if the log event is empty.
Good catch
src/typings/decoders.ts
Outdated
| @@ -14,13 +14,25 @@ interface LogEventCount { | |||
| * @property logLevelKey The key of the kv-pair that contains the log level in every record. | |||
| * @property timestampKey The key of the kv-pair that contains the timestamp in every record. | |||
| */ | |||
| interface JsonlDecoderOptionsType { | |||
| interface JsonlDecoderOptions { | |||
There was a problem hiding this comment.
How do you feel about just naming this DecoderOptions and get rid of JsonlDecoderOptions, StructuredClpIrDecoderOptions, ClpIrDecoderOptions. I feel like we have all these extra types that serve no purpose. Maybe if we actually need different types in the future can add back?
| ({logLevelKey} = decoderOptions); | ||
| } | ||
|
|
||
| return new ClpIrDecoder(streamReader, formatter, logLevelKey); |
There was a problem hiding this comment.
| return new ClpIrDecoder(streamReader, formatter, logLevelKey); | |
| return new ClpIrDecoder(streamReader, formatter); |
There was a problem hiding this comment.
Added one streamType argument to make the handling more obvious.
| module.IRProtocolErrorCode.SUPPORTED === streamReader.getIrProtocolErrorCode() | ||
| ) { | ||
| formatter = new LogbackFormatter({formatString: decoderOptions.formatString}); | ||
| ({logLevelKey} = decoderOptions); |
There was a problem hiding this comment.
| ({logLevelKey} = decoderOptions); |
|
|
||
| constructor (streamReader: ClpIrStreamReader) { | ||
| constructor (streamReader: ClpStreamReader, formatter: Nullable<LogbackFormatter>, logLevelKey: Nullable<string>) { |
There was a problem hiding this comment.
| constructor (streamReader: ClpStreamReader, formatter: Nullable<LogbackFormatter>, logLevelKey: Nullable<string>) { | |
| constructor (streamReader: ClpStreamReader, formatter: Nullable<Formatter>) { |
src/services/MainWorker.ts
Outdated
There was a problem hiding this comment.
Not clear to me why we need this bigint support in this PR? It seems to work with IRv1 stream reader?
There was a problem hiding this comment.
Two reasons:
- We enabled link flag
-sWASM_BIGINTwhen compiling clp-ffi-js: https://github.com/y-scope/clp-ffi-js/blob/450e8f84f17ec233765f1e79e60984cd8085076e/CMakeLists.txt#L72 - The timestamps returned by
reader.decodeRange()are indeedBigInt. See TS type of timestamp indecodeRangereturned results should be BigInt instead of Number. clp-ffi-js#33 . Also:- In IRv1, timestamps are of type
int64_twhich maps to BigInt by Emscripten thanks to the above flag; however, we previously have never processed the returned timestamps with Dayjs / at all so Dayjs's inability to deal with BigInt timestamps without extensions was not discovered. - In IRv2, integers are of type
int64_t. At the moment, we read all timestamp node values as integers and return directly without any processing.
- In IRv1, timestamps are of type
…uggestions from code review Co-authored-by: davemarco <[email protected]>
junhaoliao
changed the title
clp-ffi-js to 0.3.0; Refactor ClpIrDecoder to also support Structured CLP IR streams.
junhaoliao
changed the title
clp-ffi-js to 0.3.0; Refactor ClpIrDecoder to also support Structured CLP IR streams.clp-ffi-js to 0.3.0; Refactor ClpIrDecoder to also support Structured CLP IR streams without log-level filtering.
junhaoliao
changed the title
clp-ffi-js to 0.3.0; Refactor ClpIrDecoder to also support Structured CLP IR streams without log-level filtering.clp-ffi-js to 0.3.0; Refactor ClpIrDecoder to also support Structured CLP IR streams but without log-level filtering.
| @@ -8,20 +8,16 @@ interface LogEventCount { | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Description intentionally omitted.
![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
🧹 Outside diff range and nitpick comments (5)
src/services/decoders/JsonlDecoder/utils.ts (1)
Line range hint
56-68: Consider improving type validation clarityThe condition structure could be more readable. Consider extracting the type validation into a separate function.
Here's a suggested refactoring:
const convertToDayjsTimestamp = (field: JsonValue | bigint | undefined): dayjs.Dayjs => { - if (("string" !== typeof field && - "number" !== typeof field && - "bigint" !== typeof field) || - "undefined" === typeof field - ) { + const isValidTimestampType = (field: JsonValue | bigint | undefined): boolean => { + return (typeof field === "string" || + typeof field === "number" || + typeof field === "bigint") && + typeof field !== "undefined"; + }; + + if (!isValidTimestampType(field)) { field = INVALID_TIMESTAMP_VALUE; }src/utils/config.ts (1)
134-134: Consider adding type validation for localStorage data.The type casting assumes that the object structure from localStorage matches
DecoderOptions. Consider adding runtime validation to ensure type safety.Example implementation:
function isValidDecoderOptions(value: unknown): value is DecoderOptions { return typeof value === 'object' && value !== null && 'formatString' in value && typeof value.formatString === 'string' && 'logLevelKey' in value && typeof value.logLevelKey === 'string' && 'timestampKey' in value && typeof value.timestampKey === 'string'; } // Usage in getConfig: const decoderOptions = { formatString: window.localStorage.getItem( LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING ), logLevelKey: window.localStorage.getItem( LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY ), timestampKey: window.localStorage.getItem( LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY ), }; if (!isValidDecoderOptions(decoderOptions)) { console.warn('Invalid decoder options in localStorage, using defaults'); return CONFIG_DEFAULT[CONFIG_KEY.DECODER_OPTIONS]; } return decoderOptions;src/services/decoders/JsonlDecoder/index.ts (1)
Line range hint
84-88: Consider using a more specific type for formatter options.While the change to
DecoderOptionsmaintains consistency, this method only uses theformatStringproperty. Consider creating a more focused type for formatter options to improve type safety and clarity.interface FormatterOptions { formatString: string; } setFormatterOptions(options: FormatterOptions): booleansrc/services/LogFileManager/index.ts (1)
113-113: Document breaking changes in public APIThe type changes in public methods (
createandsetFormatterOptions) represent breaking changes that could affect consumers. Please ensure these changes are:
- Documented in the changelog
- Reflected in the package version bump
- Migration guide provided if necessary
Also applies to: 141-141, 164-164
src/services/decoders/ClpIrDecoder.ts (1)
125-126: Avoid using the non-null assertion operatorIn
decodeRange, the non-null assertion operator!is used onthis.#formatter. While the method ensuresthis.#formatteris not null based onthis.#streamType, it's safer to avoid using!and instead use an explicit check or TypeScript's type guards to prevent potential runtime errors.Apply this diff to use an explicit null check:
- result[0] = this.#formatter!.formatLogEvent({ + if (this.#formatter !== null) { + result[0] = this.#formatter.formatLogEvent({ fields: fields, level: level, timestamp: dayJsTimestamp, }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.json(1 hunks)src/services/LogFileManager/index.ts(4 hunks)src/services/MainWorker.ts(2 hunks)src/services/decoders/ClpIrDecoder.ts(3 hunks)src/services/decoders/JsonlDecoder/index.ts(4 hunks)src/services/decoders/JsonlDecoder/utils.ts(1 hunks)src/typings/config.ts(2 hunks)src/typings/decoders.ts(3 hunks)src/typings/worker.ts(2 hunks)src/utils/config.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/services/MainWorker.ts
🔇 Additional comments (16)
src/typings/config.ts (2)
30-30: Verify handling of existing configurations.
The type change in ConfigMap from JsonlDecoderOptionsType to DecoderOptions might affect stored configurations. Please ensure that:
- Existing configurations in localStorage are compatible with the new type
- A migration strategy exists if needed
Let's check the usage of this configuration:
#!/bin/bash
# Description: Find configuration storage and loading logic
# Expect: Code that handles decoder options persistence
rg -A 5 "CONFIG_KEY.DECODER_OPTIONS"1-1: LGTM! The generic type name better reflects its broader usage.
The change from JsonlDecoderOptionsType to DecoderOptions is appropriate as it now represents options for multiple decoder types, including both JSONL and CLP IR streams.
Let's verify that all related imports have been updated consistently:
✅ Verification successful
Let me verify if there are any remaining references to JsonlDecoderOptionsType in the codebase.
The type renaming has been consistently applied across the codebase
The verification confirms that:
- No references to the old
JsonlDecoderOptionsTyperemain in the codebase - The new
DecoderOptionsinterface is properly defined indecoders.ts - All usages of the decoder options type now reference the new
DecoderOptionstype
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to JsonlDecoderOptionsType
# Expect: No occurrences of the old type name
rg "JsonlDecoderOptionsType"
Length of output: 4997
Script:
#!/bin/bash
# Search for any remaining references to JsonlDecoderOptionsType
rg "JsonlDecoderOptionsType"
# Also search for any references to the old type in decoders.ts
rg -A 5 "DecoderOptions" "src/typings/decoders.ts"
Length of output: 492
src/services/decoders/JsonlDecoder/utils.ts (1)
56-56: Verify dayjs compatibility with bigint timestamps
The function now accepts bigint timestamps, but we should verify that dayjs properly handles bigint values to prevent potential runtime issues.
Let's verify the dayjs bigint handling:
Also applies to: 61-62
✅ Verification successful
Let me verify the implementation details of the bigint handling to ensure it's properly set up.
Bigint timestamp handling is properly configured
The implementation is correctly set up with:
- The
dayjsBigIntSupportplugin is properly imported and configured inMainWorker.tsbefore any timestamp conversions - The
convertToDayjsTimestampfunction includes proper type checking for bigint values - The dayjs extension is properly initialized using
dayjs.extend(dayjsBigIntSupport)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if dayjsBigIntSupport plugin is properly imported and configured
# Expected: Find import and setup of dayjsBigIntSupport plugin
# Search for dayjsBigIntSupport plugin usage
rg -l "dayjsBigIntSupport"
# Search for dayjs plugin configuration
rg "dayjs\.(extend|use)"
Length of output: 248
Script:
#!/bin/bash
# Check the actual implementation of convertToDayjsTimestamp
rg -A 10 "convertToDayjsTimestamp.*=" src/services/decoders/JsonlDecoder/utils.ts
# Check if MainWorker initializes before any timestamp conversions
ast-grep --pattern 'class MainWorker {
$$$
constructor($$$) {
$$$
}
$$$
}'
Length of output: 784
src/typings/decoders.ts (3)
15-19: LGTM! Successfully consolidated decoder options.
The unified DecoderOptions interface effectively replaces the specific decoder option types as discussed, maintaining a clean and maintainable type structure.
84-84: LGTM! Method signature updated correctly.
The parameter type update to DecoderOptions maintains consistency with the type consolidation.
107-107: LGTM! Export properly added.
The DecoderOptions type is correctly exported, maintaining alphabetical ordering.
package.json (1)
33-33: Verify the compatibility with clp-ffi-js ^0.3.0
The upgrade from ^0.2.0 to ^0.3.0 may introduce breaking changes as this is a pre-1.0 version. Please ensure:
- All breaking changes are addressed in the codebase
- The package-lock.json is updated and committed
- The dependent PR Update the IR format protocol version to 0.0.1 #30 in clp-ffi-js is merged and released
Consider adding a comment in the package.json explaining the dependency on PR #30 to help future maintainers understand the version constraints.
✅ Verification successful
Let me gather more information about the breaking changes and their impact.
Package update verification completed successfully
The verification shows:
- package-lock.json is properly updated with version 0.3.0
- The dependency is used only in ClpIrDecoder.ts for structured CLP IR streams
- The integration is straightforward with proper initialization and usage patterns
The update appears safe to proceed with, considering:
- package-lock.json reflects the correct version
- Usage is well-encapsulated within a single service
- The implementation aligns with the PR's objective to support Structured CLP IR streams
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dependency update impact
# Test 1: Check if package-lock.json exists and contains the new version
echo "Checking package-lock.json..."
if [ -f "package-lock.json" ]; then
rg "clp-ffi-js.*0\.3\.0" package-lock.json
else
echo "Warning: package-lock.json not found"
fi
# Test 2: Find all imports of clp-ffi-js to verify usage
echo "Checking clp-ffi-js usage..."
rg "from ['\"]clp-ffi-js['\"]|require\(['\"]clp-ffi-js['\"]\)" -g '!{node_modules,dist}/**'
Length of output: 529
Script:
#!/bin/bash
# Check the actual usage of ClpStreamReader in the codebase
echo "Checking ClpStreamReader usage..."
rg "ClpStreamReader" -A 5 -B 5
# Look for any potential configuration or initialization code
echo "Checking clpFfiJsModuleInit usage..."
rg "clpFfiJsModuleInit" -A 5 -B 5
Length of output: 4082
src/typings/worker.ts (2)
4-4: LGTM! Type import update is consistent.
The renaming from DecoderOptionsType to DecoderOptions aligns with the broader refactoring effort and maintains proper import organization.
92-92: Verify worker communication type safety.
The type update looks correct. However, since this interface is crucial for worker communication, we should verify that all message handlers are updated accordingly.
✅ Verification successful
Type safety in worker communication is properly maintained
The verification shows that the DecoderOptions type is consistently used throughout the codebase:
- Properly defined in
src/typings/decoders.ts - Correctly used in worker message handling in
MainWorker.ts - Consistently implemented in both JsonlDecoder and ClpIrDecoder
- Properly passed through the configuration system
All message handlers and decoders are using the type correctly, maintaining type safety in the worker communication pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all worker message handlers use the new DecoderOptions type
# Test: Search for LOAD_FILE message handling to ensure type consistency
rg -A 5 "WORKER_REQ_CODE.LOAD_FILE"
# Test: Search for any remaining references to the old type
rg "DecoderOptionsType"
Length of output: 1273
Script:
#!/bin/bash
# Let's verify the DecoderOptions type usage and implementation
# Check the definition and usage of DecoderOptions
ast-grep --pattern 'type DecoderOptions = $_'
# Search for any decoder option related types and interfaces
rg -g '*.ts' -g '*.tsx' 'type.*Decoder|interface.*Decoder' -A 3
# Look for actual usage of decoder options in the codebase
rg -g '*.ts' -g '*.tsx' 'decoderOptions' -B 2 -A 2
Length of output: 7299
src/utils/config.ts (1)
9-9: LGTM: Import statement update is consistent with type standardization.
The change from DecoderOptionsType to DecoderOptions aligns with the broader refactoring effort.
src/services/decoders/JsonlDecoder/index.ts (3)
7-7: LGTM: Import statement is correctly updated.
The addition of DecoderOptions to the imports aligns with the type system changes.
28-30: LGTM: Documentation is clear and accurate.
The JSDoc comment properly references DecoderOptions and clearly explains the purpose of the decoder.
Line range hint 52-57: Verify type compatibility with existing code.
The constructor now uses DecoderOptions instead of JsonlDecoderOptionsType. While the implementation looks correct, we should verify that all required properties (logLevelKey, timestampKey, formatString) are properly defined in the new type.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have verified the type compatibility:
Type compatibility verified - change is safe
The DecoderOptions interface in src/typings/decoders.ts contains all the required properties (formatString, logLevelKey, timestampKey) that were previously used with JsonlDecoderOptionsType. Additionally, there are no remaining references to JsonlDecoderOptionsType in the codebase, confirming a complete migration to the new type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that DecoderOptions type contains all required properties
# and check for any direct usage of JsonlDecoderOptionsType in the codebase
# Check the definition of DecoderOptions
echo "Checking DecoderOptions definition:"
rg -A 10 "type DecoderOptions|interface DecoderOptions"
# Look for any remaining references to JsonlDecoderOptionsType
echo "Checking for remaining JsonlDecoderOptionsType references:"
rg "JsonlDecoderOptionsType"
Length of output: 829
src/services/LogFileManager/index.ts (2)
147-147: Verify ClpIrDecoder implementation compatibility
The ClpIrDecoder.create() call has been updated to use the new options type. Please ensure that the ClpIrDecoder class has been updated to handle the new DecoderOptions type correctly.
#!/bin/bash
# Check ClpIrDecoder implementation
ast-grep --pattern 'class ClpIrDecoder {
$$$
static create($_: $_) {
$$$
}
$$$
}'4-4: Verify removal of old type import
Please ensure that the old DecoderOptionsType import has been removed to prevent confusion and maintain clean imports.
✅ Verification successful
No old type import found, changes are clean
The search results confirm that:
- There are no remaining references to
DecoderOptionsTypein the codebase - The new
DecoderOptionsinterface is properly defined insrc/typings/decoders.ts - All usages of the type have been consistently updated across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to DecoderOptionsType
rg "DecoderOptionsType" --type ts
Length of output: 95
Script:
#!/bin/bash
# Let's check for any imports of DecoderOptionsType in the entire codebase
rg "import.*DecoderOptionsType" --type ts
# Also check for any type declarations or interfaces named DecoderOptionsType
ast-grep --pattern 'type DecoderOptionsType = $_'
ast-grep --pattern 'interface DecoderOptionsType $_'
# Let's also verify the new DecoderOptions type is properly used
rg "DecoderOptions" --type ts -A 2
Length of output: 4433
src/services/decoders/ClpIrDecoder.ts (1)
88-90:
Add null check for 'this.#formatter' in 'setFormatterOptions'
When streamType is UNSTRUCTURED, this.#formatter is set to null in the constructor. In setFormatterOptions, attempting to instantiate a new LogbackFormatter when this.#formatter is null may not be intended. Consider adding a null check to ensure that this.#formatter is not null before updating it.
Apply this diff to add the null check:
setFormatterOptions (options: DecoderOptions): boolean {
+ if (this.#formatter === null) {
+ return true;
+ }
this.#formatter = new LogbackFormatter({formatString: options.formatString});
return true;
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/services/decoders/ClpIrDecoder.ts (3)
16-19: Consider moving shared utilities to a common location.The utility functions
convertToDayjsTimestampandisJsonObjectare imported fromJsonlDecoder/utils. Since these are now used by multiple decoders, consider moving them to a shared utilities directory (e.g.,src/utils/decoders) to improve maintainability and reduce duplication.
49-51: Enhance method documentation.The JSDoc comment could be more descriptive about the
decoderOptionsparameter. Consider documenting the expected properties and their impact on the decoder's behaviour.
98-133: Consider using immutable operations in decodeRange.The current implementation mutates the result array by directly modifying
result[0]. Consider creating a new array with transformed results to improve code maintainability and reduce potential side effects.Apply this diff:
- results.forEach((result) => { + return results.map((result) => { const [ message, timestamp, level, ] = result; const dayJsTimestamp: Dayjs = convertToDayjsTimestamp(timestamp); let fields: JsonObject = {}; try { fields = JSON.parse(message) as JsonObject; if (false === isJsonObject(fields)) { throw new Error("Unexpected non-object."); } } catch (e) { console.error(e, message); } - // `this.#formatter` has been null-checked at the method entry. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - result[0] = this.#formatter!.formatLogEvent({ + return [ + this.#formatter!.formatLogEvent({ fields: fields, level: level, timestamp: dayJsTimestamp, - }); + }), + result[1], + result[2] + ] as DecodeResult; - }); - - return results; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/services/decoders/ClpIrDecoder.ts(2 hunks)src/services/decoders/JsonlDecoder/index.ts(7 hunks)src/typings/decoders.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/services/decoders/JsonlDecoder/index.ts
- src/typings/decoders.ts
🔇 Additional comments (2)
src/services/decoders/ClpIrDecoder.ts (2)
22-25: LGTM! Clear and well-defined enum.
The enum effectively distinguishes between structured and unstructured stream types with self-documenting values.
34-43:
Add validation for decoderOptions.formatString.
When initializing the formatter for structured streams, ensure decoderOptions.formatString is defined to prevent potential runtime errors.
Apply this diff:
- this.#formatter = (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) ?
- new LogbackFormatter({formatString: decoderOptions.formatString}) :
- null;
+ this.#formatter = (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) ?
+ new LogbackFormatter({
+ formatString: decoderOptions.formatString ?? '%d{ISO8601} [%p] %m%n'
+ }) :
+ null;Likely invalid or redundant comment.
| } | ||
|
|
||
| /** | ||
| * Creates a new ClpIrDecoder instance. | ||
| * | ||
| * @param dataArray The input data array to be passed to the decoder. | ||
| * @param decoderOptions The options are only effective if the stream type is |
There was a problem hiding this comment.
cant suggest but
/**
* Creates a new ClpIrDecoder instance.
*
* @param dataArray The input data array to be passed to the decoder.
* @param decoderOptions
* @return The created ClpIrDecoder instance.
*
* NOTE: `decoderOptions` only affects decode results if the stream type is
* type is {@link CLP_IR_STREAM_TYPE.STRUCTURED}.
*/
There was a problem hiding this comment.
I moved the NOTE above the parameter list just so jsdoc doesn't get confused by thinking it's part of the "return".
| console.error(e, message); | ||
| } | ||
|
|
||
| // `this.#formatter` has been null-checked at the method entry. |
There was a problem hiding this comment.
The for of loop fixed this weird typescript error, but if you like for each better, I think they are similiar.
There was a problem hiding this comment.
Right, having the check in the callback confuses TS. In favour of getting rid of the eslint override i agree it's better to use a for-loop.
| @@ -53,12 +53,13 @@ const convertToLogLevelValue = (field: JsonValue | undefined): LOG_LEVEL => { | |||
| * - the timestamp's value is an unsupported type. | |||
| * - the timestamp's value is not a valid dayjs timestamp. | |||
| */ | |||
| const convertToDayjsTimestamp = (field: JsonValue | undefined): dayjs.Dayjs => { | |||
| const convertToDayjsTimestamp = (field: JsonValue | bigint | undefined): dayjs.Dayjs => { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Also how do u feel about this file to utils? Since it is shared by both IR and Json now?
There was a problem hiding this comment.
right that's an issue... Just to understand why we need the mitigation, where are we using json.parse with BigInts?
There was a problem hiding this comment.
Potentially also in json decoder if logs have an bigint?
There was a problem hiding this comment.
omg...
nlohmann/jsonacknowledges there are implementation whose number types can store up to 2^53-1 : https://json.nlohmann.me/features/types/number_handling/#number-interoperability- yet it does serialize numbers out of that range, imo for good reasons though: https://github.com/nlohmann/json/blob/4a602df34e71afca2242b34c743d4cc2486cd071/tests/src/unit-serialization.cpp#L168-L183
funny finding: https://www.github.com/nlohmann/json/issues/1708 -> https://www.github.com/nlohmann/json/pull/1722
Let's discuss the issue with the clp core people before we make changes.
There was a problem hiding this comment.
As discussed offline, calling JSON.parse() with any clp-ffi-js-returned JSON string containing number in +/- 2^53-1 ~ 2^64-1 would encounter loss of precision, but luckily nothing throws.
Let's address the loss of precision issue in another PR.
There was a problem hiding this comment.
To be tracked at #116
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/services/decoders/ClpIrDecoder.ts (5)
22-25: Add JSDoc documentation for the CLP_IR_STREAM_TYPE enum.Consider adding documentation to explain the purpose and usage of each stream type for better maintainability.
+/** + * Enum representing the types of CLP IR streams that can be processed. + * @enum {string} + */ enum CLP_IR_STREAM_TYPE { + /** Structured logs containing JSON-formatted messages */ STRUCTURED = "structured", + /** Unstructured logs containing plain text messages */ UNSTRUCTURED = "unstructured", }
34-43: Add parameter validation in the constructor.The constructor should validate its parameters to ensure robust initialization.
constructor ( streamType: CLP_IR_STREAM_TYPE, streamReader: ClpStreamReader, decoderOptions: DecoderOptions ) { + if (!Object.values(CLP_IR_STREAM_TYPE).includes(streamType)) { + throw new Error(`Invalid stream type: ${streamType}`); + } + if (!streamReader) { + throw new Error('StreamReader is required'); + } this.#streamType = streamType; this.#streamReader = streamReader; this.#formatter = (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) ? new LogbackFormatter({formatString: decoderOptions.formatString}) : null; }
89-91: Add validation in setFormatterOptions method.The method should validate the formatString option before creating a new formatter.
setFormatterOptions (options: DecoderOptions): boolean { + if (!options?.formatString) { + console.error('Invalid formatter options: formatString is required'); + return false; + } this.#formatter = new LogbackFormatter({formatString: options.formatString}); return true; }
103-111: Enhance error handling for missing formatter.The error message could be more descriptive and include troubleshooting information.
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."); + console.error( + "Formatter is not set for structured logs. " + + "Please ensure setFormatterOptions is called before decoding structured logs." + ); } return results; }
122-129: Improve error handling for JSON parsing.Consider adding more context to the error message and handling specific JSON parsing errors differently.
try { fields = JSON.parse(message) as JsonObject; if (false === isJsonObject(fields)) { throw new Error("Unexpected non-object."); } } catch (e) { - console.error(e, message); + if (e instanceof SyntaxError) { + console.error(`Invalid JSON format in message: ${message}`, e); + } else { + console.error(`Error processing message: ${message}`, e); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/services/decoders/ClpIrDecoder.ts(2 hunks)
🔇 Additional comments (1)
src/services/decoders/ClpIrDecoder.ts (1)
1-19: LGTM! Imports are well-organized and properly utilized.
The imports are logically grouped and all imported entities are used within the implementation.
junhaoliao
changed the title
clp-ffi-js to 0.3.0; Refactor ClpIrDecoder to also support Structured CLP IR streams but without log-level filtering.
References
https://github.com/y-scope/clp-ffi-js/releases/tag/v0.3.0 added support for decoding Structured CLP IR streams but without log-level filtering. In the decoded results returned by the reader, the message contains a complete JSON-serialized log event. Before display the log events in the log viewer, we can use the existing formatter for JSON logs to format the log events to improve readbility.
Description
clp-ffi-jsto0.3.0.Validation performed
Summary by CodeRabbit
New Features
clp-ffi-jsto enhance performance and stability.bigintin timestamp handling for improved data processing.Improvements
ClpIrDecoderfor more robust data parsing.Bug Fixes
Documentation