Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(formatter): Add YScope formatter for structured logs and remove Logback-style formatter. #123

Merged
merged 24 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/components/modals/SettingsModal/SettingsDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: add this on the docs site once the site infra is ready

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai let's open a GitHub issue for this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

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.`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@davemarco davemarco Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid about the bit about the default pattern since the default pattern will be empty soon. I added a small example instead

Copy link
Contributor Author

@davemarco davemarco Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, the following placeholder would format a timestamp field with name @timestamp: {@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}.

initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString,
label: "Decoder: Format string",
name: LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING,
Expand Down
6 changes: 3 additions & 3 deletions src/services/decoders/ClpIrDecoder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
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";
import {
convertToDayjsTimestamp,
isJsonObject,
Expand All @@ -39,7 +39,7 @@ class ClpIrDecoder implements Decoder {
this.#streamType = streamType;
this.#streamReader = streamReader;
this.#formatter = (streamType === CLP_IR_STREAM_TYPE.STRUCTURED) ?
new LogbackFormatter({formatString: decoderOptions.formatString}) :
new YScopeFormatter({formatString: decoderOptions.formatString}) :
null;
}

Expand Down Expand Up @@ -87,7 +87,7 @@ class ClpIrDecoder implements Decoder {
}

setFormatterOptions (options: DecoderOptions): boolean {
this.#formatter = new LogbackFormatter({formatString: options.formatString});
this.#formatter = new YScopeFormatter({formatString: options.formatString});

return true;
}
Expand Down
6 changes: 3 additions & 3 deletions src/services/decoders/JsonlDecoder/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
LogEvent,
LogLevelFilter,
} from "../../../typings/logs";
import LogbackFormatter from "../../formatters/LogbackFormatter";
import YscopeFormatter from "../../formatters/YScopeFormatter";
import {
convertToDayjsTimestamp,
convertToLogLevelValue,
Expand Down Expand Up @@ -53,7 +53,7 @@ class JsonlDecoder implements Decoder {
this.#dataArray = dataArray;
this.#logLevelKey = decoderOptions.logLevelKey;
this.#timestampKey = decoderOptions.timestampKey;
this.#formatter = new LogbackFormatter({formatString: decoderOptions.formatString});
this.#formatter = new YscopeFormatter({formatString: decoderOptions.formatString});
}

getEstimatedNumEvents (): number {
Expand Down Expand Up @@ -82,7 +82,7 @@ class JsonlDecoder implements Decoder {
}

setFormatterOptions (options: DecoderOptions): boolean {
this.#formatter = new LogbackFormatter({formatString: options.formatString});
this.#formatter = new YscopeFormatter({formatString: options.formatString});

return true;
}
Expand Down
152 changes: 0 additions & 152 deletions src/services/formatters/LogbackFormatter.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can be wrong but the plan is to remove this formatter, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay i will delete it. I wasn't sure what the plan was.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import {Nullable} from "../../../../typings/common";
import {YScopeFieldFormatter} from "../../../../typings/formatters";
import {JsonValue} from "../../../../typings/js";
import {jsonValueToString} from "../utils";


/**
* 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.
*/
davemarco marked this conversation as resolved.
Show resolved Hide resolved
class RoundFormatter implements YScopeFieldFormatter {
constructor (options: Nullable<string>) {
if (null !== options) {
throw Error(`Round formatter does not support option ${options}`);
davemarco marked this conversation as resolved.
Show resolved Hide resolved
}
}

// eslint-disable-next-line class-methods-use-this
formatField (field: JsonValue): string {
if ("number" === typeof field) {
field = Math.round(field);
}
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved

return jsonValueToString(field);
}
}

export default RoundFormatter;
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import {Dayjs} from "dayjs";

import {Nullable} from "../../../../typings/common";
import {YScopeFieldFormatter} from "../../../../typings/formatters";
import {JsonValue} from "../../../../typings/js";
import {convertToDayjsTimestamp} from "../../../decoders/JsonlDecoder/utils";


/**
* A formatter that formats timestamp values using a specified date-time pattern.
* If no pattern is provided as an option, the timestamp is formatted in the
* default ISO 8601 format.
*/
davemarco marked this conversation as resolved.
Show resolved Hide resolved
class TimestampFormatter implements YScopeFieldFormatter {
#dateFormat: Nullable<string> = null;

constructor (options: Nullable<string>) {
this.#dateFormat = options;
}

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);
}
Copy link

@coderabbitai coderabbitai bot Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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}`);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to dayjs already has error handling

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}

export default TimestampFormatter;
105 changes: 105 additions & 0 deletions src/services/formatters/YScopeFormatter/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import {Nullable} from "../../../typings/common";
import {
FIELD_PLACEHOLDER_REGEX,
Formatter,
FormatterOptionsType,
REPLACEMENT_CHARACTER_REGEX,
YScopeFieldFormatter,
YScopeFieldPlaceholder,
} from "../../../typings/formatters";
import {LogEvent} from "../../../typings/logs";
import {
getFormattedField,
removeEscapeCharacters,
replaceDoubleBacklash,
splitFieldPlaceholder,
YSCOPE_FORMATTERS_MAP,
} from "./utils";


/**
* 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit picking - any better names than YscopeFormatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonFormatter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline - it seems we can't find a better name than YscopeFormatter at the moment. Let's stick with YscopeFormatter then.

readonly #processedFormatString: string;

#fieldPlaceholders: YScopeFieldPlaceholder[] = [];

constructor (options: FormatterOptionsType) {
if (REPLACEMENT_CHARACTER_REGEX.test(options.formatString)) {
console.log("Replacement character is an invalid character in format string." +
"Replacement character will appear as \\.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 "\\"');

}

this.#processedFormatString = replaceDoubleBacklash(options.formatString);
this.#parseFieldPlaceholder();
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add input validation and improve warning message

Two improvements needed:

  1. Add input validation for the format string
  2. Make the warning message more specific about the replacement character

Apply this diff:

 constructor (options: FormatterOptionsType) {
+    if (!options?.formatString?.trim()) {
+        throw new Error("Format string must not be empty");
+    }
+
     if (REPLACEMENT_CHARACTER_REGEX.test(options.formatString)) {
-        console.log("Replacement character is an invalid character in format string." +
-             "Replacement character will appear as \\.");
+        console.warn(
+            "Found Unicode replacement character (U+FFFD) in format string. " +
+            "This character will be displayed as '\\' in the output."
+        );
     }

     this.#processedFormatString = replaceDoubleBacklash(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.

Suggested change
constructor (options: FormatterOptionsType) {
if (REPLACEMENT_CHARACTER_REGEX.test(options.formatString)) {
console.log("Replacement character is an invalid character in format string." +
"Replacement character will appear as \\.");
}
this.#processedFormatString = replaceDoubleBacklash(options.formatString);
this.#parseFieldPlaceholder();
}
constructor (options: FormatterOptionsType) {
if (!options?.formatString?.trim()) {
throw new Error("Format string must not be empty");
}
if (REPLACEMENT_CHARACTER_REGEX.test(options.formatString)) {
console.warn(
"Found Unicode replacement character (U+FFFD) in format string. " +
"This character will be displayed as '\\' in the output."
);
}
this.#processedFormatString = replaceDoubleBacklash(options.formatString);
this.#parseFieldPlaceholder();
}


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`;
}
Copy link

@coderabbitai coderabbitai bot Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize string concatenation and add error handling

Consider these improvements:

  1. Use array join instead of string concatenation
  2. Add error handling for malformed log events

Apply this diff:

 formatLogEvent (logEvent: LogEvent): string {
-    let formattedLog = "";
+    const fragments: string[] = [];
     let lastIndex = 0;

     for (const fieldPlaceholder of this.#fieldPlaceholders) {
+        try {
             const formatStringFragment =
                 this.#processedFormatString.slice(lastIndex, fieldPlaceholder.range.start);
-            const cleanedFragment = removeEscapeCharacters(formatStringFragment);
-            formattedLog += cleanedFragment;
+            fragments.push(removeEscapeCharacters(formatStringFragment));
+            fragments.push(getFormattedField(logEvent, fieldPlaceholder));
             lastIndex = fieldPlaceholder.range.end;
+        } catch (error) {
+            console.warn(
+                `Failed to format field "${fieldPlaceholder.fieldNameKeys.join(".")}":`,
+                error
+            );
+            fragments.push("[ERROR]");
+        }
     }

     const remainder = this.#processedFormatString.slice(lastIndex);
-    formattedLog += removeEscapeCharacters(remainder);
+    fragments.push(removeEscapeCharacters(remainder));

-    return `${formattedLog}\n`;
+    return `${fragments.join("")}\n`;
 }
📝 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.

Suggested change
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`;
}
formatLogEvent (logEvent: LogEvent): string {
const fragments: string[] = [];
let lastIndex = 0;
for (const fieldPlaceholder of this.#fieldPlaceholders) {
try {
const formatStringFragment =
this.#processedFormatString.slice(lastIndex, fieldPlaceholder.range.start);
fragments.push(removeEscapeCharacters(formatStringFragment));
fragments.push(getFormattedField(logEvent, fieldPlaceholder));
lastIndex = fieldPlaceholder.range.end;
} catch (error) {
console.warn(
`Failed to format field "${fieldPlaceholder.fieldNameKeys.join(".")}":`,
error
);
fragments.push("[ERROR]");
}
}
const remainder = this.#processedFormatString.slice(lastIndex);
fragments.push(removeEscapeCharacters(remainder));
return `${fragments.join("")}\n`;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the Rabbit on the const fragments: string[] = []; proposal. The try-catch is unnecessary.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@junhaoliao, understood. Here's the updated formatLogEvent method without the try-catch block:

formatLogEvent (logEvent: LogEvent): string {
    const fragments: string[] = [];
    let lastIndex = 0;

    for (const fieldPlaceholder of this.#fieldPlaceholders) {
        const formatStringFragment =
            this.#processedFormatString.slice(lastIndex, fieldPlaceholder.range.start);
        fragments.push(removeEscapeCharacters(formatStringFragment));
        fragments.push(getFormattedField(logEvent, fieldPlaceholder));
        lastIndex = fieldPlaceholder.range.end;
    }

    const remainder = this.#processedFormatString.slice(lastIndex);
    fragments.push(removeEscapeCharacters(remainder));

    return `${fragments.join("")}\n`;
}

This optimizes string concatenation using an array and join, and omits the unnecessary try-catch block.


/**
* Parses field name, formatter name, and formatter options from placeholders in the format
* string. For each placeholder, it creates a corresponding `YScopeFieldFormatter` and adds
* it to the class-level array.
*
* @throws Error if `FIELD_PLACEHOLDER_REGEX` does not contain a capture group.
* @throws Error if a specified formatter is not supported.
*/
#parseFieldPlaceholder () {
const placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g");
const it = this.#processedFormatString.matchAll(placeholderPattern);
for (const match of it) {
// `fullMatch` includes braces and `groupMatch` excludes them.
const [fullMatch, groupMatch]: (string | undefined) [] = match;

if ("undefined" === typeof groupMatch) {
throw Error("Field placeholder regex is invalid and does not have a capture group");
}

const {fieldNameKeys, formatterName, formatterOptions} =
splitFieldPlaceholder(groupMatch);

let fieldFormatter: Nullable<YScopeFieldFormatter> = null;
if (null !== formatterName) {
const FieldFormatterConstructor = YSCOPE_FORMATTERS_MAP[formatterName];
if ("undefined" === typeof FieldFormatterConstructor) {
throw Error(`Formatter ${formatterName} is not currently supported`);
}
fieldFormatter = new FieldFormatterConstructor(formatterOptions);
}

this.#fieldPlaceholders.push({
fieldNameKeys: fieldNameKeys,
fieldFormatter: fieldFormatter,
range: {
start: match.index,
end: match.index + fullMatch.length,
},
});
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize regex usage and add validation

Consider these improvements:

  1. Move regex pattern to class-level constant
  2. Add validation for format string content

Apply this diff:

 class YScopeFormatter implements Formatter {
+    static readonly #PLACEHOLDER_PATTERN = new RegExp(FIELD_PLACEHOLDER_REGEX, "g");
     readonly #processedFormatString: string;
     #fieldPlaceholders: YScopeFieldPlaceholder[] = [];

     #parseFieldPlaceholder () {
+        if (!this.#processedFormatString.includes("{")) {
+            throw new Error("Format string contains no placeholders");
+        }
-        const placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g");
-        const it = this.#processedFormatString.matchAll(placeholderPattern);
+        const it = this.#processedFormatString.matchAll(YScopeFormatter.#PLACEHOLDER_PATTERN);
+        let hasPlaceholders = false;
         for (const match of it) {
+            hasPlaceholders = true;
             // ... rest of the code ...
         }
+        if (!hasPlaceholders) {
+            throw new Error("No valid placeholders found in format string");
+        }
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

}

export default YScopeFormatter;
Loading
Loading