feat!: Add support for filtering with hierarchal and auto-generated keys. #199
Conversation
WalkthroughThis pull request updates the dependency version in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ClpIrDecoder
participant parseFilterKeys
participant ClpStreamReader
Caller->>ClpIrDecoder: Instantiate with decoderOptions
ClpIrDecoder->>parseFilterKeys: Process decoderOptions (flag: true)
parseFilterKeys-->>ClpIrDecoder: Return processed readerOptions
ClpIrDecoder->>ClpStreamReader: Initialize with readerOptions
sequenceDiagram
participant Caller
participant YscopeFormatter
participant parseKey
Caller->>YscopeFormatter: Parse field placeholder
YscopeFormatter->>parseKey: Call with fieldName
parseKey-->>YscopeFormatter: Return ParsedKey object
YscopeFormatter->>YscopeFormatter: Validate and process ParsedKey
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.json(1 hunks)src/services/decoders/ClpIrDecoder/index.ts(2 hunks)src/services/decoders/JsonlDecoder/index.ts(5 hunks)src/services/decoders/utils.ts(1 hunks)src/services/formatters/YscopeFormatter/index.ts(3 hunks)src/services/formatters/YscopeFormatter/utils.ts(6 hunks)src/typings/formatters.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/services/formatters/YscopeFormatter/index.tssrc/services/decoders/ClpIrDecoder/index.tssrc/services/decoders/utils.tssrc/typings/formatters.tssrc/services/decoders/JsonlDecoder/index.tssrc/services/formatters/YscopeFormatter/utils.ts
🪛 GitHub Check: lint-check
src/services/decoders/ClpIrDecoder/index.ts
[failure] 29-29:
Imports must not be broken into multiple lines if there are 1 or less elements
[failure] 29-29:
Unexpected line break after this opening brace
[failure] 31-31:
Unexpected line break before this closing brace
src/services/decoders/utils.ts
[failure] 1-1:
Run autofix to sort these imports!
[failure] 9-9:
Imports must not be broken into multiple lines if there are 1 or less elements
src/services/decoders/JsonlDecoder/index.ts
[failure] 27-27:
Imports must not be broken into multiple lines if there are 1 or less elements
[failure] 27-27:
Unexpected line break after this opening brace
[failure] 29-29:
Unexpected line break before this closing brace
🪛 ESLint
src/services/decoders/ClpIrDecoder/index.ts
[error] 29-31: Imports must not be broken into multiple lines if there are 1 or less elements.
(import-newlines/enforce)
[error] 29-29: Unexpected line break after this opening brace.
(@stylistic/object-curly-newline)
[error] 31-31: Unexpected line break before this closing brace.
(@stylistic/object-curly-newline)
src/services/decoders/utils.ts
[error] 9-11: Imports must not be broken into multiple lines if there are 1 or less elements.
(import-newlines/enforce)
[error] 9-9: Unexpected line break after this opening brace.
(@stylistic/object-curly-newline)
[error] 11-11: Unexpected line break before this closing brace.
(@stylistic/object-curly-newline)
[error] 45-45: Expected '===' and instead saw '=='.
(eqeqeq)
src/services/decoders/JsonlDecoder/index.ts
[error] 27-29: Imports must not be broken into multiple lines if there are 1 or less elements.
(import-newlines/enforce)
[error] 27-27: Unexpected line break after this opening brace.
(@stylistic/object-curly-newline)
[error] 29-29: Unexpected line break before this closing brace.
(@stylistic/object-curly-newline)
🪛 GitHub Actions: lint
src/services/decoders/ClpIrDecoder/index.ts
[warning] 1-1: Run autofix to sort these imports! (simple-import-sort/imports)
🔇 Additional comments (19)
package.json (1)
33-33: Dependency version update looks appropriate.The update from
^0.4.0to^0.5.0for theclp-ffi-jslibrary aligns with the PR's objective to add support for hierarchal and autogenerated keys. According to semantic versioning, this minor version update should introduce new features without breaking existing functionality.src/services/formatters/YscopeFormatter/index.ts (2)
6-6: Type import looks good.The import of
ParsedKeytype is consistent with the renaming fromParsedFieldNametoParsedKeyin the typings file.
16-16: Function import is appropriate.Adding the import for
parseKeyfunction is necessary for the new implementation of the field placeholder parsing.src/services/decoders/ClpIrDecoder/index.ts (1)
48-49: Implementation of filter key parsing looks good.The code correctly uses the new
parseFilterKeysfunction to preprocess decoder options before passing them to theClpStreamReader. This enables support for hierarchal and autogenerated keys as intended in the PR.src/typings/formatters.ts (4)
62-64: Updated documentation is clear and accurate.The comment for the
ParsedKeytype has been appropriately updated to reflect its broader usage for both format strings and filter keys.
68-71: Type rename is well-implemented.Renaming from
ParsedFieldNametoParsedKeybetter represents the type's purpose, especially now that it's used for both format string fields and filter keys.
77-77: Property type reference updated correctly.The property type reference in
YscopeFieldPlaceholderhas been correctly updated to use the renamedParsedKeytype.
127-127: Export update is consistent.The export statement has been updated to use the new type name, maintaining consistency throughout the codebase.
src/services/decoders/utils.ts (3)
13-28: Function looks good!The
preprocessThenParseFilterKeyfunction properly handles the preprocessing of filter keys, including warning about Unicode replacement characters and removing escaped backslashes.
38-57: Functionality looks good!The
parseFilterKeysfunction correctly handles the validation of auto-generated keys based on decoder support, throwing appropriate errors when necessary.🧰 Tools
🪛 ESLint
[error] 45-45: Expected '===' and instead saw '=='.
(eqeqeq)
59-62: Export statement looks good!The export statement properly includes both utility functions.
src/services/decoders/JsonlDecoder/index.ts (3)
41-43: Property type changes look good!The properties have been correctly updated from strings to arrays to support hierarchical key access.
60-63: Implementation of filter key parsing looks good!The constructor now properly uses the
parseFilterKeysutility to parse and extract key parts, passingfalseforsupportsAutoGeneratedKeysto ensure proper validation for JSONL logs.
175-182: Improved nested JSON value retrievalThe implementation now correctly uses
getNestedJsonValuewith the key parts arrays, allowing for hierarchical key access in JSON objects.src/services/formatters/YscopeFormatter/utils.ts (5)
6-6: Type rename looks good!The change from
ParsedFieldNametoParsedKeyis consistent with the PR objectives of supporting hierarchical keys.
84-84: Documentation update looks good!The JSDoc comment has been updated to reflect the new terminology.
96-96: Parameter type update looks good!The function parameter has been correctly updated to use the
ParsedKeytype.
165-165: Return type update looks good!The
parseKeyfunction's return type has been updated toParsedKeyfor consistency.
179-219: Documentation and return structure updates look good!The JSDoc comments and return object structure have been updated to reflect the new naming conventions and simplify the returned object.
| const {fieldName, formatterName, formatterOptions} = | ||
| splitFieldPlaceholder(groupMatch); | ||
|
|
||
| const parsedFieldName: ParsedKey = parseKey(fieldName); | ||
| if (null === this.#structuredIrNamespaceKeys && ParsedKey.isAutoGenerated) { | ||
| throw new Error( | ||
| "`@` is a reserved symbol and must be escaped with `\\` " + | ||
| "for JSONL logs." | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix reference to ParsedKey type.
The condition on line 99 incorrectly references ParsedKey.isAutoGenerated where ParsedKey is a type, not an object. This should be checking the property on the instance.
Apply this diff to fix the type reference:
- if (null === this.#structuredIrNamespaceKeys && ParsedKey.isAutoGenerated) {
+ if (null === this.#structuredIrNamespaceKeys && parsedFieldName.isAutoGenerated) {📝 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 {fieldName, formatterName, formatterOptions} = | |
| splitFieldPlaceholder(groupMatch); | |
| const parsedFieldName: ParsedKey = parseKey(fieldName); | |
| if (null === this.#structuredIrNamespaceKeys && ParsedKey.isAutoGenerated) { | |
| throw new Error( | |
| "`@` is a reserved symbol and must be escaped with `\\` " + | |
| "for JSONL logs." | |
| ); | |
| } | |
| const {fieldName, formatterName, formatterOptions} = | |
| splitFieldPlaceholder(groupMatch); | |
| const parsedFieldName: ParsedKey = parseKey(fieldName); | |
| if (null === this.#structuredIrNamespaceKeys && parsedFieldName.isAutoGenerated) { | |
| throw new Error( | |
| "`@` is a reserved symbol and must be escaped with `\\` " + | |
| "for JSONL logs." | |
| ); | |
| } |
| class ClpIrDecoder implements Decoder { | ||
| #streamReader: ClpStreamReader; | ||
|
|
||
| readonly #streamType: CLP_IR_STREAM_TYPE; | ||
|
|
||
| readonly #structuredIrNamespaceKeys: StructuredIrNamespaceKeys; | ||
|
|
||
| #formatter: Nullable<Formatter> = null; | ||
|
|
||
| constructor ( | ||
| ffiModule: MainModule, | ||
| dataArray: Uint8Array, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix import formatting to comply with code style.
The import statement for parseFilterKeys violates the project's import formatting guidelines. Imports with a single element should not be broken into multiple lines.
Apply this diff to fix the import formatting:
-import {
- parseFilterKeys,
-} from "../utils";
+import { parseFilterKeys } from "../utils";📝 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.
| import { | |
| parseFilterKeys, | |
| } from "../utils"; | |
| import { parseFilterKeys } from "../utils"; |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 29-29:
Imports must not be broken into multiple lines if there are 1 or less elements
[failure] 29-29:
Unexpected line break after this opening brace
[failure] 31-31:
Unexpected line break before this closing brace
🪛 ESLint
[error] 29-31: Imports must not be broken into multiple lines if there are 1 or less elements.
(import-newlines/enforce)
[error] 29-29: Unexpected line break after this opening brace.
(@stylistic/object-curly-newline)
[error] 31-31: Unexpected line break before this closing brace.
(@stylistic/object-curly-newline)
src/services/decoders/utils.ts
Outdated
| if (false == supportsAutoGeneratedKeys && | ||
| (parsedLogLevelKey.isAutoGenerated || parsedTimestampKey.isAutoGenerated)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use strict equality operator
Use === instead of == for equality comparisons as per the project's coding guidelines.
Apply this diff to fix the equality comparison:
- if (false == supportsAutoGeneratedKeys &&
+ if (false === supportsAutoGeneratedKeys &&📝 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.
| if (false == supportsAutoGeneratedKeys && | |
| (parsedLogLevelKey.isAutoGenerated || parsedTimestampKey.isAutoGenerated)) { | |
| if (false === supportsAutoGeneratedKeys && | |
| (parsedLogLevelKey.isAutoGenerated || parsedTimestampKey.isAutoGenerated)) { |
🧰 Tools
🪛 ESLint
[error] 45-45: Expected '===' and instead saw '=='.
(eqeqeq)
src/services/decoders/utils.ts
Outdated
| import { | ||
| ParsedKey, | ||
| REPLACEMENT_CHARACTER, | ||
| } from "../../typings/formatters"; | ||
| import { | ||
| parseKey, | ||
| replaceDoubleBacklash, | ||
| } from "../formatters/YscopeFormatter/utils"; | ||
| import { | ||
| DecoderOptions, | ||
| } from "../../typings/decoders"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sort imports according to linting rules
The imports in this file need to be sorted, and imports with single elements should not be broken into multiple lines.
Apply this diff to fix the import formatting:
-import {
- ParsedKey,
- REPLACEMENT_CHARACTER,
-} from "../../typings/formatters";
-import {
- parseKey,
- replaceDoubleBacklash,
-} from "../formatters/YscopeFormatter/utils";
-import {
- DecoderOptions,
-} from "../../typings/decoders";
+import { DecoderOptions } from "../../typings/decoders";
+import { ParsedKey, REPLACEMENT_CHARACTER } from "../../typings/formatters";
+import { parseKey, replaceDoubleBacklash } from "../formatters/YscopeFormatter/utils";📝 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.
| import { | |
| ParsedKey, | |
| REPLACEMENT_CHARACTER, | |
| } from "../../typings/formatters"; | |
| import { | |
| parseKey, | |
| replaceDoubleBacklash, | |
| } from "../formatters/YscopeFormatter/utils"; | |
| import { | |
| DecoderOptions, | |
| } from "../../typings/decoders"; | |
| import { DecoderOptions } from "../../typings/decoders"; | |
| import { ParsedKey, REPLACEMENT_CHARACTER } from "../../typings/formatters"; | |
| import { parseKey, replaceDoubleBacklash } from "../formatters/YscopeFormatter/utils"; |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 1-1:
Run autofix to sort these imports!
[failure] 9-9:
Imports must not be broken into multiple lines if there are 1 or less elements
🪛 ESLint
[error] 9-11: Imports must not be broken into multiple lines if there are 1 or less elements.
(import-newlines/enforce)
[error] 9-9: Unexpected line break after this opening brace.
(@stylistic/object-curly-newline)
[error] 11-11: Unexpected line break before this closing brace.
(@stylistic/object-curly-newline)
| /** | ||
| * A decoder for JSONL (JSON lines) files that contain log events. See `DecoderOptions` for | ||
| * properties that are specific to log events (compared to generic JSON records). | ||
| */ | ||
| class JsonlDecoder implements Decoder { | ||
| static #textDecoder = new TextDecoder(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix import formatting
The import statement for parseFilterKeys should be on a single line since it's importing only one element.
Apply this diff to fix the import formatting:
-import {
- parseFilterKeys,
-} from "../utils";
+import { parseFilterKeys } from "../utils";📝 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.
| import { | |
| parseFilterKeys, | |
| } from "../utils"; | |
| import { parseFilterKeys } from "../utils"; |
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 27-27:
Imports must not be broken into multiple lines if there are 1 or less elements
[failure] 27-27:
Unexpected line break after this opening brace
[failure] 29-29:
Unexpected line break before this closing brace
🪛 ESLint
[error] 27-29: Imports must not be broken into multiple lines if there are 1 or less elements.
(import-newlines/enforce)
[error] 27-27: Unexpected line break after this opening brace.
(@stylistic/object-curly-newline)
[error] 29-29: Unexpected line break before this closing brace.
(@stylistic/object-curly-newline)
| */ | ||
| type YscopeFieldPlaceholder = { | ||
| parsedFieldName: ParsedFieldName; | ||
| parsedFieldName: ParsedKey; |
There was a problem hiding this comment.
just curious, how do we differentiate between a parsedKey and a parsedFieldName after this type renaming?
There was a problem hiding this comment.
We discussed offline:
The concept of "field names" are usually referenced in the context of field formatters / formatter strings. The docs also references this part of the specifier as <field-name>. For field formatters, it is fine to leave the name as-is. We can rename the field in a future refactoring PR if the name also changes in the docs.
| * @param filterKey | ||
| * @return The parsed key. | ||
| */ | ||
| const preprocessThenParseFilterKey = (filterKey: string): ParsedKey => { |
There was a problem hiding this comment.
(no need to address this right now - we can revisit in a future PR. we can file an issue if you also agree
With my recent experiences, i believe the Unicode Replacement character is used for replacing sequences that can't be decoded as UTF-8 characters, which means it may have a higher chance of occurrence than any other Unicode characters, depending on how users generate IRv2 logs.
In that case, we may want use a different character for replacement.
anyways, let's make sure this behaviour is covered when we put up the docs
There was a problem hiding this comment.
I see your point. tbh i only chose Unicode Replacement character because i thought the name was fitting. Note however, the error will only trigger on the keys not the values, which are probably human generated, so less likely to encounter replacement character.
Also, i think we want to get rid of this eventually with antler parser. With all that being said, if you want to change it to something else, we just need to change the character in the code in one place and replace the function docstrings in a few places.
There was a problem hiding this comment.
the error will only trigger on the keys not the values, which are probably human generated, so less likely to use replacement character.
fair. a rare example of how this would happen is that people can write SHIFT-JIS Japanese in the keys and the logging framework / text editor decides to encode the log events as only UTF-8 in the logs. Then to select the key in the log viewer as a level / timestamp key, they would put the key partially (or fully) containing the unicode replacement character. again, i agree this is a multi-point failure, should be really rare, and for the existing use cases we're serving, likely we won't encounter such issues.
we want to get rid of this eventually with antler parser
yeah, hopefully we won't really see odd use cases before we migrate there
src/services/decoders/utils.ts
Outdated
| "`@` is a reserved symbol and must be escaped with `\\` " + | ||
| "for JSONL logs." |
There was a problem hiding this comment.
how about (need to reflow if there's line length violation)
| "`@` is a reserved symbol and must be escaped with `\\` " + | |
| "for JSONL logs." | |
| "`@` is a reserved symbol for CLP IR logs and must be escaped by `\\` " + | |
| "for JSONL logs." |
| "`@` is a reserved symbol and must be escaped with `\\` " + | ||
| "for JSONL logs." |
There was a problem hiding this comment.
similar to the one thrown at parseFilterKeys(), i believe mentioning "CLP IR" helps with the context.
shall we consider making this message a const so it can be reused across multiple places?
There was a problem hiding this comment.
I'm okay with adding CLP IR and I think we can define it here, then import to other location.
|
I made small change please look at last commit |
davemarco
changed the title
|
@junhaoliao - reminder to reapprove, i made a very minor tweak in a newer commit |
2 participants
Description
Add support for filtering with hierarchal and autogenerated keys.
To do this we upgrade to latest clp-ffi-js 0.5.0, and pass the new parsed hierarchal reader options.
In addition we modify the json decoder, to also support hierarchal filter keys.
Checklist
breaking change.
Validation performed
I performed basic validation and checked filtering for IRV2 and json with hierarchal and autogenerated keys.
Summary by CodeRabbit
Chores
clp-ffi-jsdependency for improved performance and functionality.New Features
Refactor