-
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): Display entire log event as JSON by default and remind users to set format string. #129
feat(formatter): Display entire log event as JSON by default and remind users to set format string. #129
Changes from 3 commits
14a3114
5c1d573
1942e04
027f00d
f2959ae
78f336c
2092ca3
8855636
8de0b4c
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ import { | |||||||||||
import { | ||||||||||||
Alert, | ||||||||||||
Box, | ||||||||||||
Button, | ||||||||||||
CircularProgress, | ||||||||||||
IconButton, | ||||||||||||
Typography, | ||||||||||||
|
@@ -38,7 +39,7 @@ interface PopUpMessageProps { | |||||||||||
* @return | ||||||||||||
*/ | ||||||||||||
const PopUpMessageBox = ({message}: PopUpMessageProps) => { | ||||||||||||
const {id, level, message: messageStr, title, timeoutMillis} = message; | ||||||||||||
const {id, level, button, message: messageStr, title, timeoutMillis} = message; | ||||||||||||
|
||||||||||||
const {handlePopUpMessageClose} = useContext(NotificationContext); | ||||||||||||
const [percentRemaining, setPercentRemaining] = useState<number>(100); | ||||||||||||
|
@@ -113,6 +114,16 @@ const PopUpMessageBox = ({message}: PopUpMessageProps) => { | |||||||||||
<Typography level={"body-sm"}> | ||||||||||||
{messageStr} | ||||||||||||
</Typography> | ||||||||||||
{button && ( | ||||||||||||
<Button | ||||||||||||
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. Sorry i forgot to post this in the last batch of comments: can we right align this button? 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. what do you mean specifically, can u sugggest? also the linter appears to be formatting like this. Or do u mean in UI...? 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. |
||||||||||||
className={"pop-up-message-box-callback-button"} | ||||||||||||
color={color} | ||||||||||||
variant={"soft"} | ||||||||||||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
onClick={button.callback} | ||||||||||||
> | ||||||||||||
{button.title} | ||||||||||||
</Button> | ||||||||||||
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. Following the type change in
Suggested change
|
||||||||||||
)} | ||||||||||||
</div> | ||||||||||||
</Alert> | ||||||||||||
); | ||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -30,6 +30,10 @@ | |||||
width: 300px; | ||||||
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
Suggested change
|
||||||
} | ||||||
|
||||||
.pop-up-message-box-callback-button { | ||||||
margin-top: 10px !important; | ||||||
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. Instead of this, can we change
|
||||||
} | ||||||
|
||||||
.pop-up-message-box-title-container { | ||||||
display: flex; | ||||||
align-items: center; | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -38,7 +38,8 @@ const CONFIG_FORM_FIELDS = [ | |||||||||
\`{<field-name>[:<formatter-name>[:<formatter-options>]]}\`, where \`field-name\` is | ||||||||||
required, while \`formatter-name\` and \`formatter-options\` are optional. For example, | ||||||||||
the following placeholder would format a timestamp field with name \`@timestamp\`: | ||||||||||
\`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`.`, | ||||||||||
\`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. The default setting is | ||||||||||
an empty string which will print all fields formatted as JSON.`, | ||||||||||
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
Suggested change
|
||||||||||
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString, | ||||||||||
label: "Decoder: Format string", | ||||||||||
name: LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING, | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,7 +14,10 @@ import LogExportManager, { | |||||||||||||||||||
} from "../services/LogExportManager"; | ||||||||||||||||||||
import {Nullable} from "../typings/common"; | ||||||||||||||||||||
import {CONFIG_KEY} from "../typings/config"; | ||||||||||||||||||||
import {LogLevelFilter} from "../typings/logs"; | ||||||||||||||||||||
import { | ||||||||||||||||||||
LOG_LEVEL, | ||||||||||||||||||||
LogLevelFilter, | ||||||||||||||||||||
} from "../typings/logs"; | ||||||||||||||||||||
import {DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS} from "../typings/notifications"; | ||||||||||||||||||||
import {UI_STATE} from "../typings/states"; | ||||||||||||||||||||
import {SEARCH_PARAM_NAMES} from "../typings/url"; | ||||||||||||||||||||
|
@@ -36,6 +39,7 @@ import { | |||||||||||||||||||
NavigationAction, | ||||||||||||||||||||
} from "../utils/actions"; | ||||||||||||||||||||
import { | ||||||||||||||||||||
CONFIG_DEFAULT, | ||||||||||||||||||||
EXPORT_LOGS_CHUNK_SIZE, | ||||||||||||||||||||
getConfig, | ||||||||||||||||||||
} from "../utils/config"; | ||||||||||||||||||||
|
@@ -56,8 +60,9 @@ import { | |||||||||||||||||||
|
||||||||||||||||||||
interface StateContextType { | ||||||||||||||||||||
beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap, | ||||||||||||||||||||
fileName: string, | ||||||||||||||||||||
exportProgress: Nullable<number>, | ||||||||||||||||||||
fileName: string, | ||||||||||||||||||||
isSettingsModalOpen: boolean, | ||||||||||||||||||||
uiState: UI_STATE, | ||||||||||||||||||||
logData: string, | ||||||||||||||||||||
numEvents: number, | ||||||||||||||||||||
|
@@ -70,6 +75,7 @@ interface StateContextType { | |||||||||||||||||||
exportLogs: () => void, | ||||||||||||||||||||
loadFile: (fileSrc: FileSrcType, cursor: CursorType) => void, | ||||||||||||||||||||
loadPageByAction: (navAction: NavigationAction) => void, | ||||||||||||||||||||
setIsSettingsModalOpen: (isOpen: boolean) => void, | ||||||||||||||||||||
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. For consistency, how about
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. I prefer without new. I think the new for log level filter was just there to solve a namespace issue (i think loglevelfilter taken). Instead, i left as isOpen, and changed newLogLevelFilter to just filter. |
||||||||||||||||||||
setLogLevelFilter: (newLogLevelFilter: LogLevelFilter) => void, | ||||||||||||||||||||
startQuery: (queryString: string, isRegex: boolean, isCaseSensitive: boolean) => void, | ||||||||||||||||||||
} | ||||||||||||||||||||
|
@@ -82,6 +88,7 @@ const STATE_DEFAULT: Readonly<StateContextType> = Object.freeze({ | |||||||||||||||||||
beginLineNumToLogEventNum: new Map<number, number>(), | ||||||||||||||||||||
exportProgress: null, | ||||||||||||||||||||
fileName: "", | ||||||||||||||||||||
isSettingsModalOpen: false, | ||||||||||||||||||||
logData: "No file is open.", | ||||||||||||||||||||
numEvents: 0, | ||||||||||||||||||||
numPages: 0, | ||||||||||||||||||||
|
@@ -94,6 +101,7 @@ const STATE_DEFAULT: Readonly<StateContextType> = Object.freeze({ | |||||||||||||||||||
exportLogs: () => null, | ||||||||||||||||||||
loadFile: () => null, | ||||||||||||||||||||
loadPageByAction: () => null, | ||||||||||||||||||||
setIsSettingsModalOpen: () => null, | ||||||||||||||||||||
setLogLevelFilter: () => null, | ||||||||||||||||||||
startQuery: () => null, | ||||||||||||||||||||
}); | ||||||||||||||||||||
|
@@ -236,6 +244,8 @@ const StateContextProvider = ({children}: StateContextProviderProps) => { | |||||||||||||||||||
// States | ||||||||||||||||||||
const [exportProgress, setExportProgress] = | ||||||||||||||||||||
useState<Nullable<number>>(STATE_DEFAULT.exportProgress); | ||||||||||||||||||||
const [isSettingsModalOpen, setIsSettingsModalOpen] = | ||||||||||||||||||||
useState<boolean>(STATE_DEFAULT.isSettingsModalOpen); | ||||||||||||||||||||
const [fileName, setFileName] = useState<string>(STATE_DEFAULT.fileName); | ||||||||||||||||||||
const [logData, setLogData] = useState<string>(STATE_DEFAULT.logData); | ||||||||||||||||||||
const [numEvents, setNumEvents] = useState<number>(STATE_DEFAULT.numEvents); | ||||||||||||||||||||
|
@@ -274,6 +284,20 @@ const StateContextProvider = ({children}: StateContextProviderProps) => { | |||||||||||||||||||
setFileName(args.fileName); | ||||||||||||||||||||
setNumEvents(args.numEvents); | ||||||||||||||||||||
setOnDiskFileSizeInBytes(args.onDiskFileSizeInBytes); | ||||||||||||||||||||
if (getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString === | ||||||||||||||||||||
CONFIG_DEFAULT[CONFIG_KEY.DECODER_OPTIONS].formatString) { | ||||||||||||||||||||
postPopUp({ | ||||||||||||||||||||
button: { | ||||||||||||||||||||
title: "Open Settings", | ||||||||||||||||||||
callback: () => { setIsSettingsModalOpen(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. Following the type change in
Suggested change
|
||||||||||||||||||||
level: LOG_LEVEL.INFO, | ||||||||||||||||||||
message: "Input a custom format string in settings dialog" + | ||||||||||||||||||||
" to improve readability of JSON logs", | ||||||||||||||||||||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
timeoutMillis: 2 * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, | ||||||||||||||||||||
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. Can we extract a const from this, place this next to |
||||||||||||||||||||
title: "Format JSON Logs", | ||||||||||||||||||||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
}); | ||||||||||||||||||||
} | ||||||||||||||||||||
break; | ||||||||||||||||||||
case WORKER_RESP_CODE.NOTIFICATION: | ||||||||||||||||||||
postPopUp({ | ||||||||||||||||||||
|
@@ -510,6 +534,7 @@ const StateContextProvider = ({children}: StateContextProviderProps) => { | |||||||||||||||||||
beginLineNumToLogEventNum: beginLineNumToLogEventNumRef.current, | ||||||||||||||||||||
exportProgress: exportProgress, | ||||||||||||||||||||
fileName: fileName, | ||||||||||||||||||||
isSettingsModalOpen: isSettingsModalOpen, | ||||||||||||||||||||
logData: logData, | ||||||||||||||||||||
numEvents: numEvents, | ||||||||||||||||||||
numPages: numPages, | ||||||||||||||||||||
|
@@ -522,6 +547,7 @@ const StateContextProvider = ({children}: StateContextProviderProps) => { | |||||||||||||||||||
exportLogs: exportLogs, | ||||||||||||||||||||
loadFile: loadFile, | ||||||||||||||||||||
loadPageByAction: loadPageByAction, | ||||||||||||||||||||
setIsSettingsModalOpen: setIsSettingsModalOpen, | ||||||||||||||||||||
setLogLevelFilter: setLogLevelFilter, | ||||||||||||||||||||
startQuery: startQuery, | ||||||||||||||||||||
}} | ||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,14 @@ | ||
import {LOG_LEVEL} from "./logs"; | ||
|
||
|
||
/** | ||
* Optional button for pop-up. | ||
*/ | ||
type PopUpButton = { | ||
title: string, | ||
callback: () => void, | ||
} | ||
|
||
/** | ||
* Contents of pop-up messages and its associated auto dismiss timeout. | ||
*/ | ||
|
@@ -9,6 +17,7 @@ interface PopUpMessage { | |
message: string, | ||
timeoutMillis: number, | ||
title: string, | ||
button?: PopUpButton, | ||
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
|
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,7 @@ const QUERY_CHUNK_SIZE = 10_000; | |
*/ | ||
const CONFIG_DEFAULT: ConfigMap = Object.freeze({ | ||
[CONFIG_KEY.DECODER_OPTIONS]: { | ||
formatString: "{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}" + | ||
" {log\\.level} [{process\\.thread\\.name}] {message}", | ||
formatString: "", | ||
logLevelKey: "log.level", | ||
timestampKey: "@timestamp", | ||
}, | ||
|
@@ -42,8 +41,10 @@ const testConfig = ({key, value}: ConfigUpdate): Nullable<string> => { | |
let result = null; | ||
switch (key) { | ||
case CONFIG_KEY.DECODER_OPTIONS: | ||
if (Object.values(value).includes("")) { | ||
result = "Decoder options cannot be empty."; | ||
if ("" === value.timestampKey) { | ||
result = "Timestamp key cannot be empty."; | ||
} else if ("" === value.logLevelKey) { | ||
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 checking the strings' |
||
result = "Log level key cannot be empty."; | ||
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 Improve validation logic and user experience The current validation implementation has several areas for improvement:
Here's a suggested improvement: - if ("" === value.timestampKey) {
- result = "Timestamp key cannot be empty.";
- } else if ("" === value.logLevelKey) {
- result = "Log level key cannot be empty.";
+ const errors: string[] = [];
+ if (!value.timestampKey?.length) {
+ errors.push("Timestamp key cannot be empty");
+ }
+ if (!value.logLevelKey?.length) {
+ errors.push("Log level key cannot be empty");
+ }
+ if (!value.formatString?.length && !confirm("Are you sure you want to use empty format string? This will print all fields in JSON format.")) {
+ errors.push("Format string was left empty");
+ }
+ result = errors.length ? errors.join(". ") : null;
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. This is not neccesary 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, understood. |
||
} | ||
break; | ||
case CONFIG_KEY.INITIAL_TAB_NAME: | ||
|
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.
Shall we explain in the high-level docs: