-
Notifications
You must be signed in to change notification settings - Fork 14
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
Changes from 14 commits
c29025d
e44d47f
71ee56c
d406e51
b55ebab
19c31d5
9ea0006
47bd40e
ebfa463
9d5ae88
c74eb59
c9a48e0
61b4769
087548d
01a3889
2acd20a
acf7a2c
197125a
cd23262
267b1f9
2385919
de03857
c6c65a1
2bba5ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString, | ||
label: "Decoder: Format string", | ||
name: LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it used in logback formatter, so I reused but curious There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. In my opinion, using Dayjs's datetime syntax in our own syntax is fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kirkrodrigues - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
|
||
|
||
/** | ||
* 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. | ||
* | ||
* @param pattern | ||
* @return The corresponding Day.js date format string. | ||
*/ | ||
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; | ||
}; | ||
|
||
/** | ||
* A formatter that uses a Logback-like format string to format log events into a string. See | ||
* `LogbackFormatterOptionsType` for details about the format string. | ||
|
@@ -51,12 +34,6 @@ class LogbackFormatter implements Formatter { | |
this.#parseKeys(); | ||
} | ||
|
||
/** | ||
* Formats the given log event. | ||
* | ||
* @param logEvent | ||
* @return The formatted log event. | ||
*/ | ||
formatLogEvent (logEvent: LogEvent): string { | ||
const {fields, timestamp} = logEvent; | ||
const formatStringWithTimestamp: string = | ||
|
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,39 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {Dayjs} from "dayjs"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {Nullable} from "../../../../typings/common"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {YScopeFieldFormatter} from "../../../../typings/formatters"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {JsonValue} from "../../../../typings/js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {convertDateTimeFormatterPatternToDayJs} from "../../../../utils/formatters"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (null !== options) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.#dateFormat = convertDateTimeFormatterPatternToDayJs(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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convert to dayjs already has error handling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davemarco, thank you for the clarification. Since (_/) ✏️ Learnings added
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default TimestampFormatter; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,109 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BACKSLASH_REGEX, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
FIELD_PLACEHOLDER_REGEX, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Formatter, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
FormatterOptionsType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
YScopeFieldFormatter, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
YScopeFieldPlaceholder, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} from "../../../typings/formatters"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import {LogEvent} from "../../../typings/logs"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
getFormattedField, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit picking - any better names than YscopeFormatter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JsonFormatter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline - it seems we can't find a better name than |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#formatString: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#fieldPlaceholders: YScopeFieldPlaceholder[] = []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
constructor (options: FormatterOptionsType) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.#formatString = options.formatString; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.#parseFieldPlaceholder(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add input validation in the constructor The constructor should validate the 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
formatLogEvent (logEvent: LogEvent): string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const placeholderPattern = new RegExp(FIELD_PLACEHOLDER_REGEX, "g"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const backslashPattern = new RegExp(BACKSLASH_REGEX, "g"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let formattedLog = ""; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Keeps track of the last position in format string. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let lastIndex = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for (const fieldPlaceholder of this.#fieldPlaceholders) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const placeholderMatch = placeholderPattern.exec(this.#formatString); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (null === placeholderMatch) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw Error("Insufficient placeholder quantity: format string was modified"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const notPlaceholder = this.#formatString.slice(lastIndex, placeholderMatch.index); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const cleanedNotPlaceholder = notPlaceholder.replaceAll(backslashPattern, ""); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
formattedLog += cleanedNotPlaceholder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
formattedLog += getFormattedField(logEvent, fieldPlaceholder); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
lastIndex = placeholderMatch.index + placeholderMatch[0].length; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const remainder = this.#formatString.slice(lastIndex); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const cleanedRemainder = remainder.replace(backslashPattern, ""); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
formattedLog += cleanedRemainder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return `${formattedLog}\n`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error handling and validation in parseFieldPlaceholder Consider these improvements:
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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default YScopeFormatter; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Length of output: 490