feat(formatter): Add YScope formatter for structured logs and remove Logback-style formatter. #123
Conversation
WalkthroughThis pull request introduces multiple changes across several files, primarily focusing on the transition from Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@davemarco has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
davemarco
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (23)
src/utils/formatters.ts (1)
1-9: Documentation should list supported patternsThe documentation mentions that not all patterns are supported but doesn't specify which ones are. Consider enhancing the documentation to:
- List all supported patterns and their Day.js equivalents
- Add examples of valid conversions
- Document what happens with unsupported patterns
/** * Converts a *simple* java.time.format.DateTimeFormatter pattern to a Day.js date format string. * * NOTE: This method doesn't handle all possible patterns. Check the implementation to determine * what's supported. * + * Supported patterns: + * - yyyy -> YYYY (4-digit year) + * - yy -> YY (2-digit year) + * - dd/d -> D (day of month) + * + * Example: + * Input: "yyyy-MM-dd" + * Output: "YYYY-MM-D" * * @param pattern * @return The corresponding Day.js date format string. */src/utils/js.ts (2)
7-14: Consider enhancing the documentation with edge cases.The documentation is clear and includes a good example. Consider adding:
- Edge case examples (e.g., empty keys array, non-existent paths)
- Return value examples
/** * Gets a nested value from a JSON object. * * @param fields The JSON object. * @param keys An array of keys representing the path to the nested value. E.g., the field * `{"a:" {"b": 0}}` would be accessed using array `["a", "b"]` + * @example + * // Returns 0 + * getNestedJsonValue({"a": {"b": 0}}, ["a", "b"]) + * // Returns undefined + * getNestedJsonValue({"a": {"b": 0}}, ["a", "c"]) + * // Returns undefined + * getNestedJsonValue({"a": {"b": 0}}, []) * @return The nested value if found, otherwise `undefined`. */
15-28: Consider optimizing the implementation for better type safety and efficiency.While the implementation is functional, there are opportunities for improvement:
const getNestedJsonValue = (fields: JsonObject, keys: string[]): JsonValue | undefined => { + if (keys.length === 0) { + return fields; + } - let result: JsonValue | undefined = fields; + let result = fields as JsonObject; for (const key of keys) { if ("object" !== typeof result || null === result || Array.isArray(result)) { - // `undefined` seems natural as return value for this function since matches - // js behaviour. - // eslint-disable-next-line no-undefined - return undefined; + return; // implicit undefined return is cleaner } result = result[key]; } return result; };The changes above:
- Add an early return for empty keys array
- Improve type safety with explicit casting
- Remove the need for ESLint disable comment
src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts (3)
7-11: Consider enhancing documentation with examplesThe documentation clearly describes the purpose but would benefit from concrete examples showing:
- Input/output for numerical values
- Behaviour with non-numerical values
- Error cases with options
Add examples like:
/** * A field formatter that rounds numerical values to the nearest integer. * If the field value is not a number, it is returned as-is after being * converted to a string. Does not currently support any options. + * + * @example + * // Numerical values + * formatField(3.7) => "4" + * formatField(-2.3) => "-2" + * + * // Non-numerical values + * formatField("hello") => "hello" + * formatField({"key": "value"}) => "{\"key\":\"value\"}" + * + * // Error cases + * new RoundFormatter("precision=2") => Error: Round formatter does not support option precision=2 */
13-17: Enhance error handling in constructorThe current error handling could be more informative and specific.
Consider this improvement:
constructor (options: Nullable<string>) { if (null !== options) { - throw Error(`Round formatter does not support option ${options}`); + throw new Error( + `Round formatter does not support any options, but received: "${options}". ` + + 'This formatter only rounds numbers to the nearest integer.' + ); } }
19-20: Consider making formatField a static methodThe eslint-disable comment suggests this method doesn't use instance properties. Consider making it static since the formatter is stateless.
- // eslint-disable-next-line class-methods-use-this - formatField (field: JsonValue): string { + static formatField (field: JsonValue): string {src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts (1)
18-22: Consider adding pattern validation and simplifying the null check.The constructor could benefit from two improvements:
- Validate the result of
convertDateTimeFormatterPatternToDayJsto ensure it returns a valid pattern- Simplify the null check using optional chaining
Consider this improvement:
constructor (options: Nullable<string>) { - if (null !== options) { - this.#dateFormat = convertDateTimeFormatterPatternToDayJs(options); + const convertedFormat = options && convertDateTimeFormatterPatternToDayJs(options); + if (convertedFormat) { + this.#dateFormat = convertedFormat; } }src/services/formatters/YScopeFormatter/utils.ts (5)
13-19: Enhance documentation for YSCOPE_FORMATTERS_MAPThe current documentation could be more descriptive. Consider adding details about each supported formatter and their purposes.
/** * List of currently supported field formatters. + * + * @property {typeof TimestampFormatter} timestamp - Formats timestamp values according to specified patterns + * @property {typeof RoundFormatter} round - Rounds numerical values to the nearest integer */
21-26: Complete the JSDoc documentationThe function's documentation is missing parameter and return type descriptions.
/** * Converts a JSON value to its string representation. * - * @param input - * @return + * @param {JsonValue | undefined} input - The JSON value to convert + * @return {string} The string representation of the input */
27-32: Consider explicit null handlingThe current implementation might not handle
nullvalues optimally. Consider adding explicit null handling for clarity.const jsonValueToString = (input: JsonValue | undefined): string => { - // Behaviour is different for `undefined`. + if (input === null || input === undefined) { + return String(input); + } return "object" === typeof input ? JSON.stringify(input) : String(input); };
34-40: Fix JSDoc parameter documentationThe JSDoc has mismatched parameter names and missing declarations.
/** * Validates a component string. * - * @param subfield + * @param {string | undefined} component - The component string to validate * @return {Nullable<string>} The component string if valid, or `null` if the component is undefined or empty. */🧰 Tools
59-88: Add input validationConsider adding input validation for the fieldPlaceholder parameter to handle empty or malformed input gracefully.
const splitFieldPlaceholder = (fieldPlaceholder: string): { fieldNameKeys: string[], formatterName: Nullable<string>, formatterOptions: Nullable<string>, } => { + if (!fieldPlaceholder || typeof fieldPlaceholder !== 'string') { + throw new Error('Invalid field placeholder: must be a non-empty string'); + } + let [fieldName, formatterName, formatterOptions]: Nullable<string|undefined>[] = fieldPlaceholder.split(COLON_REGEX, 3);src/services/formatters/LogbackFormatter.ts (1)
Line range hint
103-104: Critical: Address potential format string vulnerability.The TODO comment highlights a significant issue where variable values containing '%' could be misinterpreted as format specifiers, leading to incorrect log formatting. This should be addressed to ensure reliable log output.
Consider applying this fix:
#formatVariables (formatString: string, logEvent: JsonObject): string { - // eslint-disable-next-line no-warning-comments - // TODO These don't handle the case where a variable value may contain a '%' itself for (const key of this.#keys) { if (false === (key in logEvent)) { continue; } const specifier = `%${key}`; const value = logEvent[key]; const valueStr = "object" === typeof value ? JSON.stringify(value) : - String(value); + String(value).replace(/%/g, '%%'); formatString = formatString.replace(specifier, valueStr); }This solution escapes '%' characters in variable values by doubling them, which is a common approach in format string handling.
src/services/decoders/ClpIrDecoder.ts (2)
Line range hint
121-136: Improve error handling and type safety in JSON parsingThe current implementation has several areas for improvement:
- JSON parsing errors are only logged to console
- The fields object lacks proper typing
- Error details might be lost in the catch block
Consider this improved implementation:
interface LogFields extends JsonObject { // Add expected field types here } try { const parsedFields = JSON.parse(message); if (!isJsonObject(parsedFields)) { throw new Error('Parsed message is not a valid JSON object'); } fields = parsedFields as LogFields; } catch (e) { fields = { error: 'Failed to parse message', raw: message }; console.error('JSON parsing error:', e instanceof Error ? e.message : String(e)); }🧰 Tools
🪛 GitHub Check: lint-check
[failure] 15-15:
Unable to resolve path to module '../../formatters/YScopeFormatter'
Line range hint
25-44: Consider decoupling formatter initializationThe current implementation tightly couples the decoder with YscopeFormatter initialization. Consider implementing a formatter factory or dependency injection pattern to make the decoder more flexible and testable.
Example approach:
interface FormatterFactory { createFormatter(options: FormatterOptions): Formatter; } class ClpIrDecoder { constructor( streamType: CLP_IR_STREAM_TYPE, streamReader: ClpStreamReader, private formatterFactory: FormatterFactory, decoderOptions: DecoderOptions ) { // ... this.#formatter = this.createFormatter(decoderOptions); } private createFormatter(options: DecoderOptions): Nullable<Formatter> { return this.#streamType === CLP_IR_STREAM_TYPE.STRUCTURED ? this.formatterFactory.createFormatter(options) : null; } }Also applies to: 89-91
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 41-41:
Unsafe assignment of an error typed value
[failure] 42-42:
Unsafe construction of an any type valuesrc/components/modals/SettingsModal/SettingsDialog.tsx (1)
36-40: Enhance the helper text clarity and completeness.While the helper text effectively explains the basic syntax, consider these improvements for better user guidance:
- Show the default pattern as a concrete example
- Add examples for common formatter types (timestamp and rounding) mentioned in the PR
- Format the text for better readability
Here's a suggested improvement:
- helperText: `[JSON] Log messages conversion pattern. Add field-placeholders to insert - fields from JSON log events. A field-placeholder uses the following syntax: - \`{<field-name>[:<formatter-name>[:<formatter-options>]]}\`. \`field-name\` is required, - while \`formatter-name\` and \`formatter-options\` are optional. See the default pattern - for an example.`, + helperText: `[JSON] Log messages conversion pattern using field-placeholders. + + Syntax: \`{<field-name>[:<formatter-name>[:<formatter-options>]]}\` + • field-name: Required. The JSON field to extract (e.g., 'message', 'level') + • formatter-name: Optional. Available formatters: 'timestamp', 'round' + • formatter-options: Optional. Formatter-specific settings + + Examples: + • Basic field: {message} + • Timestamp: {timestamp:timestamp:iso} + • Rounded number: {value:round:2}`,src/typings/formatters.ts (3)
40-40: Grammatical correction needed for clarity.Please revise the sentence for clarity and grammatical correctness.
Suggested correction:
- * - To denote a field-name with periods escape the periods with a backslashes. + * - To denote a field-name with periods, escape the periods with backslashes.
94-94: Typo in comment: 'backlash' should be 'backslash'.There is a typographical error in the comment.
Suggested correction:
- * Pattern to remove backlash. + * Pattern to remove backslash.
96-111: Consider adding unit tests for regex patterns to ensure correctness.The regex patterns defined are critical for parsing the Yscope format strings. It is advisable to add unit tests to verify their correctness and robustness against edge cases.
src/services/formatters/YScopeFormatter/index.ts (4)
26-29: Validate 'formatString' in the constructorTo prevent potential runtime errors, add validation to ensure
options.formatStringis defined and is a non-empty string before using it.Apply this diff to add validation:
constructor (options: FormatterOptionsType) { + if (!options.formatString || typeof options.formatString !== 'string') { + throw new Error('formatString must be a non-empty string'); + } this.#formatString = options.formatString; this.#parseFieldPlaceholder(); }
83-87: Enhance error message for invalid placeholdersWhen throwing an error due to an invalid placeholder, include the actual placeholder string in the error message to aid in debugging.
Apply this diff to improve the error message:
if ("undefined" === typeof placeholderString) { - throw Error("Field placeholder regex is invalid and does not have a capture group"); + throw Error(`Invalid placeholder: unable to extract placeholder from '${execResult[0]}'`); }
102-103: Provide more context in unsupported formatter errorWhen a specified formatter is not supported, include the placeholder string or formatter options to help identify the issue.
Apply this diff to enhance the error message:
if ("undefined" === typeof FieldFormatterConstructor) { - throw Error(`Formatter ${formatterName} is not currently supported`); + throw Error(`Formatter '${formatterName}' specified in placeholder '${placeholderString}' is not currently supported`); }
43-45: Clarify error message for placeholder index out of boundsImprove the error message to include the current index and total placeholders, providing clearer context for the error.
Apply this diff:
if ("undefined" === typeof fieldPlaceholder) { - throw new Error("Unexpected change in placeholder quantity in format string."); + throw new Error(`Placeholder index ${placeholderIndex} exceeds total placeholders ${this.#fieldPlaceholders.length}.`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
src/components/modals/SettingsModal/SettingsDialog.tsx(1 hunks)src/services/decoders/ClpIrDecoder.ts(3 hunks)src/services/decoders/JsonlDecoder/index.ts(3 hunks)src/services/formatters/LogbackFormatter.ts(1 hunks)src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts(1 hunks)src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts(1 hunks)src/services/formatters/YScopeFormatter/index.ts(1 hunks)src/services/formatters/YScopeFormatter/utils.ts(1 hunks)src/typings/formatters.ts(2 hunks)src/utils/config.ts(1 hunks)src/utils/formatters.ts(1 hunks)src/utils/js.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-check
src/services/decoders/ClpIrDecoder.ts
[failure] 15-15:
Unable to resolve path to module '../../formatters/YScopeFormatter'
[failure] 42-42:
Unsafe construction of an any type value
[failure] 90-90:
Unsafe assignment of an error typed value
[failure] 90-90:
Unsafe construction of an any type value
🔇 Additional comments (16)
src/utils/formatters.ts (2)
19-19: LGTM!
The export statement follows TypeScript best practices.
10-17: 🛠️ Refactor suggestion
Consider enhancing pattern support and optimizing replacements
The current implementation has two potential areas for improvement:
- Limited pattern support: Common patterns like 'HH', 'mm', 'ss', 'SSS' are missing
- Multiple string replacements could be optimized
Consider this more comprehensive implementation:
const convertDateTimeFormatterPatternToDayJs = (pattern: string): string => {
- pattern = pattern.replace("yyyy", "YYYY");
- pattern = pattern.replace("yy", "YY");
- pattern = pattern.replace("dd", "D");
- pattern = pattern.replace("d", "D");
-
- return pattern;
+ const conversions = new Map([
+ ['yyyy', 'YYYY'],
+ ['yy', 'YY'],
+ ['dd', 'D'],
+ ['d', 'D'],
+ ['HH', 'H'],
+ ['mm', 'm'],
+ ['ss', 's'],
+ ['SSS', 'ms']
+ ]);
+
+ return Array.from(conversions.entries()).reduce(
+ (result, [javaPattern, dayJsPattern]) =>
+ result.replace(new RegExp(javaPattern, 'g'), dayJsPattern),
+ pattern
+ );
};Let's verify the usage of this function in the codebase:
src/utils/js.ts (3)
1-4: LGTM! Clean and well-structured imports.
The import statement is properly organized and uses appropriate TypeScript types.
30-30: LGTM! Clean export statement.
The named export is properly implemented.
15-28: Verify the function usage in YscopeFormatter.
Since this function is intended to be used by the YscopeFormatter, let's verify its integration.
✅ Verification successful
Let me check the usage context in YScopeFormatter to ensure it's being used correctly.
Function is correctly integrated and used in YScopeFormatter
The verification shows that:
- The function is imported and used in YScopeFormatter as expected
- The type compatibility is correct with
logEvent.fieldsbeing of typeJsonObject - The usage handles undefined returns appropriately by converting them to empty strings
- The function is used to safely access nested fields using the
fieldNameKeysarray
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how getNestedJsonValue is being used in the codebase
# Find imports of getNestedJsonValue
echo "Checking imports:"
rg "import.*getNestedJsonValue" -A 2
# Find usage of getNestedJsonValue
echo "\nChecking usage:"
rg "getNestedJsonValue\(" -A 2
Length of output: 723
Script:
#!/bin/bash
# Get the implementation context around the usage
rg "getNestedJsonValue\(" -B 10 -A 5 src/services/formatters/YScopeFormatter/index.ts
# Check the type of logEvent.fields to verify type compatibility
ast-grep --pattern 'interface LogEvent {
$$$
fields: $_
$$$
}'
Length of output: 1068
src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts (1)
12-27: Verify integration with YScope formatting system
Let's verify how this formatter integrates with the broader YScope formatting system.
✅ Verification successful
RoundFormatter is properly integrated with YScope formatting system
The formatter is correctly integrated:
- Imported and registered in
src/services/formatters/YScopeFormatter/utils.tsas the 'round' formatter - Implements the required
YScopeFieldFormatterinterface - Follows the same pattern as other formatters in the system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check formatter registration and usage in the codebase
# Check if RoundFormatter is properly registered in the formatters map
rg -l "RoundFormatter" --type typescript
# Check for formatter usage patterns
ast-grep --pattern 'new RoundFormatter($_)'
Length of output: 121
Script:
#!/bin/bash
# Search for RoundFormatter class references
rg "RoundFormatter"
# Search for formatter registration patterns
rg "formatters\s*=|formatters\.set|registerFormatter" -A 3
# Look for test files related to RoundFormatter
rg "RoundFormatter.*test|test.*RoundFormatter"
Length of output: 551
src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts (3)
1-8: LGTM! Well-organized imports.
The imports are properly organized, separating external dependencies from internal ones, and all imports are necessary for the implementation.
10-17: LGTM! Well-documented class with proper TypeScript practices.
The class is well-documented and follows TypeScript best practices with proper interface implementation and private member typing.
25-30: Address the TODO comment regarding timestamp parsing efficiency.
The comment identifies a potential performance issue with parsing timestamps multiple times. This could impact performance when processing large log files.
Let's analyze the impact:
Consider these architectural improvements:
- Cache parsed timestamps when the same field is formatted multiple times
- Accept pre-parsed timestamps in the formatField method
src/services/formatters/YScopeFormatter/utils.ts (1)
91-95: LGTM!
The exports are well-organized and follow best practices.
src/services/formatters/LogbackFormatter.ts (1)
10-10: LGTM! Good refactoring of the date format conversion logic.
Moving the convertDateTimeFormatterPatternToDayJs function to a utility file improves code organization and promotes reusability across the codebase.
src/utils/config.ts (1)
22-23: Verify format string parsing in YscopeFormatter
The format string uses escaped colons and nested object paths which need special handling in the parser.
Let's verify the formatter's handling of these cases:
Consider improving readability
Consider using a template literal for better readability:
- formatString: "{@timestamp:timestamp:yyyy-MM-dd HH\\:mm\\:ss.SSS}" +
- " [{process\\.thread\\.name}] {log\\.level} {message} \n",
+ formatString: `{@timestamp:timestamp:yyyy-MM-dd HH\\:mm\\:ss.SSS} [{process\\.thread\\.name}] {log\\.level} {message}\n`,✅ Verification successful
Let me gather more information about the format string parsing implementation.
Let me check the actual implementation of YscopeFormatter to verify the format string handling.
Let me check the field placeholder regex and formatter utilities to complete the verification.
Format string parsing is properly implemented
The YscopeFormatter implementation correctly handles:
- Escaped colons and braces through
FIELD_PLACEHOLDER_REGEXandCOLON_REGEX - Nested object paths via
PERIOD_REGEXandgetNestedJsonValue - Field formatting with dedicated formatters (timestamp, round)
- Proper escaping of backslashes and special characters
The format string in the config is valid and will be correctly parsed by the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if YscopeFormatter properly handles escaped colons and nested paths
# Search for the format string parsing logic
ast-grep --pattern 'class YscopeFormatter {
$$$
constructor($_) {
$$$
}
$$$
}'
# Check for escape character handling
rg -A 5 'formatString.*\\\\:'
Length of output: 520
Script:
#!/bin/bash
# Search for format string parsing implementation
rg -l "YscopeFormatter|formatString" --type ts
# Look for format string parsing logic
ast-grep --pattern 'function $_(formatString: $_) {
$$$
}'
# Search for any tests related to format string parsing
fd test -e ts -x rg -l "formatString|YscopeFormatter" {}
Length of output: 494
Script:
#!/bin/bash
# Check YscopeFormatter implementation
cat src/services/formatters/YScopeFormatter/index.ts
# Look for any format string parsing tests
rg -A 10 "describe.*YscopeFormatter" --type ts
# Check if there's any format string validation
ast-grep --pattern 'class YScopeFormatter {
$$$
parse($_) {
$$$
}
$$$
}'
Length of output: 4787
Script:
#!/bin/bash
# Check the field placeholder regex and formatter utilities
cat src/typings/formatters.ts
cat src/services/formatters/YScopeFormatter/utils.ts
# Look for any related tests
fd test -e ts -x rg -l "FIELD_PLACEHOLDER_REGEX|splitFieldPlaceholder" {}
Length of output: 7004
src/services/decoders/JsonlDecoder/index.ts (3)
19-19: LGTM! Clean import of the new formatter.
The import statement follows the existing code style and properly imports the new YscopeFormatter.
Line range hint 206-206: Verify YscopeFormatter behaviour with invalid log events.
The decoder properly handles invalid JSON by creating default log events. We should ensure YscopeFormatter maintains this robustness when formatting these default events.
Let's verify the formatter's handling of invalid events:
#!/bin/bash
# Description: Check YscopeFormatter's handling of invalid/default log events
# Search for handling of default/invalid events in YscopeFormatter
ast-grep --pattern 'class YscopeFormatter {
$$$
formatLogEvent(logEvent) {
$$$
}
}'
# Look for any null checks or defensive programming
rg -A 5 'formatLogEvent.*\{' src/services/formatters/YScopeFormatter/56-56: ```shell
#!/bin/bash
Let's search for YscopeFormatter implementation and constructor
rg -l "class YscopeFormatter" src/
```shell
#!/bin/bash
# Once we find the file, let's examine its content
cat src/services/formatters/YScopeFormatter/index.ts
#!/bin/bash
# Let's also check for any validation or error handling patterns
rg -A 5 "constructor.*YscopeFormatter" src/#!/bin/bash
# Search for any validation utilities that might be used
rg "validateFormat|validateString|validate.*Format" src/src/typings/formatters.ts (1)
38-38:
Typo in JSON example.
There is a typo in the JSON example. The colon is incorrectly placed within the quotation marks.
Suggested correction:
- * `{"a:" {"b": 0}}` may be denoted by `a.b`.
+ * `{"a": {"b": 0}}` may be denoted by `a.b`.Likely invalid or redundant comment.
src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts
Outdated
Show resolved
Hide resolved
| formatField (field: JsonValue): string { | ||
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: We already parsed the timestamp during deserialization so this is perhaps | ||
| // inefficient. However, this field formatter can be used for multiple keys, so using | ||
| // the single parsed timestamp by itself would not work. Perhaps in future we can check | ||
| // if the key is the same as timestamp key and avoid parsing again. | ||
| const timestamp: Dayjs = convertToDayjsTimestamp(field); | ||
| if (null === this.#dateFormat) { | ||
| return timestamp.format(); | ||
| } | ||
|
|
||
| return timestamp.format(this.#dateFormat); | ||
| } |
There was a problem hiding this comment.
Add error handling for invalid inputs.
The method should handle cases where the input cannot be converted to a timestamp.
Consider adding try-catch:
formatField (field: JsonValue): string {
+ try {
const timestamp: Dayjs = convertToDayjsTimestamp(field);
if (null === this.#dateFormat) {
return timestamp.format();
}
return timestamp.format(this.#dateFormat);
+ } catch (error) {
+ throw new Error(`Failed to format timestamp: ${error.message}`);
+ }
}📝 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.
| formatField (field: JsonValue): string { | |
| // eslint-disable-next-line no-warning-comments | |
| // TODO: We already parsed the timestamp during deserialization so this is perhaps | |
| // inefficient. However, this field formatter can be used for multiple keys, so using | |
| // the single parsed timestamp by itself would not work. Perhaps in future we can check | |
| // if the key is the same as timestamp key and avoid parsing again. | |
| const timestamp: Dayjs = convertToDayjsTimestamp(field); | |
| if (null === this.#dateFormat) { | |
| return timestamp.format(); | |
| } | |
| return timestamp.format(this.#dateFormat); | |
| } | |
| formatField (field: JsonValue): string { | |
| // eslint-disable-next-line no-warning-comments | |
| // TODO: We already parsed the timestamp during deserialization so this is perhaps | |
| // inefficient. However, this field formatter can be used for multiple keys, so using | |
| // the single parsed timestamp by itself would not work. Perhaps in future we can check | |
| // if the key is the same as timestamp key and avoid parsing again. | |
| try { | |
| const timestamp: Dayjs = convertToDayjsTimestamp(field); | |
| if (null === this.#dateFormat) { | |
| return timestamp.format(); | |
| } | |
| return timestamp.format(this.#dateFormat); | |
| } catch (error) { | |
| throw new Error(`Failed to format timestamp: ${error.message}`); | |
| } | |
| } |
There was a problem hiding this comment.
convert to dayjs already has error handling
There was a problem hiding this comment.
@davemarco, thank you for the clarification. Since convertToDayjsTimestamp already handles errors internally, additional error handling here is unnecessary.
(_/)
(o.o)
(>?)
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts:24-36
Timestamp: 2024-11-18T01:34:54.777Z
Learning: In `src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts`, the `convertToDayjsTimestamp` function already includes error handling for invalid inputs, so wrapping it in try-catch in the `TimestampFormatter.formatField` method is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (null !== formatterName) { | ||
| formatterName.replace(BACKSLASH_REGEX, ""); | ||
| } |
There was a problem hiding this comment.
Fix string replacement operations
The replace operations don't modify the original strings as strings are immutable in JavaScript. The results need to be reassigned.
if (null !== formatterName) {
- formatterName.replace(BACKSLASH_REGEX, "");
+ formatterName = formatterName.replace(BACKSLASH_REGEX, "");
}
formatterOptions = validateComponent(formatterOptions);
if (null !== formatterOptions) {
- formatterOptions.replace(BACKSLASH_REGEX, "");
+ formatterOptions = formatterOptions.replace(BACKSLASH_REGEX, "");
}Also applies to: 83-85
There was a problem hiding this comment.
this is stale but thanks
There was a problem hiding this comment.
| @@ -12,7 +12,7 @@ | |||
| import {Formatter} from "../../typings/formatters"; | |||
| import {JsonObject} from "../../typings/js"; | |||
| import {LogLevelFilter} from "../../typings/logs"; | |||
| import LogbackFormatter from "../formatters/LogbackFormatter"; | |||
| import YscopeFormatter from "../../formatters/YScopeFormatter"; | |||
There was a problem hiding this comment.
💡 Codebase verification
The import path needs to be updated to match the directory structure
The import statement needs to be corrected to point to the actual YScopeFormatter directory:
-import YscopeFormatter from "../../formatters/YScopeFormatter";
+import YscopeFormatter from "../../formatters/YScopeFormatter/index";- The formatter is located at
src/services/formatters/YScopeFormatter/index.ts - The current import path is incomplete and doesn't point to the actual file
🔗 Analysis chain
Fix the import path casing
The import statement uses inconsistent casing compared to the actual file name. Also, the static analysis indicates the path cannot be resolved.
Apply this fix:
-import YscopeFormatter from "../../formatters/YScopeFormatter";
+import YscopeFormatter from "../../formatters/YscopeFormatter";🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the correct path and casing
fd -t f "yscope.*formatter" -I
Length of output: 32
Script:
#!/bin/bash
# Let's try a broader search for formatter files
fd -t f "formatter" -i
# Also search for YScope-related files with any extension
fd "yscope" -i
# Use ripgrep to find any references to YScope formatter
rg -i "yscope.*formatter"
Length of output: 3398
🧰 Tools
🪛 GitHub Check: lint-check
[failure] 15-15:
Unable to resolve path to module '../../formatters/YScopeFormatter'
There was a problem hiding this comment.
i think index is a special name
There was a problem hiding this comment.
@davemarco, you're correct that index is a special name and doesn't need to be specified in the import path. Thank you for pointing that out. However, I noticed that the linter is reporting an error:
Unable to resolve path to module '../../formatters/YScopeFormatter'
This might be due to a casing mismatch in the import path. The directory seems to be named YscopeFormatter with a lowercase "s", but the import statement uses YScopeFormatter with an uppercase "S". In case-sensitive file systems, this can cause issues. Could you please verify if the casing matches the actual directory name?
✏️ Learnings added
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/decoders/ClpIrDecoder.ts:0-0
Timestamp: 2024-11-18T01:36:21.946Z
Learning: In JavaScript/TypeScript module imports, `index` is automatically resolved when importing from a directory, so specifying `index` in the import path is unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| * A formatter that uses a Yscope format string to format log events into a string. See | ||
| * `YscopeFormatterOptionsType` for details about the format string. | ||
| */ | ||
| class YscopeFormatter implements Formatter { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent class naming
The class is named YscopeFormatter, but elsewhere in the codebase and file structure, 'YScope' uses a capital 'S'. For consistency and to adhere to naming conventions, consider renaming the class to YScopeFormatter.
Apply this diff to update the class name:
-class YscopeFormatter implements Formatter {
+class YScopeFormatter implements Formatter {And update the export statement:
-export default YscopeFormatter;
+export default YScopeFormatter;There was a problem hiding this comment.
@davemarco, you're welcome! Glad to hear that you made the change.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
davemarco
changed the title
| @@ -7,26 +7,9 @@ import { | |||
| } from "../../typings/formatters"; | |||
| import {JsonObject} from "../../typings/js"; | |||
| import {LogEvent} from "../../typings/logs"; | |||
| import {convertDateTimeFormatterPatternToDayJs} from "../../utils/formatters"; | |||
There was a problem hiding this comment.
Is this function necessary? can't we just link to day JS documentation and use dayjs format syntax?
There was a problem hiding this comment.
I know it used in logback formatter, so I reused but curious
There was a problem hiding this comment.
can't we just link to day JS documentation and use dayjs format syntax?
I agree. In my opinion, using Dayjs's datetime syntax in our own syntax is fine.
There was a problem hiding this comment.
@kirkrodrigues - convertDateTimeFormatterPatternToDayJs function seems unnecessary. It effectively changes the capitalization in the date format string, before it is passed to DayJs. I guess the purpose is to conform with the java syntax? I think it would be simpler to just conform with the dayjs syntax, instead of providing support to translate the format. let us know ur thoughts?
There was a problem hiding this comment.
Iirc, this was added to support Logback's syntax? I'm fine with conforming to Dayjs' syntax, but my understanding was that some users wanted to use Logback's syntax. @junhaoliao, is that no longer true?
There was a problem hiding this comment.
In the "old" log viewer we have massed deployed, we hard code the format string for different customers. The configuration interface was never directly exposed, so I feel it's fine to remove the Logback formatter and directly provide the new formatter.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
src/services/formatters/YScopeFormatter/utils.ts (4)
19-22: Consider adding documentation for extending formattersThe
YSCOPE_FORMATTERS_MAPis well-structured, but could benefit from documentation explaining how to add new formatters to the map. This would help future contributors understand the process of extending the formatter system.Add JSDoc comments explaining the requirements and steps for adding new formatters:
/** * List of currently supported field formatters. + * + * To add a new formatter: + * 1. Create a new formatter class in the FieldFormatters directory + * 2. Implement the required formatter interface + * 3. Add the formatter to this map with a unique key */ const YSCOPE_FORMATTERS_MAP: YScopeFieldFormatterMap = Object.freeze({ timestamp: TimestampFormatter, round: RoundFormatter, });
24-35: Enhance documentation and readability of jsonValueToStringThe function implementation is correct but could benefit from improved documentation and readability.
Consider these improvements:
/** * Converts a JSON value to its string representation. + * + * @param input - The JSON value to convert + * - If undefined: returns "undefined" + * - If object: returns JSON.stringify result + * - Otherwise: returns String conversion * - * @param input - * @return + * @return The string representation of the input */ const jsonValueToString = (input: JsonValue | undefined): string => { - // Behaviour is different for `undefined`. - return "object" === typeof input ? - JSON.stringify(input) : - String(input); + if (typeof input === "object") { + return JSON.stringify(input); + } + return String(input); };
45-59: Add validation and make defaults configurableThe function handles undefined values well, but could benefit from additional validation and configuration options.
Consider these improvements:
+ const DEFAULT_EMPTY_VALUE = ""; + const getFormattedField = ( logEvent: LogEvent, - fieldPlaceholder: YScopeFieldPlaceholder + fieldPlaceholder: YScopeFieldPlaceholder, + options: { emptyValue?: string } = {} ): string => { + if (!logEvent?.fields) { + throw new Error("Invalid log event structure"); + } + let nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys); if ("undefined" === typeof nestedValue) { - nestedValue = ""; + nestedValue = options.emptyValue ?? DEFAULT_EMPTY_VALUE; }
96-98: Enhance error message with more contextThe error message could be more helpful by including the field placeholder that failed to parse.
if (null === fieldName) { - throw Error("Field name could not be parsed"); + throw Error(`Field name could not be parsed from placeholder: "${fieldPlaceholder}"`); }src/typings/formatters.ts (4)
31-50: Documentation needs minor improvementsThe documentation has a grammatical error and could be clearer:
- Line 45: "All three as" appears to be a typo
- Consider clarifying the escape character explanation with examples
- * All three as may contain any character, except that colons (:), right braces (}), - * and backslashes (\) must be escaped with a backslash. + * All three components may contain any character, except that colons (:), right braces (}), + * and backslashes (\) must be escaped with a backslash. For example: + * - Field name with period: "my\.field" + * - Formatter with colon: "timestamp\:iso"
56-62: Add @returns tag to JSDocThe method documentation should include a @returns tag for completeness.
/** * Formats the given log event. * * @param logEvent + * @returns The formatted log event as a string. - * @return The formatted log event. */
65-74: Improve documentation and maintain naming consistency
- Add @returns tag to JSDoc
- The parameter name in the documentation doesn't match the method signature (logEvent vs field)
/** * Formats the given field. * - * @param logEvent + * @param field The field value to format + * @returns The formatted field as a string - * @return The formatted field. */
91-111: Consider browser compatibility and improve regex documentationThe negative lookbehind patterns (
(?<!\\)) might not be supported in all browsers. Also, the documentation could be more detailed:
- Add browser compatibility notes
- Explain the regex patterns with examples
- Consider adding tests for these patterns
Consider providing fallback patterns for browsers that don't support negative lookbehind.
src/services/formatters/YScopeFormatter/index.ts (3)
17-20: Enhance class documentation with examples and detailed format string syntaxThe current documentation could be more helpful by including:
- Format string syntax examples
- Explanation of supported formatters (timestamp, rounding)
- Usage examples with sample input and output
Would you like me to help generate comprehensive documentation?
42-44: Improve error message clarityThe error message could be more descriptive to help with debugging.
Apply this diff:
- throw Error("Insufficient placeholder quantity: format string was modified"); + throw Error(`Expected ${this.#fieldPlaceholders.length} placeholders but found fewer. The format string might have been modified: ${this.#formatString}`);
59-59: Make newline handling configurableThe method always appends a newline character. Consider making this configurable through the formatter options.
Would you like me to propose a solution that makes the newline behaviour configurable?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/services/decoders/ClpIrDecoder.ts(3 hunks)src/services/formatters/YScopeFormatter/index.ts(1 hunks)src/services/formatters/YScopeFormatter/utils.ts(1 hunks)src/typings/formatters.ts(2 hunks)src/utils/config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/services/decoders/ClpIrDecoder.ts
- src/utils/config.ts
🔇 Additional comments (6)
src/services/formatters/YScopeFormatter/utils.ts (2)
67-73: LGTM! Clean and well-documented implementation
The function is concise, properly documented, and handles edge cases appropriately.
120-125: LGTM! Well-organized exports
The exports are clean and include all necessary functions and constants.
src/typings/formatters.ts (4)
52-52: LGTM: Type union is well-defined
The union type correctly combines both formatter option types.
76-82: LGTM: Well-structured type definition
The type correctly represents the parsed field placeholder structure with support for nested fields.
113-126: LGTM: Exports are well-organized
All necessary types and constants are properly exported.
84-89: LGTM: Verify formatter implementations
The type correctly defines the structure for formatter constructors. Let's verify the existing formatter implementations match this interface.
✅ Verification successful
Let me gather more information to verify all formatter implementations.
Formatter implementations correctly match the interface
Both existing formatters (RoundFormatter and TimestampFormatter) properly implement the YScopeFieldFormatter interface with constructors that accept a Nullable<string> parameter as specified in the YScopeFieldFormatterMap type.
RoundFormatter: Accepts options but throws if non-null options are providedTimestampFormatter: Properly handles the optional format string parameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all formatter implementations to verify they match the interface
ast-grep --pattern 'class $_ implements YScopeFieldFormatter {
constructor($_: Nullable<string>) {
$$$
}
$$$
}'
Length of output: 1824
Script:
#!/bin/bash
# Find all classes that implement YScopeFieldFormatter
ast-grep --pattern 'class $_ implements YScopeFieldFormatter'
# Also search for the interface definition
ast-grep --pattern 'interface YScopeFieldFormatter'
Length of output: 117
Script:
#!/bin/bash
# Search for all formatter class files
fd ".*Formatter.ts" src/services/formatters
# Search for interface definition and implementations with ripgrep
rg "implements YScopeFieldFormatter" -A 5
rg "interface YScopeFieldFormatter" -A 5
Length of output: 1991
| const pattern = new RegExp(BACKSLASH_REGEX, "g"); | ||
|
|
||
| // Remove escape characters (`\`) after the field name is split. | ||
| fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(pattern, "")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize regex pattern creation and string replacement
The regex pattern is recreated for each function call and used multiple times.
Consider moving the pattern to a constant and using a more efficient approach:
- const pattern = new RegExp(BACKSLASH_REGEX, "g");
+const BACKSLASH_PATTERN = new RegExp(BACKSLASH_REGEX, "g");
// Remove escape characters (`\`) after the field name is split.
- fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(pattern, ""));
+ fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(BACKSLASH_PATTERN, ""));📝 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 pattern = new RegExp(BACKSLASH_REGEX, "g"); | |
| // Remove escape characters (`\`) after the field name is split. | |
| fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(pattern, "")); | |
| const BACKSLASH_PATTERN = new RegExp(BACKSLASH_REGEX, "g"); | |
| // Remove escape characters (`\`) after the field name is split. | |
| fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(BACKSLASH_PATTERN, "")); |
| constructor (options: FormatterOptionsType) { | ||
| this.#formatString = options.formatString; | ||
| this.#parseFieldPlaceholder(); | ||
| } |
There was a problem hiding this comment.
Add input validation in the constructor
The constructor should validate the formatString parameter to ensure it's not undefined, empty, or malformed before processing.
Apply this diff to add validation:
constructor (options: FormatterOptionsType) {
+ if (!options?.formatString?.trim()) {
+ throw new Error('Format string must not be empty');
+ }
this.#formatString = options.formatString;
this.#parseFieldPlaceholder();
}📝 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.
| constructor (options: FormatterOptionsType) { | |
| this.#formatString = options.formatString; | |
| this.#parseFieldPlaceholder(); | |
| } | |
| constructor (options: FormatterOptionsType) { | |
| if (!options?.formatString?.trim()) { | |
| throw new Error('Format string must not be empty'); | |
| } | |
| this.#formatString = options.formatString; | |
| this.#parseFieldPlaceholder(); | |
| } |
| const placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | ||
| const backslashPattern = new RegExp(BACKSLASH_REGEX, "g"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize regex pattern creation
The RegExp objects are recreated on every method call. Consider moving them to class-level constants for better performance.
Apply this diff:
class YScopeFormatter implements Formatter {
+ static readonly #PLACEHOLDER_PATTERN = new RegExp(FIELD_PLACEHOLDER_REGEX, "g");
+ static readonly #BACKSLASH_PATTERN = new RegExp(BACKSLASH_REGEX, "g");
#formatString: string;
formatLogEvent (logEvent: LogEvent): string {
- const placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g");
- const backslashPattern = new RegExp(BACKSLASH_REGEX, "g");
+ const placeholderPattern = YScopeFormatter.#PLACEHOLDER_PATTERN;
+ const backslashPattern = YScopeFormatter.#BACKSLASH_PATTERN;📝 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 placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | |
| const backslashPattern = new RegExp(BACKSLASH_REGEX, "g"); | |
| class YScopeFormatter implements Formatter { | |
| static readonly #PLACEHOLDER_PATTERN = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | |
| static readonly #BACKSLASH_PATTERN = new RegExp(BACKSLASH_REGEX, "g"); | |
| #formatString: string; | |
| formatLogEvent (logEvent: LogEvent): string { | |
| const placeholderPattern = YScopeFormatter.#PLACEHOLDER_PATTERN; | |
| const backslashPattern = YScopeFormatter.#BACKSLASH_PATTERN; |
| #parseFieldPlaceholder () { | ||
| const pattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | ||
| const it = this.#formatString.matchAll(pattern); | ||
| for (const execResult of it) { | ||
| // The 1-index of exec result is the capture group in `FIELD_PLACEHOLDER_REGEX`. | ||
| // (i.e. entire field-placeholder excluding braces). | ||
| const [, placeholderString]: (string | undefined) [] = execResult; | ||
|
|
||
| if ("undefined" === typeof placeholderString) { | ||
| throw Error("Field placeholder regex is invalid and does not have a capture group"); | ||
| } | ||
|
|
||
| const {fieldNameKeys, formatterName, formatterOptions} = | ||
| splitFieldPlaceholder(placeholderString); | ||
|
|
||
| if (null === formatterName) { | ||
| this.#fieldPlaceholders.push({ | ||
| fieldNameKeys: fieldNameKeys, | ||
| fieldFormatter: null, | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| const FieldFormatterConstructor = YSCOPE_FORMATTERS_MAP[formatterName]; | ||
| if ("undefined" === typeof FieldFormatterConstructor) { | ||
| throw Error(`Formatter ${formatterName} is not currently supported`); | ||
| } | ||
|
|
||
| const fieldFormatter: YScopeFieldFormatter = | ||
| new FieldFormatterConstructor(formatterOptions); | ||
|
|
||
| this.#fieldPlaceholders.push({ | ||
| fieldNameKeys: fieldNameKeys, | ||
| fieldFormatter: fieldFormatter, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and validation in parseFieldPlaceholder
Consider these improvements:
- Add early validation for empty format string
- Improve error messages with context
- Clean up regex iterator resources
Apply this diff:
#parseFieldPlaceholder () {
+ if (!this.#formatString.includes('{')) {
+ throw Error('Format string contains no placeholders');
+ }
const pattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g");
const it = this.#formatString.matchAll(pattern);
+ let hasPlaceholders = false;
for (const execResult of it) {
+ hasPlaceholders = true;
// The 1-index of exec result is the capture group in `FIELD_PLACEHOLDER_REGEX`.
// (i.e. entire field-placeholder excluding braces).
const [, placeholderString]: (string | undefined) [] = execResult;
if ("undefined" === typeof placeholderString) {
- throw Error("Field placeholder regex is invalid and does not have a capture group");
+ throw Error(`Invalid placeholder format in: ${execResult[0]}`);
}
const {fieldNameKeys, formatterName, formatterOptions} =
splitFieldPlaceholder(placeholderString);
if (null === formatterName) {
this.#fieldPlaceholders.push({
fieldNameKeys: fieldNameKeys,
fieldFormatter: null,
});
continue;
}
const FieldFormatterConstructor = YSCOPE_FORMATTERS_MAP[formatterName];
if ("undefined" === typeof FieldFormatterConstructor) {
- throw Error(`Formatter ${formatterName} is not currently supported`);
+ throw Error(`Unsupported formatter '${formatterName}' in placeholder: ${execResult[0]}`);
}
const fieldFormatter: YScopeFieldFormatter =
new FieldFormatterConstructor(formatterOptions);
this.#fieldPlaceholders.push({
fieldNameKeys: fieldNameKeys,
fieldFormatter: fieldFormatter,
});
}
+ if (!hasPlaceholders) {
+ throw Error('No valid placeholders found in format string');
+ }
}📝 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.
| #parseFieldPlaceholder () { | |
| const pattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | |
| const it = this.#formatString.matchAll(pattern); | |
| for (const execResult of it) { | |
| // The 1-index of exec result is the capture group in `FIELD_PLACEHOLDER_REGEX`. | |
| // (i.e. entire field-placeholder excluding braces). | |
| const [, placeholderString]: (string | undefined) [] = execResult; | |
| if ("undefined" === typeof placeholderString) { | |
| throw Error("Field placeholder regex is invalid and does not have a capture group"); | |
| } | |
| const {fieldNameKeys, formatterName, formatterOptions} = | |
| splitFieldPlaceholder(placeholderString); | |
| if (null === formatterName) { | |
| this.#fieldPlaceholders.push({ | |
| fieldNameKeys: fieldNameKeys, | |
| fieldFormatter: null, | |
| }); | |
| continue; | |
| } | |
| const FieldFormatterConstructor = YSCOPE_FORMATTERS_MAP[formatterName]; | |
| if ("undefined" === typeof FieldFormatterConstructor) { | |
| throw Error(`Formatter ${formatterName} is not currently supported`); | |
| } | |
| const fieldFormatter: YScopeFieldFormatter = | |
| new FieldFormatterConstructor(formatterOptions); | |
| this.#fieldPlaceholders.push({ | |
| fieldNameKeys: fieldNameKeys, | |
| fieldFormatter: fieldFormatter, | |
| }); | |
| } | |
| } | |
| #parseFieldPlaceholder () { | |
| if (!this.#formatString.includes('{')) { | |
| throw Error('Format string contains no placeholders'); | |
| } | |
| const pattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | |
| const it = this.#formatString.matchAll(pattern); | |
| let hasPlaceholders = false; | |
| for (const execResult of it) { | |
| hasPlaceholders = true; | |
| // The 1-index of exec result is the capture group in `FIELD_PLACEHOLDER_REGEX`. | |
| // (i.e. entire field-placeholder excluding braces). | |
| const [, placeholderString]: (string | undefined) [] = execResult; | |
| if ("undefined" === typeof placeholderString) { | |
| throw Error(`Invalid placeholder format in: ${execResult[0]}`); | |
| } | |
| const {fieldNameKeys, formatterName, formatterOptions} = | |
| splitFieldPlaceholder(placeholderString); | |
| if (null === formatterName) { | |
| this.#fieldPlaceholders.push({ | |
| fieldNameKeys: fieldNameKeys, | |
| fieldFormatter: null, | |
| }); | |
| continue; | |
| } | |
| const FieldFormatterConstructor = YSCOPE_FORMATTERS_MAP[formatterName]; | |
| if ("undefined" === typeof FieldFormatterConstructor) { | |
| throw Error(`Unsupported formatter '${formatterName}' in placeholder: ${execResult[0]}`); | |
| } | |
| const fieldFormatter: YScopeFieldFormatter = | |
| new FieldFormatterConstructor(formatterOptions); | |
| this.#fieldPlaceholders.push({ | |
| fieldNameKeys: fieldNameKeys, | |
| fieldFormatter: fieldFormatter, | |
| }); | |
| } | |
| if (!hasPlaceholders) { | |
| throw Error('No valid placeholders found in format string'); | |
| } | |
| } |
davemarco
changed the title
| @@ -33,8 +33,11 @@ import ThemeSwitchToggle from "./ThemeSwitchToggle"; | |||
|
|
|||
| const CONFIG_FORM_FIELDS = [ | |||
| { | |||
| helperText: "[JSON] Log messages conversion pattern. The current syntax is similar to" + | |||
| " Logback conversion patterns but will change in a future release.", | |||
| helperText: `[JSON] Log messages conversion pattern. Add field-placeholders to insert | |||
There was a problem hiding this comment.
TODO: add this on the docs site once the site infra is ready
There was a problem hiding this comment.
@coderabbitai let's open a GitHub issue for this.
There was a problem hiding this comment.
@junhaoliao, I've opened a GitHub issue as requested: #128
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Opening a new GitHub issue as requested
gh issue create \
--title "Add documentation for the [JSON] log messages conversion pattern" \
--body $'This issue is to add documentation to the site for the [JSON] log messages conversion pattern once the site infrastructure is ready.\n\nPR URL: https://github.com/y-scope/yscope-log-viewer/pull/123\nComment URL: https://github.com/y-scope/yscope-log-viewer/pull/123#discussion_r1847584208\nRequested by @junhaoliao' \
--assignee junhaoliao
Length of output: 490
There was a problem hiding this comment.
I can be wrong but the plan is to remove this formatter, right?
There was a problem hiding this comment.
Okay i will delete it. I wasn't sure what the plan was.
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | ||
|
|
||
| const pattern = new RegExp(BACKSLASH_REGEX, "g"); | ||
|
|
||
| // Remove escape characters (`\`) after the field name is split. | ||
| fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(pattern, "")); |
There was a problem hiding this comment.
Can we extract a helper function from this?
/**
* Parses a field name into an array of field name keys, supporting nested fields and escaping.
*
* @param {string} fieldName This may include nested field paths (e.g., "a.b.c") or escaped characters (e.g., "a\.b").
* @returns {string[]} - An array of field name keys, with escaped characters unescaped.
*/
There was a problem hiding this comment.
That code is just the cleaning code (i.e. removes the escape characters after the fieldName is split). The splitting is just one line earlier in function. let fieldNameKeys = fieldName.split(PERIOD_REGEX); I can put that in a function if you want? Let me know. A helper cleaning function is not that uselful since would need to handle both string and string[], and would probably make the code more complex.
There was a problem hiding this comment.
For now I added a comment above the split line. splits field name into an array of field name keys to support nested fields.
There was a problem hiding this comment.
Right, the original comment is about breaking the code into testable pieces. At this point if we are not checking in test cases, the code is fine to be left as-is.
src/typings/formatters.ts
Outdated
| /** | ||
| * Pattern to remove backlash. | ||
| */ | ||
| const BACKSLASH_REGEX = Object.freeze(/(?<!\\)\\/); |
There was a problem hiding this comment.
@kirkrodrigues To represent two literal backslashes "\", do we \\\ or \\\\?
This Regex takes cares of \\\ if i'm not wrong.
However, according to the ANTLR Grammar in our internal docs, it seems we need \\\\, which might not be possible with a single Regex pattern?
(Please correct me if I'm wrong with any of these. )
There was a problem hiding this comment.
We need \\\\ (one escape per literal backslash).
There was a problem hiding this comment.
I think this regex takes care of \\\\ or "\\". Say someone puts "\\" in their format string. The global replace using (pattern,"") replace the first "\" or \\ instance with "". It will not replace the second or following "\" or \\since it had a \\ or "\" in front of it. A negative look behind will also work i think. Anyways some of this is confusing, i can explain in person need be.
There was a problem hiding this comment.
However I may misunderstand your question... let me know
There was a problem hiding this comment.
Sorry I might have misunderstood the regex in the first place. 😅 I'll revisit those as the first thing when I review this again.
There was a problem hiding this comment.
Comment no longer relevant since added replacement character.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/services/formatters/YScopeFormatter/index.ts (2)
59-59: Consider making the trailing newline configurableThe method always appends a newline character to the formatted log. This might not be desirable in all cases, especially when the formatter is used in different contexts (e.g., console output vs file output).
Consider making this configurable through the formatter options:
interface FormatterOptionsType { formatString: string; + appendNewline?: boolean; } formatLogEvent (logEvent: LogEvent): string { // ... existing code ... - return `${formattedLog}\n`; + return this.options.appendNewline ? `${formattedLog}\n` : formattedLog; }
73-74: Ensure proper cleanup of regex iteratorThe
matchAlliterator should be properly cleaned up to prevent potential memory leaks in case of errors.Consider using a try-finally block:
#parseFieldPlaceholder () { const placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); const it = this.#processedFormatString.matchAll(placeholderPattern); + try { for (const match of it) { // ... existing code ... } + } finally { + // Clean up the iterator + it.return?.(); + } }src/services/formatters/YScopeFormatter/utils.ts (2)
83-88: Consider explicit null handlingThe current implementation might not handle
nullvalues distinctly from other objects. Consider being more explicit:- return "object" === typeof input ? - JSON.stringify(input) : - String(input); + if (input === null) { + return "null"; + } + return typeof input === "object" ? + JSON.stringify(input) : + String(input);
98-112: Enhance error handling for field formattingConsider adding explicit error handling for invalid formatter configurations and logging for debugging:
const formattedField = fieldPlaceholder.fieldFormatter ? - fieldPlaceholder.fieldFormatter.formatField(nestedValue) : + try { + fieldPlaceholder.fieldFormatter.formatField(nestedValue) + } catch (error) { + console.warn(`Failed to format field: ${error.message}`); + jsonValueToString(nestedValue) + } : jsonValueToString(nestedValue);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/services/formatters/YScopeFormatter/index.ts(1 hunks)src/services/formatters/YScopeFormatter/utils.ts(1 hunks)
🔇 Additional comments (2)
src/services/formatters/YScopeFormatter/utils.ts (2)
1-25: LGTM! Well-organized imports and constant definition.
The imports are properly grouped and the formatter map is correctly frozen to prevent runtime modifications.
172-179: LGTM! Well-organized exports.
All necessary utilities are properly exported with clear naming.
| const splitFieldPlaceholder = (placeholderString: string): { | ||
| fieldNameKeys: string[], | ||
| formatterName: Nullable<string>, | ||
| formatterOptions: Nullable<string>, | ||
| } => { | ||
| let [fieldName, | ||
| formatterName, | ||
| formatterOptions]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3); | ||
|
|
||
| fieldName = validateComponent(fieldName); | ||
| if (null === fieldName) { | ||
| throw Error("Field name could not be parsed"); | ||
| } | ||
|
|
||
| // Splits field name into an array of field name keys to support nested fields. | ||
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | ||
|
|
||
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | ||
|
|
||
| formatterName = validateComponent(formatterName); | ||
| if (null !== formatterName) { | ||
| formatterName = removeEscapeCharacters(formatterName); | ||
| } | ||
|
|
||
| formatterOptions = validateComponent(formatterOptions); | ||
| if (null !== formatterOptions) { | ||
| formatterOptions = removeEscapeCharacters(formatterOptions); | ||
| } | ||
|
|
||
| return {fieldNameKeys, formatterName, formatterOptions}; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for placeholderString
Consider adding input validation to prevent processing of invalid placeholder strings:
const splitFieldPlaceholder = (placeholderString: string): {
fieldNameKeys: string[],
formatterName: Nullable<string>,
formatterOptions: Nullable<string>,
} => {
+ if (!placeholderString || typeof placeholderString !== "string") {
+ throw new Error("Invalid placeholder string");
+ }
+
let [fieldName,
formatterName,
formatterOptions]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3);📝 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 splitFieldPlaceholder = (placeholderString: string): { | |
| fieldNameKeys: string[], | |
| formatterName: Nullable<string>, | |
| formatterOptions: Nullable<string>, | |
| } => { | |
| let [fieldName, | |
| formatterName, | |
| formatterOptions]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3); | |
| fieldName = validateComponent(fieldName); | |
| if (null === fieldName) { | |
| throw Error("Field name could not be parsed"); | |
| } | |
| // Splits field name into an array of field name keys to support nested fields. | |
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | |
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | |
| formatterName = validateComponent(formatterName); | |
| if (null !== formatterName) { | |
| formatterName = removeEscapeCharacters(formatterName); | |
| } | |
| formatterOptions = validateComponent(formatterOptions); | |
| if (null !== formatterOptions) { | |
| formatterOptions = removeEscapeCharacters(formatterOptions); | |
| } | |
| return {fieldNameKeys, formatterName, formatterOptions}; | |
| }; | |
| const splitFieldPlaceholder = (placeholderString: string): { | |
| fieldNameKeys: string[], | |
| formatterName: Nullable<string>, | |
| formatterOptions: Nullable<string>, | |
| } => { | |
| if (!placeholderString || typeof placeholderString !== "string") { | |
| throw new Error("Invalid placeholder string"); | |
| } | |
| let [fieldName, | |
| formatterName, | |
| formatterOptions]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3); | |
| fieldName = validateComponent(fieldName); | |
| if (null === fieldName) { | |
| throw Error("Field name could not be parsed"); | |
| } | |
| // Splits field name into an array of field name keys to support nested fields. | |
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | |
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | |
| formatterName = validateComponent(formatterName); | |
| if (null !== formatterName) { | |
| formatterName = removeEscapeCharacters(formatterName); | |
| } | |
| formatterOptions = validateComponent(formatterOptions); | |
| if (null !== formatterOptions) { | |
| formatterOptions = removeEscapeCharacters(formatterOptions); | |
| } | |
| return {fieldNameKeys, formatterName, formatterOptions}; | |
| }; |
| const removeBackslash = (str: string): string => { | ||
| const pattern = new RegExp(SINGLE_BACKSLASH_REGEX, "g"); | ||
| return str.replaceAll(pattern, ""); | ||
| }; | ||
|
|
||
| /** | ||
| * Replaces all replacement characters in format string with a single backslash. Purpose is to | ||
| * remove, albeit indirectly through intermediate replacement character, escape character in | ||
| * front of a backslash character. | ||
| * | ||
| * @param str | ||
| * @return Modified string. | ||
| */ | ||
| const replaceReplacementCharacter = (str: string): string => { | ||
| const pattern = new RegExp(REPLACEMENT_CHARACTER_REGEX, "g"); | ||
| return str.replaceAll(pattern, "\\"); | ||
| }; | ||
|
|
||
| /** | ||
| * Removes escape characters from a string. | ||
| * | ||
| * @param str | ||
| * @return Modified string. | ||
| */ | ||
| const removeEscapeCharacters = (str: string): string => { | ||
| // `removeBackslash()`, which removes all backlashes, is called before | ||
| // `replaceReplacementCharacter()` to prevent removal of escaped backslashes. | ||
| return replaceReplacementCharacter(removeBackslash(str)); | ||
| }; | ||
|
|
||
| /** | ||
| * Replaces all escaped backslashes in format string with replacement character. | ||
| * | ||
| * @param formatString | ||
| * @return Modified format string. | ||
| */ | ||
| const replaceDoubleBacklash = (formatString: string): string => { | ||
| const pattern = new RegExp(DOUBLE_BACKSLASH_REGEX, "g"); | ||
| return formatString.replaceAll(pattern, REPLACEMENT_CHARACTER); | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Testing coverage needed for string manipulation utilities
The codebase lacks dedicated test files for the YScopeFormatter's string manipulation utilities. These utilities handle critical string transformations including escape sequences and backslash handling, which warrant thorough testing to ensure reliability.
Key areas requiring test coverage:
removeBackslashfunction for single backslash removalreplaceReplacementCharacterfunction for character substitutionremoveEscapeCharactersfunction for combined operationsreplaceDoubleBacklashfunction for escaped backslash handling
🔗 Analysis chain
Consider adding unit tests for edge cases
The string manipulation functions handle various escape sequences. Please ensure comprehensive testing for edge cases such as:
- Multiple consecutive backslashes
- Mixed escaped and unescaped characters
- Empty strings
Let me search for test files specifically to verify test coverage.
Let me try a broader search to find any test files in the project structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test coverage of edge cases
rg -l "test|spec" | xargs rg "removeBackslash|replaceReplacementCharacter|removeEscapeCharacters|replaceDoubleBacklash"
Length of output: 645
Script:
#!/bin/bash
# Search for test files related to YScopeFormatter
fd "test|spec" | grep -i "yscope"
# Check contents of any test files found
fd "test|spec" | grep -i "yscope" | xargs cat
Length of output: 79
Script:
#!/bin/bash
# Find all test files in the repository
fd -e test.ts -e spec.ts
# Look for any tests related to string formatting or utils
fd -e test.ts -e spec.ts | xargs rg -l "string|format|utils?"
Length of output: 2299
|
Reminder to mention this fixes #86. |
| helperText: `[JSON] Log messages conversion pattern. Add field-placeholders to insert | ||
| fields from JSON log events. A field-placeholder uses the following syntax: | ||
| \`{<field-name>[:<formatter-name>[:<formatter-options>]]}\`. \`field-name\` is required, | ||
| while \`formatter-name\` and \`formatter-options\` are optional. See the default pattern | ||
| for an example.`, |
There was a problem hiding this comment.
How about:
Log message conversion pattern: use field placeholders to insert values from JSON log events. The syntax is `{<field-name>[:<formatter-name>[:<formatter-options>]]}`, where `field-name` is required, while `formatter-name` and `formatter-options` are optional. Refer to the default pattern for an example.
There was a problem hiding this comment.
I got rid about the bit about the default pattern since the default pattern will be empty soon. I added a small example instead
There was a problem hiding this comment.
For example, the following placeholder would format a timestamp field with name @timestamp: {@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}.
src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts
Outdated
Show resolved
Hide resolved
src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts
Outdated
Show resolved
Hide resolved
src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts
Outdated
Show resolved
Hide resolved
src/typings/formatters.ts
Outdated
| /** | ||
| * Parsed field placeholder from a Yscope format string. | ||
| */ | ||
| type YScopeFieldPlaceholder = { |
There was a problem hiding this comment.
As per the naming conventions, let's change it as YscopeFieldPlaceholder.
src/typings/formatters.ts
Outdated
| formatLogEvent: (logEvent: LogEvent) => string | ||
| } | ||
|
|
||
| interface YScopeFieldFormatter { |
There was a problem hiding this comment.
As per the naming conventions, let's change it as YscopeFieldFormatter.
src/typings/formatters.ts
Outdated
| /** | ||
| * Type for list of currently supported Yscope field formatters. | ||
| */ | ||
| type YScopeFieldFormatterMap = { |
There was a problem hiding this comment.
As per the naming conventions, let's change it as YscopeFieldFormatterMap.
| console.log("Replacement character is an invalid character in format string." + | ||
| "Replacement character will appear as \\."); |
There was a problem hiding this comment.
| console.log("Replacement character is an invalid character in format string." + | |
| "Replacement character will appear as \\."); | |
| console.warn("Unicode REPLACEMENT CHARACTER `U+FFFD` is found in Decoder Format" + | |
| ' String, which will appear as "\\"'); |
src/typings/formatters.ts
Outdated
| /** | ||
| * Pattern to replace double backlash. | ||
| */ | ||
| const DOUBLE_BACKSLASH_REGEX = Object.freeze(/\\\\/); |
There was a problem hiding this comment.
Does this have to be a regex? If not, can we write
const DOUBLE_BACKSLASH_PATTERN = "\\\\";
There was a problem hiding this comment.
see other longer comment
src/typings/formatters.ts
Outdated
| /** | ||
| * Pattern to replace replacement character. | ||
| */ | ||
| const REPLACEMENT_CHARACTER_REGEX = Object.freeze(/�/); |
There was a problem hiding this comment.
Does this have to be a regex? If not, can we use the REPLACEMENT_CHARACTER from above?
There was a problem hiding this comment.
It dosent, but I think its cleaner that they are all regexes. This regex we could pass as a string into new Regex instead if passing regex. Like here in current use in code. const replacementCharacterPattern = new RegExp(REPLACEMENT_CHARACTER_REGEX, "g"); . But other regex, like this one,COLON_REGEX, we are using directly wihtout creating it since no global flag, like placeholderString.split(COLON_REGEX, 3); I think it's simpler to just leave them as regex? But let me know
There was a problem hiding this comment.
Right, split() makes a copy of the RegExp and sets the global flag on the copy, so we don't explicitly need the global flag set.
Back to whether we want to replace the RegExp objects with pure strings, I feel the replacement is worthy. If I didn't understand wrong from the MDN docs, the string manipulation functions (e.g. replaceAll) are different symbols when we call it with a regular string v.s. when we call it with a RegExp object. I assume the former's implementation is faster is most cases. (With that said, in general we shouldn't make changes based on assumptions though benchmarking this seems trivial. )
src/typings/formatters.ts
Outdated
| * replace escaped backslashes with a rare character and add them back after parsing field | ||
| * placeholder with regex is finished. | ||
| */ | ||
| const REPLACEMENT_CHARACTER = Object.freeze("�"); |
There was a problem hiding this comment.
Correct me if I'm wrong - strings are already immutable in JS. Does this have to be Object.freezed?
There was a problem hiding this comment.
I think your right... changed
src/typings/formatters.ts
Outdated
| /** | ||
| * Pattern to remove single backlash. | ||
| */ | ||
| const SINGLE_BACKSLASH_REGEX = Object.freeze(/\\/); |
There was a problem hiding this comment.
Does this have to be a regex? If not, can we write
const SINGLE_BACKSLASH_CHARACTER = "\\";
There was a problem hiding this comment.
See previous longer comment. I like consistency, but this one we can change as well if you prefer
| formatLogEvent (logEvent: LogEvent): string { | ||
| let formattedLog = ""; | ||
|
|
||
| // Keeps track of the last position in format string. | ||
| let lastIndex = 0; | ||
|
|
||
| for (const fieldPlaceholder of this.#fieldPlaceholders) { | ||
| const formatStringFragment = | ||
| this.#processedFormatString.slice(lastIndex, fieldPlaceholder.range.start); | ||
| const cleanedFragment = removeEscapeCharacters(formatStringFragment); | ||
|
|
||
| formattedLog += cleanedFragment; | ||
|
|
||
| formattedLog += getFormattedField(logEvent, fieldPlaceholder); | ||
| lastIndex = fieldPlaceholder.range.end; | ||
| } | ||
|
|
||
| const remainder = this.#processedFormatString.slice(lastIndex); | ||
| formattedLog += removeEscapeCharacters(remainder); | ||
|
|
||
| return `${formattedLog}\n`; | ||
| } |
There was a problem hiding this comment.
I agree with the Rabbit on the const fragments: string[] = []; proposal. The try-catch is unnecessary.
There was a problem hiding this comment.
Someone could import the global regex elsewhere in the code base multiple times, which could lead to a bug with last index.
Right.
We could maybe declare some readonly on the formatter class, create them in constructor, and reuse them? I think the rabbit suggested this.
Sure. I think this is the way to go.
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | ||
|
|
||
| const pattern = new RegExp(BACKSLASH_REGEX, "g"); | ||
|
|
||
| // Remove escape characters (`\`) after the field name is split. | ||
| fieldNameKeys = fieldNameKeys.map((key) => key.replaceAll(pattern, "")); |
There was a problem hiding this comment.
Right, the original comment is about breaking the code into testable pieces. At this point if we are not checking in test cases, the code is fine to be left as-is.
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
src/typings/formatters.ts (3)
7-22: Consider enhancing the documentation with more examplesThe documentation is clear but could benefit from:
- Examples of formatter options usage
- Examples of escaped characters (e.g.,
\:,\},\\)- Example of a complete format string with multiple placeholders
40-49: Fix parameter name in JSDocThe JSDoc parameter name
logEventdoesn't match the method signature parameter namefield./** * Formats the given field. * - * @param logEvent + * @param field * @return The formatted field. */ formatField: (field: JsonValue) => string
72-72: Remove unnecessary Object.freeze calls
Object.freezeis unnecessary for regex literals and string constants as they are already immutable. Consider removing these calls to improve code clarity.-const REPLACEMENT_CHARACTER = Object.freeze("�"); +const REPLACEMENT_CHARACTER = "�"; -const REPLACEMENT_CHARACTER_REGEX = Object.freeze(/�/); +const REPLACEMENT_CHARACTER_REGEX = /�/; -const DOUBLE_BACKSLASH_REGEX = Object.freeze(/\\\\/); +const DOUBLE_BACKSLASH_REGEX = /\\\\/; -const SINGLE_BACKSLASH_REGEX = Object.freeze(/\\/); +const SINGLE_BACKSLASH_REGEX = /\\/; -const COLON_REGEX = Object.freeze(/(?<!\\):/); +const COLON_REGEX = /(?<!\\):/; -const FIELD_PLACEHOLDER_REGEX = Object.freeze(/(?<!\\)\{(.*?)(?<!\\)\}/); +const FIELD_PLACEHOLDER_REGEX = /(?<!\\)\{(.*?)(?<!\\)\}/; -const PERIOD_REGEX = Object.freeze(/(?<!\\)\./); +const PERIOD_REGEX = /(?<!\\)\./;Also applies to: 79-79, 84-84, 89-89, 94-94, 99-99, 104-104
src/services/formatters/YScopeFormatter/utils.ts (5)
35-74: Consider memoizing the string replacement results.For scenarios where the same input strings are processed repeatedly, consider memoizing the results of these string manipulation functions to avoid redundant processing.
Example implementation:
const memoizedRemoveBackslash = (() => { const cache = new Map<string, string>(); return (str: string): string => { if (cache.has(str)) { return cache.get(str)!; } const result = removeBackslash(str); cache.set(str, result); return result; }; })();
83-88: Enhance documentation for edge cases.The function handles undefined values differently, but this behaviour should be more explicitly documented. Consider updating the JSDoc to include examples of different input types and their expected outputs, especially for edge cases like
null,undefined, and empty objects.
98-112: Consider making the default value configurable.Currently, undefined fields are always converted to an empty string. Consider making this default value configurable to support different formatting requirements.
Example implementation:
const getFormattedField = ( logEvent: LogEvent, fieldPlaceholder: YScopeFieldPlaceholder, defaultValue: string = "" ): string => { const nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys); if ("undefined" === typeof nestedValue) { return defaultValue; } // ... rest of the implementation };
144-148: Fix formatting issues.Address the linting errors by adding a trailing comma and proper line breaks.
let [ fieldName, formatterName, - formatterOptions - ]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3); + formatterOptions, + ]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3);🧰 Tools
🪛 GitHub Check: lint-check
[warning] 147-147:
Missing trailing comma
[failure] 148-148:
A linebreak is required before ']'
151-153: Enhance error message with context.The error message could be more helpful by including the actual placeholder string that failed to parse.
- throw Error("Field name could not be parsed"); + throw Error(`Field name could not be parsed from placeholder: "${placeholderString}"`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts(1 hunks)src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts(1 hunks)src/services/formatters/YScopeFormatter/utils.ts(1 hunks)src/typings/formatters.ts(1 hunks)src/utils/config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts
- src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts
- src/utils/config.ts
🧰 Additional context used
🪛 GitHub Check: lint-check
src/services/formatters/YScopeFormatter/utils.ts
[warning] 147-147:
Missing trailing comma
[failure] 148-148:
A linebreak is required before ']'
🔇 Additional comments (4)
src/typings/formatters.ts (2)
105-121: LGTM! Well-organized exports
The exports are clearly separated into types and constants, following TypeScript best practices.
94-94: Verify browser compatibility for lookbehind assertions
The regex patterns use negative lookbehind assertions ((?<!\\)). Please ensure compatibility with all target browsers, as lookbehind support varies.
Also applies to: 104-104
src/services/formatters/YScopeFormatter/utils.ts (2)
1-25: LGTM! Well-structured imports and constant definition.
The imports are well-organized, and the YSCOPE_FIELD_FORMATTER_MAP is properly documented and immutable. The map correctly includes the two formatters mentioned in the PR objectives.
174-181: LGTM! Well-organized exports.
All necessary functions are exported, and the exports are well-organized.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts (2)
7-11: Consider enhancing documentation with examplesThe documentation clearly explains the purpose but would benefit from concrete examples showing:
- Input/output for numerical values
- Handling of non-numerical values
- Edge cases (e.g., undefined, null)
Add examples like:
/** * Examples: * - formatField(42.6) => "43" * - formatField(42.4) => "42" * - formatField("hello") => "hello" * - formatField(null) => "null" */
19-26: Consider these improvements to the formatting method
- Use modern type checking syntax
- Add bounds checking for large numbers
- Consider making the method static since it doesn't use instance properties
Here's a suggested improvement:
- // eslint-disable-next-line class-methods-use-this - formatField (field: JsonValue): string { - if ("number" === typeof field) { - field = Math.round(field); - } - - return jsonValueToString(field); - } + static formatField (field: JsonValue): string { + if (typeof field !== 'number') { + return jsonValueToString(field); + } + + if (Math.abs(field) > Number.MAX_SAFE_INTEGER) { + return jsonValueToString(field); + } + + return jsonValueToString(Math.round(field)); + }src/services/formatters/YscopeFormatter/FieldFormatters/TimestampFormatter.ts (3)
9-12: Consider enhancing the documentationWhile the current documentation is clear, it would be beneficial to add:
- The expected format of the
optionsparameter- Example usage with different date-time patterns
Example enhancement:
/** * A formatter for timestamp values, using a specified date-time pattern. * Options: If no pattern is provided, defaults to ISO 8601 format. + * @param options - Date-time pattern string (e.g., 'YYYY-MM-DD HH:mm:ss') + * @example + * const formatter = new TimestampFormatter('YYYY-MM-DD'); + * const result = formatter.formatField('2024-01-01T12:00:00Z'); + * // result: '2024-01-01' */
16-18: Consider adding type validation for options parameterThe constructor could benefit from runtime type validation to ensure the date format string is valid.
constructor (options: Nullable<string>) { + if (options !== null && typeof options !== 'string') { + throw new Error('Date format must be a string or null'); + } this.#dateFormat = options; }
21-26: Consider creating an issue to track the performance optimizationThe TODO comment identifies a valid performance concern regarding repeated timestamp parsing. This should be tracked for future optimization.
Would you like me to create a GitHub issue to track this performance optimization task?
src/typings/formatters.ts (1)
7-22: Documentation needs clarity improvementsThe documentation has a few areas that could be improved:
- Line 20: The phrase "All three as" is unclear and seems to be a typo
- Missing an example for period escaping mentioned in line 15
Consider updating the documentation with this diff:
* - Nested fields can be specified using periods (`.`) to denote hierarchy. E.g., the field * `{"a:" {"b": 0}}` may be denoted by `a.b`. - * - To denote a field-name with periods escape the periods with a backslashes. + * - To denote a field-name with periods, escape them with backslashes. E.g., `a\.b` refers to the field + * `{"a.b": 0}`. * - <formatter-name> (optional) is the name of the formatter to apply to the value before * inserting it into the string. * - <formatter-options> (optional) defines any options for the formatter denoted by formatter-name. * - * All three as may contain any character, except that colons (:), right braces (}), + * All components may contain any character, except that colons (:), right braces (}), * and backslashes (\) must be escaped with a backslash.src/services/formatters/YscopeFormatter/index.ts (3)
30-33: Use proper error handling instead ofconsole.logIn the constructor, when detecting an invalid replacement character in the format string, using
console.logmay not be appropriate for production code. Consider throwing an exception or utilizing a logging framework to provide clearer feedback to the caller.
74-76: Clarify error handling for invalid regex matchesThe error message in
#parseFieldPlaceholderstates that the field placeholder regex is invalid if there is no capture group:if ("undefined" === typeof groupMatch) { throw Error("Field placeholder regex is invalid and does not have a capture group"); }This situation could indicate an internal issue with the regex rather than a user input error. Ensure that this error handling appropriately reflects the source of the problem and consider whether this check is necessary in the production code or if it should be addressed during development.
94-96: Ensure safe access to match indicesIn the assignment of the
rangeproperty:range: { start: match.index, end: match.index + fullMatch.length, },Although
match.indexshould always be defined when a match is found, it might be prudent to include a check or assertion to handle any unexpectedundefinedvalues, enhancing the robustness of the code.src/services/formatters/YscopeFormatter/utils.ts (1)
128-128: Usecomponent === undefinedfor clarityIn TypeScript, it's more idiomatic and clear to compare directly to
undefinedusingcomponent === undefinedrather than usingtypeof.Update the condition as follows:
-if ("undefined" === typeof component || 0 === component.length) { +if (component === undefined || component.length === 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/components/modals/SettingsModal/SettingsDialog.tsx(1 hunks)src/services/decoders/ClpIrDecoder.ts(3 hunks)src/services/decoders/JsonlDecoder/index.ts(3 hunks)src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts(1 hunks)src/services/formatters/YscopeFormatter/FieldFormatters/TimestampFormatter.ts(1 hunks)src/services/formatters/YscopeFormatter/index.ts(1 hunks)src/services/formatters/YscopeFormatter/utils.ts(1 hunks)src/typings/formatters.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/modals/SettingsModal/SettingsDialog.tsx
- src/services/decoders/ClpIrDecoder.ts
- src/services/decoders/JsonlDecoder/index.ts
🧰 Additional context used
📓 Learnings (2)
src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts (1)
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/formatters/YScopeFormatter/FieldFormatters/RoundFormatter.ts:21-23
Timestamp: 2024-11-18T01:32:52.411Z
Learning: Infinity and NaN are not valid JSON values, so handling these cases in field formatters like `RoundFormatter` is unnecessary.
src/services/formatters/YscopeFormatter/FieldFormatters/TimestampFormatter.ts (1)
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#123
File: src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts:24-36
Timestamp: 2024-11-18T01:34:54.885Z
Learning: In `src/services/formatters/YScopeFormatter/FieldFormatters/TimestampFormatter.ts`, the `convertToDayjsTimestamp` function already includes error handling for invalid inputs, so wrapping it in try-catch in the `TimestampFormatter.formatField` method is unnecessary.
🔇 Additional comments (10)
src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts (2)
12-17: LGTM! Clear error handling for unsupported options
The constructor correctly implements the "no options" requirement with clear error messaging.
29-29: Export statement looks good
src/services/formatters/YscopeFormatter/FieldFormatters/TimestampFormatter.ts (3)
1-7: LGTM! Well-organized imports
The imports are properly organized with external dependencies first, followed by internal imports, and all imports are being used.
20-32: LGTM! Clean implementation with proper error handling
The implementation is clean and leverages the error handling already present in convertToDayjsTimestamp as noted in the learnings. The conditional formatting based on dateFormat is well implemented.
27-31: Verify date format pattern validation
We should ensure that invalid date format patterns are properly handled.
✅ Verification successful
Invalid date formats are properly handled
The codebase already implements robust date format validation through the convertToDayjsTimestamp utility:
- Invalid timestamps are sanitized to a default value instead of showing "Invalid Date"
- The
TimestampFormatteruses this utility to safely handle timestamp formatting - When no format is specified, it defaults to ISO 8601 format
- The dayjs library's built-in format validation is utilized through
isValid()checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for date format pattern validation in the codebase
rg -A 5 "format.*pattern|dayjs.*format" --type typescript
# Search for any existing tests validating date formats
rg -A 5 "test.*format.*invalid|test.*invalid.*format" --type typescript
Length of output: 203
Script:
#!/bin/bash
# Search for date format pattern validation in the codebase
rg -A 5 "format.*pattern|dayjs.*format"
# Search for any existing tests validating date formats
rg -A 5 "test.*format.*invalid|test.*invalid.*format"
# Search for files containing 'TimestampFormatter'
fd TimestampFormatter
# Look for test files related to TimestampFormatter
fd -g "*test*.ts" -g "*test*.js" -X grep -l "TimestampFormatter"
Length of output: 1077
Script:
#!/bin/bash
# Get the content of TimestampFormatter to analyze validation
cat src/services/formatters/YscopeFormatter/FieldFormatters/TimestampFormatter.ts
# Search for test files with a different pattern
find . -type f -name "*test*.ts" -o -name "*test*.js" | xargs grep -l "TimestampFormatter"
# Look for dayjs usage and validation
rg "dayjs" -A 5
Length of output: 8453
src/typings/formatters.ts (3)
40-67: LGTM! Well-structured type definitions
The field formatter types and interfaces are well-designed with clear separation of concerns and proper TypeScript type safety.
105-121: LGTM! Well-organized exports
The exports are properly structured and include all necessary types, interfaces, and constants.
94-104: Verify regex patterns with comprehensive test cases
The regex patterns for parsing format strings are complex and handle various edge cases. We should ensure they work correctly together.
Let's verify the regex patterns with some test cases:
✅ Verification successful
Let me search for existing tests and implementation details to better understand how these regex patterns are used.
Let me check how these regex patterns are used in actual formatting logic to ensure they handle all cases correctly.
Based on the code analysis, I can now provide a final response:
Regex patterns are properly implemented with comprehensive validation
The regex patterns are well-designed and properly used throughout the codebase with:
- Proper escape character handling through
removeEscapeCharacters - Validation of field names, formatter names, and options via
validateComponent - Error handling for invalid field names and format strings
- Support for nested fields using period-separated keys
- Proper handling of escaped special characters (colons, periods, braces)
The implementation in YscopeFormatter and its utilities shows that these regex patterns are working as intended, with proper error handling and edge case management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Create a test file and verify regex patterns
cat << 'EOF' > test_patterns.ts
import {
COLON_REGEX,
FIELD_PLACEHOLDER_REGEX,
PERIOD_REGEX
} from './src/typings/formatters';
// Test cases
const tests = [
'{field}',
'{field:formatter}',
'{field.nested}',
'{field\\.with\\.dots}',
'{field\\:with\\:colons}',
'{nested.field:formatter:options}'
];
console.log('Testing patterns...');
tests.forEach(test => {
console.log(`\nTest case: ${test}`);
console.log('Field placeholder:', test.match(FIELD_PLACEHOLDER_REGEX));
console.log('Colon splits:', test.split(COLON_REGEX));
console.log('Period splits:', test.split(PERIOD_REGEX));
});
EOF
# Find test files
echo "Searching for existing test files:"
fd -e test.ts -e spec.ts formatters
Length of output: 122
Script:
#!/bin/bash
# Search for test files in the entire codebase
fd -e test.ts -e spec.ts
# Find implementation details of how these regex patterns are used
rg -A 5 "COLON_REGEX|FIELD_PLACEHOLDER_REGEX|PERIOD_REGEX" --type ts
# Look for any formatter-related test files
rg -l "formatter" --type ts | grep -i "test\|spec"
Length of output: 4962
Script:
#!/bin/bash
# Look for the formatter implementation and test files
rg -A 10 "class YscopeFormatter" --type ts
# Check for any validation or error handling related to these patterns
rg -A 5 "validateComponent|removeEscapeCharacters" --type ts
# Look for any documentation about format string syntax
fd README -e md | xargs rg -i "format.*string|placeholder"
Length of output: 5779
src/services/formatters/YscopeFormatter/index.ts (1)
55-55: Review the addition of a newline character at the end of the formatted string
The formatLogEvent method appends a newline character to the formatted log output:
return `${formattedLogFragments.join("")}\n`;Adding a newline may not be suitable for all use cases, especially if the caller expects to handle line breaks explicitly. Consider removing the newline from the formatter and allowing the caller to decide whether to add it.
Run the following script to identify how formatLogEvent is used elsewhere in the codebase and assess whether the appended newline causes any issues:
src/services/formatters/YscopeFormatter/utils.ts (1)
62-66: Function removeEscapeCharacters is well-implemented
The removeEscapeCharacters function correctly removes escape characters while preserving escaped backslashes, adhering to the intended logic.
| const FieldFormatterConstructor = YSCOPE_FIELD_FORMATTER_MAP[formatterName]; | ||
| if ("undefined" === typeof FieldFormatterConstructor) { | ||
| throw Error(`Formatter ${formatterName} is not currently supported`); | ||
| } | ||
| fieldFormatter = new FieldFormatterConstructor(formatterOptions); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle unsupported formatter types more gracefully
When an unsupported formatter is encountered, the code throws an error:
if ("undefined" === typeof FieldFormatterConstructor) {
throw Error(`Formatter ${formatterName} is not currently supported`);
}This could cause the entire application to crash if the format string includes an unknown formatter. Consider handling this scenario more gracefully by notifying the user about the unsupported formatter without throwing an uncaught exception. Possible approaches include:
- Logging a warning and skipping the unsupported formatter.
- Providing a default formatter as a fallback.
- Returning the placeholder as-is in the formatted output.
|
|
||
| // Initialize commonly used regular expressions to facilitate reuse. | ||
| const singleBackslashPattern = new RegExp(SINGLE_BACKSLASH_REGEX, "g"); | ||
| const doubleBacklashPattern = new RegExp(DOUBLE_BACKSLASH_REGEX, "g"); |
There was a problem hiding this comment.
Fix typo: 'doubleBacklash' should be 'doubleBackslash'
The variable and function names doubleBacklashPattern and replaceDoubleBacklash contain a typo. The correct term is 'doubleBackslash'. This typo could lead to confusion and potential errors in the codebase.
Apply this diff to correct the typos:
-const doubleBacklashPattern = new RegExp(DOUBLE_BACKSLASH_REGEX, "g");
+const doubleBackslashPattern = new RegExp(DOUBLE_BACKSLASH_REGEX, "g");
...
-const replaceDoubleBacklash = (formatString: string): string => {
+const replaceDoubleBackslash = (formatString: string): string => {
- return formatString.replaceAll(doubleBacklashPattern, REPLACEMENT_CHARACTER);
+ return formatString.replaceAll(doubleBackslashPattern, REPLACEMENT_CHARACTER);
...
export {
getFormattedField,
jsonValueToString,
removeEscapeCharacters,
- replaceDoubleBacklash,
+ replaceDoubleBackslash,
splitFieldPlaceholder,
YSCOPE_FIELD_FORMATTER_MAP,
};Also applies to: 79-80, 187-187
| let [ | ||
| fieldName, | ||
| formatterName, | ||
| formatterOptions, | ||
| ]: Nullable<string|undefined>[ | ||
| ] = placeholderString.split(COLON_REGEX, 3); |
There was a problem hiding this comment.
Fix TypeScript syntax error in destructuring assignment
The type annotation in the destructuring assignment is incorrectly applied. Type annotations should be on the variable, not directly on the destructured elements. This will result in a syntax error.
Modify the code as follows to ensure correct syntax:
-let [
- fieldName,
- formatterName,
- formatterOptions,
-]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3);
+const components: Nullable<string | undefined>[] = placeholderString.split(COLON_REGEX, 3);
+let [
+ fieldName,
+ formatterName,
+ formatterOptions,
+] = components;Alternatively, you can use type assertion:
-let [
- fieldName,
- formatterName,
- formatterOptions,
-]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3);
+let [
+ fieldName,
+ formatterName,
+ formatterOptions,
+] = placeholderString.split(COLON_REGEX, 3) as Nullable<string | undefined>[];This ensures the code compiles without errors.
📝 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.
| let [ | |
| fieldName, | |
| formatterName, | |
| formatterOptions, | |
| ]: Nullable<string|undefined>[ | |
| ] = placeholderString.split(COLON_REGEX, 3); | |
| const components: Nullable<string | undefined>[] = placeholderString.split(COLON_REGEX, 3); | |
| let [ | |
| fieldName, | |
| formatterName, | |
| formatterOptions, | |
| ] = components; | |
| ``` | |
| Option 2 (Using type assertion): | |
| ```suggestion | |
| let [ | |
| fieldName, | |
| formatterName, | |
| formatterOptions, | |
| ] = placeholderString.split(COLON_REGEX, 3) as Nullable<string | undefined>[]; |
src/services/formatters/YscopeFormatter/FieldFormatters/RoundFormatter.ts
Outdated
Show resolved
Hide resolved
| const singleBackslashPattern = new RegExp(SINGLE_BACKSLASH_REGEX, "g"); | ||
| const doubleBacklashPattern = new RegExp(DOUBLE_BACKSLASH_REGEX, "g"); |
There was a problem hiding this comment.
I understand these are no longer exported; however, we still want to name all globals in SCREAMING_SNAKE_CASE.
Co-authored-by: Junhao Liao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
src/typings/formatters.ts (1)
7-22: Clarify escaping rules in documentationThe documentation should explicitly state that a double backslash (
\\) is required to escape special characters. Consider adding an example to illustrate this.Add this clarification after line 21:
* All three as may contain any character, except that colons (:), right braces (}), * and backslashes (\) must be escaped with a backslash. +* For example, to include a literal colon, use `\\:`. To include a literal backslash, use `\\\\`.src/services/formatters/YscopeFormatter/utils.ts (3)
61-65: Consider adding input validationThe
removeEscapeCharactersfunction should validate its input parameter to handle null or undefined cases gracefully.Apply this diff:
const removeEscapeCharacters = (str: string): string => { + if (!str) { + return str; + } // `removeBackslash()`, which removes all backlashes, is called before // `replaceReplacementCharacter()` to prevent removal of escaped backslashes. return replaceReplacementCharacter(removeBackslash(str)); };
89-94: Enhance type checking and edge case handlingThe current implementation could be more robust with precise type checking and null handling.
Apply this diff:
const jsonValueToString = (input: JsonValue | undefined): string => { - // Behaviour is different for `undefined`. - return "object" === typeof input ? - JSON.stringify(input) : - String(input); + if (input === null || input === undefined) { + return ""; + } + if (typeof input === "object") { + return JSON.stringify(input); + } + return String(input); };
126-132: Improve type handling in validateComponentThe function could be more explicit in handling different types of empty values.
Apply this diff:
const validateComponent = (component: string | undefined): Nullable<string> => { - if ("undefined" === typeof component || 0 === component.length) { + if (component === undefined || component === null || component.trim().length === 0) { return null; } - return component; + return component.trim(); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/services/formatters/YscopeFormatter/index.ts(1 hunks)src/services/formatters/YscopeFormatter/utils.ts(1 hunks)src/typings/formatters.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/formatters/YscopeFormatter/index.ts
🔇 Additional comments (1)
src/services/formatters/YscopeFormatter/utils.ts (1)
181-188: LGTM! Exports are well organized
The exports section clearly specifies all necessary utilities and follows a consistent format.
| export type { | ||
| Formatter, | ||
| FormatterOptionsType, | ||
| LogbackFormatterOptionsType, | ||
| YscopeFieldFormatter, | ||
| YscopeFieldFormatterMap, | ||
| YscopeFieldPlaceholder, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update export names to match interface changes
Ensure the exported type names match the standardized YScope naming convention.
Formatter,
FormatterOptionsType,
- YscopeFieldFormatter,
- YscopeFieldFormatterMap,
- YscopeFieldPlaceholder,
+ YScopeFieldFormatter,
+ YScopeFieldFormatterMap,
+ YScopeFieldPlaceholder,📝 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.
| export type { | |
| Formatter, | |
| FormatterOptionsType, | |
| LogbackFormatterOptionsType, | |
| YscopeFieldFormatter, | |
| YscopeFieldFormatterMap, | |
| YscopeFieldPlaceholder, | |
| }; | |
| export type { | |
| Formatter, | |
| FormatterOptionsType, | |
| YScopeFieldFormatter, | |
| YScopeFieldFormatterMap, | |
| YScopeFieldPlaceholder, | |
| }; |
| interface YscopeFormatterOptionsType { | ||
| formatString: string, | ||
| } | ||
|
|
||
| type FormatterOptionsType = LogbackFormatterOptionsType; | ||
| type FormatterOptionsType = YscopeFormatterOptionsType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize naming convention: "Yscope" vs "YScope"
As per previous review discussions and naming conventions, the interface should be renamed to maintain consistency.
-interface YscopeFormatterOptionsType {
+interface YScopeFormatterOptionsType {
formatString: string,
}
-type FormatterOptionsType = YscopeFormatterOptionsType;
+type FormatterOptionsType = YScopeFormatterOptionsType;📝 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.
| interface YscopeFormatterOptionsType { | |
| formatString: string, | |
| } | |
| type FormatterOptionsType = LogbackFormatterOptionsType; | |
| type FormatterOptionsType = YscopeFormatterOptionsType; | |
| interface YScopeFormatterOptionsType { | |
| formatString: string, | |
| } | |
| type FormatterOptionsType = YScopeFormatterOptionsType; |
| interface YscopeFieldFormatter { | ||
|
|
||
| /** | ||
| * Formats the given field. | ||
| * | ||
| * @param logEvent | ||
| * @return The formatted field. | ||
| */ | ||
| formatField: (field: JsonValue) => string | ||
| } | ||
|
|
||
| /** | ||
| * Type for list of currently supported Yscope field formatters. | ||
| */ | ||
| type YscopeFieldFormatterMap = { | ||
| [key: string]: new (options: Nullable<string>) => YscopeFieldFormatter; | ||
| }; | ||
|
|
||
| /** | ||
| * Parsed field placeholder from a Yscope format string. | ||
| */ | ||
| type YscopeFieldPlaceholder = { | ||
| fieldNameKeys: string[], | ||
| fieldFormatter: Nullable<YscopeFieldFormatter>, | ||
|
|
||
| // Location of field placeholder in format string including braces. | ||
| range: {start: number, end: number} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize YScope naming in types and interfaces
Continue the naming convention standardization throughout these types.
-interface YscopeFieldFormatter {
+interface YScopeFieldFormatter {
-type YscopeFieldFormatterMap = {
- [key: string]: new (options: Nullable<string>) => YscopeFieldFormatter;
+type YScopeFieldFormatterMap = {
+ [key: string]: new (options: Nullable<string>) => YScopeFieldFormatter;
-type YscopeFieldPlaceholder = {
+type YScopeFieldPlaceholder = {📝 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.
| interface YscopeFieldFormatter { | |
| /** | |
| * Formats the given field. | |
| * | |
| * @param logEvent | |
| * @return The formatted field. | |
| */ | |
| formatField: (field: JsonValue) => string | |
| } | |
| /** | |
| * Type for list of currently supported Yscope field formatters. | |
| */ | |
| type YscopeFieldFormatterMap = { | |
| [key: string]: new (options: Nullable<string>) => YscopeFieldFormatter; | |
| }; | |
| /** | |
| * Parsed field placeholder from a Yscope format string. | |
| */ | |
| type YscopeFieldPlaceholder = { | |
| fieldNameKeys: string[], | |
| fieldFormatter: Nullable<YscopeFieldFormatter>, | |
| // Location of field placeholder in format string including braces. | |
| range: {start: number, end: number} | |
| } | |
| interface YScopeFieldFormatter { | |
| /** | |
| * Formats the given field. | |
| * | |
| * @param logEvent | |
| * @return The formatted field. | |
| */ | |
| formatField: (field: JsonValue) => string | |
| } | |
| /** | |
| * Type for list of currently supported Yscope field formatters. | |
| */ | |
| type YScopeFieldFormatterMap = { | |
| [key: string]: new (options: Nullable<string>) => YScopeFieldFormatter; | |
| }; | |
| /** | |
| * Parsed field placeholder from a Yscope format string. | |
| */ | |
| type YScopeFieldPlaceholder = { | |
| fieldNameKeys: string[], | |
| fieldFormatter: Nullable<YScopeFieldFormatter>, | |
| // Location of field placeholder in format string including braces. | |
| range: {start: number, end: number} | |
| } |
| const COLON_REGEX = Object.freeze(/(?<!\\):/); | ||
|
|
||
| /** | ||
| * Pattern to match unescaped field placeholder. | ||
| */ | ||
| const FIELD_PLACEHOLDER_REGEX = Object.freeze(/(?<!\\)\{(.*?)(?<!\\)\}/); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the package.json and README.md analysis, I can now provide the final response:
Browser compatibility issue requires attention
The regex patterns using negative lookbehind assertions are incompatible with Safari browsers before version 16.4, which could affect users. Since this is a web-based project using React and being deployed as a browser-based log viewer (as evidenced by the online demo), this compatibility issue needs to be addressed.
- Consider refactoring the regex patterns in
src/typings/formatters.tsto avoid using negative lookbehind assertions - Alternative approach: Use positive lookahead with string manipulation or split-join operations instead
🔗 Analysis chain
Verify browser compatibility for negative lookbehind assertions
The regex patterns use negative lookbehind assertions ((?<!\\)), which might not be supported in all browsers (particularly Safari versions before 16.4).
Run this script to check browser compatibility requirements in package.json:
Let me gather more information about the project's browser compatibility requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check browserslist configuration
cat package.json | jq '.browserslist' 2>/dev/null || \
cat .browserslistrc 2>/dev/null || \
echo "No browserslist configuration found"
Length of output: 42
Script:
#!/bin/bash
# Check if this is a browser-based project by looking for web-related dependencies and build tools
cat package.json | jq '.dependencies, .devDependencies'
# Also check if there's any documentation about browser support
fd README.md | xargs cat
Length of output: 5273
| const SINGLE_BACKSLASH_REGEX = new RegExp(SINGLE_BACKSLASH, "g"); | ||
| const DOUBLE_BACKSLASH_REGEX = new RegExp(DOUBLE_BACKSLASH, "g"); | ||
| const REPLACEMENT_CHARACTER_REGEX = new RegExp(REPLACEMENT_CHARACTER, "g"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use SCREAMING_SNAKE_CASE for regex pattern constants
As per the previous review comment, all global constants should follow SCREAMING_SNAKE_CASE naming convention.
Apply this diff:
-const SINGLE_BACKSLASH_REGEX = new RegExp(SINGLE_BACKSLASH, "g");
-const DOUBLE_BACKSLASH_REGEX = new RegExp(DOUBLE_BACKSLASH, "g");
-const REPLACEMENT_CHARACTER_REGEX = new RegExp(REPLACEMENT_CHARACTER, "g");
+const SINGLE_BACKSLASH_REGEX_PATTERN = new RegExp(SINGLE_BACKSLASH, "g");
+const DOUBLE_BACKSLASH_REGEX_PATTERN = new RegExp(DOUBLE_BACKSLASH, "g");
+const REPLACEMENT_CHARACTER_REGEX_PATTERN = new RegExp(REPLACEMENT_CHARACTER, "g");📝 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 SINGLE_BACKSLASH_REGEX = new RegExp(SINGLE_BACKSLASH, "g"); | |
| const DOUBLE_BACKSLASH_REGEX = new RegExp(DOUBLE_BACKSLASH, "g"); | |
| const REPLACEMENT_CHARACTER_REGEX = new RegExp(REPLACEMENT_CHARACTER, "g"); | |
| const SINGLE_BACKSLASH_REGEX_PATTERN = new RegExp(SINGLE_BACKSLASH, "g"); | |
| const DOUBLE_BACKSLASH_REGEX_PATTERN = new RegExp(DOUBLE_BACKSLASH, "g"); | |
| const REPLACEMENT_CHARACTER_REGEX_PATTERN = new RegExp(REPLACEMENT_CHARACTER, "g"); |
| const getFormattedField = ( | ||
| logEvent: LogEvent, | ||
| fieldPlaceholder: YscopeFieldPlaceholder | ||
| ): string => { | ||
| const nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys); | ||
| if ("undefined" === typeof nestedValue) { | ||
| return ""; | ||
| } | ||
|
|
||
| const formattedField = fieldPlaceholder.fieldFormatter ? | ||
| fieldPlaceholder.fieldFormatter.formatField(nestedValue) : | ||
| jsonValueToString(nestedValue); | ||
|
|
||
| return formattedField; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for field formatter operations
The field formatting operation should be wrapped in a try-catch block to handle potential formatting errors gracefully.
Apply this diff:
const getFormattedField = (
logEvent: LogEvent,
fieldPlaceholder: YscopeFieldPlaceholder
): string => {
const nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys);
if ("undefined" === typeof nestedValue) {
return "";
}
+ try {
const formattedField = fieldPlaceholder.fieldFormatter ?
fieldPlaceholder.fieldFormatter.formatField(nestedValue) :
jsonValueToString(nestedValue);
+ return formattedField;
+ } catch (error) {
+ console.error(`Error formatting field: ${error.message}`);
+ return String(nestedValue);
+ }
- return formattedField;
};📝 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 getFormattedField = ( | |
| logEvent: LogEvent, | |
| fieldPlaceholder: YscopeFieldPlaceholder | |
| ): string => { | |
| const nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys); | |
| if ("undefined" === typeof nestedValue) { | |
| return ""; | |
| } | |
| const formattedField = fieldPlaceholder.fieldFormatter ? | |
| fieldPlaceholder.fieldFormatter.formatField(nestedValue) : | |
| jsonValueToString(nestedValue); | |
| return formattedField; | |
| }; | |
| const getFormattedField = ( | |
| logEvent: LogEvent, | |
| fieldPlaceholder: YscopeFieldPlaceholder | |
| ): string => { | |
| const nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys); | |
| if ("undefined" === typeof nestedValue) { | |
| return ""; | |
| } | |
| try { | |
| const formattedField = fieldPlaceholder.fieldFormatter ? | |
| fieldPlaceholder.fieldFormatter.formatField(nestedValue) : | |
| jsonValueToString(nestedValue); | |
| return formattedField; | |
| } catch (error) { | |
| console.error(`Error formatting field: ${error.message}`); | |
| return String(nestedValue); | |
| } | |
| }; |
| const splitFieldPlaceholder = (placeholderString: string): { | ||
| fieldNameKeys: string[], | ||
| formatterName: Nullable<string>, | ||
| formatterOptions: Nullable<string>, | ||
| } => { | ||
| let [ | ||
| fieldName, | ||
| formatterName, | ||
| formatterOptions, | ||
| ]: Nullable<string|undefined>[ | ||
| ] = placeholderString.split(COLON_REGEX, 3); | ||
|
|
||
| fieldName = validateComponent(fieldName); | ||
| if (null === fieldName) { | ||
| throw Error("Field name could not be parsed"); | ||
| } | ||
|
|
||
| // Splits field name into an array of field name keys to support nested fields. | ||
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | ||
|
|
||
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | ||
|
|
||
| formatterName = validateComponent(formatterName); | ||
| if (null !== formatterName) { | ||
| formatterName = removeEscapeCharacters(formatterName); | ||
| } | ||
|
|
||
| formatterOptions = validateComponent(formatterOptions); | ||
| if (null !== formatterOptions) { | ||
| formatterOptions = removeEscapeCharacters(formatterOptions); | ||
| } | ||
|
|
||
| return {fieldNameKeys, formatterName, formatterOptions}; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in splitFieldPlaceholder
The function should provide more specific error messages and validate formatter compatibility.
Apply this diff:
const splitFieldPlaceholder = (placeholderString: string): {
fieldNameKeys: string[],
formatterName: Nullable<string>,
formatterOptions: Nullable<string>,
} => {
+ if (!placeholderString) {
+ throw new Error("Placeholder string cannot be empty");
+ }
+
let [
fieldName,
formatterName,
formatterOptions,
] = placeholderString.split(COLON_REGEX, 3);
fieldName = validateComponent(fieldName);
if (null === fieldName) {
- throw Error("Field name could not be parsed");
+ throw new Error("Invalid field name: must not be empty");
}
// Splits field name into an array of field name keys to support nested fields.
let fieldNameKeys = fieldName.split(PERIOD_REGEX);
fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key));
formatterName = validateComponent(formatterName);
if (null !== formatterName) {
formatterName = removeEscapeCharacters(formatterName);
+ // Validate formatter exists
+ if (formatterName && !(formatterName in YSCOPE_FIELD_FORMATTER_MAP)) {
+ throw new Error(`Unknown formatter: ${formatterName}`);
+ }
}
formatterOptions = validateComponent(formatterOptions);
if (null !== formatterOptions) {
formatterOptions = removeEscapeCharacters(formatterOptions);
}
return {fieldNameKeys, formatterName, formatterOptions};
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/services/formatters/YscopeFormatter/utils.ts (1)
18-24: LGTM! Consider enhancing formatter documentationThe constant definition and immutability pattern look good. Consider adding JSDoc for each formatter type to document their specific formatting behaviour.
Add documentation for each formatter:
/** * List of currently supported field formatters. + * + * @property {typeof TimestampFormatter} timestamp - Formats Unix timestamps + * @property {typeof RoundFormatter} round - Rounds numeric values */ const YSCOPE_FIELD_FORMATTER_MAP: YscopeFieldFormatterMap = Object.freeze({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/services/formatters/YscopeFormatter/utils.ts(1 hunks)
🔇 Additional comments (4)
src/services/formatters/YscopeFormatter/utils.ts (4)
34-75: LGTM! Well-documented string manipulation utilities
The implementation of backslash and escape character handling is robust and thoroughly documented. The approach of using a replacement character for escaped backslashes is a clever solution to handle complex escape sequences.
176-183: LGTM! Well-organized exports
The exports section clearly exposes all necessary utilities and maintains a clean public API.
145-150:
Fix TypeScript syntax error in destructuring assignment
The type annotation in the destructuring assignment needs to be corrected.
Apply this fix:
- let [
- fieldName,
- formatterName,
- formatterOptions,
- ]: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3);
+ const components: Nullable<string|undefined>[] = placeholderString.split(COLON_REGEX, 3);
+ let [fieldName, formatterName, formatterOptions] = components;Likely invalid or redundant comment.
99-113: 🛠️ Refactor suggestion
Add error handling for field formatting operations
The field formatting logic looks good, but it should handle potential errors from formatters gracefully.
Add error handling:
const getFormattedField = (
logEvent: LogEvent,
fieldPlaceholder: YscopeFieldPlaceholder
): string => {
const nestedValue = getNestedJsonValue(logEvent.fields, fieldPlaceholder.fieldNameKeys);
if ("undefined" === typeof nestedValue) {
return "";
}
+ try {
const formattedField = fieldPlaceholder.fieldFormatter ?
fieldPlaceholder.fieldFormatter.formatField(nestedValue) :
jsonValueToString(nestedValue);
return formattedField;
+ } catch (error) {
+ console.error(`Error formatting field: ${error.message}`);
+ return String(nestedValue);
+ }
};Likely invalid or redundant comment.
| fieldName = validateComponent(fieldName); | ||
| if (null === fieldName) { | ||
| throw Error("Field name could not be parsed"); | ||
| } | ||
|
|
||
| // Splits field name into an array of field name keys to support nested fields. | ||
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | ||
|
|
||
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | ||
|
|
||
| formatterName = validateComponent(formatterName); | ||
| if (null !== formatterName) { | ||
| formatterName = removeEscapeCharacters(formatterName); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance formatter validation
Add validation to ensure the formatter exists in YSCOPE_FIELD_FORMATTER_MAP.
Add formatter validation:
formatterName = validateComponent(formatterName);
if (null !== formatterName) {
formatterName = removeEscapeCharacters(formatterName);
+ if (!(formatterName in YSCOPE_FIELD_FORMATTER_MAP)) {
+ throw new Error(`Unsupported formatter: ${formatterName}`);
+ }
}📝 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.
| fieldName = validateComponent(fieldName); | |
| if (null === fieldName) { | |
| throw Error("Field name could not be parsed"); | |
| } | |
| // Splits field name into an array of field name keys to support nested fields. | |
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | |
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | |
| formatterName = validateComponent(formatterName); | |
| if (null !== formatterName) { | |
| formatterName = removeEscapeCharacters(formatterName); | |
| } | |
| fieldName = validateComponent(fieldName); | |
| if (null === fieldName) { | |
| throw Error("Field name could not be parsed"); | |
| } | |
| // Splits field name into an array of field name keys to support nested fields. | |
| let fieldNameKeys = fieldName.split(PERIOD_REGEX); | |
| fieldNameKeys = fieldNameKeys.map((key) => removeEscapeCharacters(key)); | |
| formatterName = validateComponent(formatterName); | |
| if (null !== formatterName) { | |
| formatterName = removeEscapeCharacters(formatterName); | |
| if (!(formatterName in YSCOPE_FIELD_FORMATTER_MAP)) { | |
| throw new Error(`Unsupported formatter: ${formatterName}`); | |
| } | |
| } |
davemarco
changed the title
davemarco
changed the title
Description
Adds a new Yscope Formatter based on spec in notion.
The format string is parsed once in the formatter class constructor. For each placeholder, we create
an object and store it in a class-level list. The object contains the field name, and a formatter for that field name.
Each field formatter type has its own class. For now, we just have timestamp and round.
Afterwards, when log events are decoded, we iterate through array of placeholders to format the log event fields
Future Work
(1) We will need a way for users to see docs for new format string syntax in another PR.
(2) Add support for default format string
Validation performed
Tested with test event in notion.
Summary by CodeRabbit
New Features
RoundFormatterandTimestampFormatterfor improved numerical and timestamp formatting.getNestedJsonValue, for retrieving nested values from JSON objects.YscopeFormatterclass for formatting log events with a YScope format string.Bug Fixes
Chores
LogbackFormatterand associated documentation.