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): Display entire log event as JSON by default and remind users to set format string. #129

Merged
merged 9 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
5 changes: 3 additions & 2 deletions src/components/CentralContainer/Sidebar/SidebarTabs/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {
forwardRef,
useState,
useContext,
} from "react";

import {
Expand All @@ -13,6 +13,7 @@ import InfoOutlinedIcon from "@mui/icons-material/InfoOutlined";
import SearchIcon from "@mui/icons-material/Search";
import SettingsOutlinedIcon from "@mui/icons-material/SettingsOutlined";

import {StateContext} from "../../../../contexts/StateContextProvider";
import {TAB_NAME} from "../../../../typings/tab";
import SettingsModal from "../../../modals/SettingsModal";
import FileInfoTabPanel from "./FileInfoTabPanel";
Expand Down Expand Up @@ -51,7 +52,7 @@ const SidebarTabs = forwardRef<HTMLDivElement, SidebarTabsProps>((
},
tabListRef
) => {
const [isSettingsModalOpen, setIsSettingsModalOpen] = useState<boolean>(false);
const {isSettingsModalOpen, setIsSettingsModalOpen} = useContext(StateContext);

const handleSettingsModalClose = () => {
setIsSettingsModalOpen(false);
Expand Down
13 changes: 12 additions & 1 deletion src/components/PopUps/PopUpMessageBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import {
Alert,
Box,
Button,
CircularProgress,
IconButton,
Typography,
Expand Down Expand Up @@ -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;
Copy link
Collaborator

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:

Displays a pop-up message in an alert box with an optional action button. The pop-up can be manually dismissed or will automatically close after the specified `timeoutMillis`. If `timeoutMillis` is `0`, the pop-up will remain open until manually closed.


const {handlePopUpMessageClose} = useContext(NotificationContext);
const [percentRemaining, setPercentRemaining] = useState<number>(100);
Expand Down Expand Up @@ -113,6 +114,16 @@ const PopUpMessageBox = ({message}: PopUpMessageProps) => {
<Typography level={"body-sm"}>
{messageStr}
</Typography>
{button && (
<Button
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@davemarco davemarco Nov 27, 2024

Choose a reason for hiding this comment

The 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...?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the confusion. I meant in the UI, we can have something like

.pop-up-message-box-actions-container {
    display: flex;
    justify-content: flex-end;
}
<Box className="pop-up-message-box-actions-container">
    <Button/>
</Box>

Then it would look like
image

className={"pop-up-message-box-callback-button"}
color={color}
variant={"solid"}
onClick={button.callback}
>
{button.title}
</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following the type change in src/typings/notifications.ts, how about

Suggested change
onClick={button.callback}
>
{button.title}
</Button>
{...primaryAction}/>

)}
</div>
</Alert>
);
Expand Down
4 changes: 4 additions & 0 deletions src/components/PopUps/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
width: 300px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

Suggested change
width: 300px;
width: 333px;

}

.pop-up-message-box-callback-button {
margin-top: 10px !important;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this, can we change

.pop-up-message-box-alert-layout {
...
    display: flex;
    flex-direction: column;
    gap: 10px;
}

}

.pop-up-message-box-title-container {
display: flex;
align-items: center;
Expand Down
3 changes: 2 additions & 1 deletion src/components/modals/SettingsModal/SettingsDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

Suggested change
\`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. The default setting is
an empty string which will print all fields formatted as JSON.`,
\`{@timestamp:timestamp:YYYY-MM-DD HH\\:mm\\:ss.SSS}\`. Leave this blank to display the
entire log event formatted as JSON.`,

initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString,
label: "Decoder: Format string",
name: LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING,
Expand Down
29 changes: 27 additions & 2 deletions src/contexts/StateContextProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
} 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";
Expand All @@ -36,6 +39,7 @@
NavigationAction,
} from "../utils/actions";
import {
CONFIG_DEFAULT,
EXPORT_LOGS_CHUNK_SIZE,
getConfig,
} from "../utils/config";
Expand All @@ -56,8 +60,9 @@

interface StateContextType {
beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap,
fileName: string,
exportProgress: Nullable<number>,
fileName: string,
isSettingsModalOpen: boolean,
uiState: UI_STATE,
logData: string,
numEvents: number,
Expand All @@ -70,6 +75,7 @@
exportLogs: () => void,
loadFile: (fileSrc: FileSrcType, cursor: CursorType) => void,
loadPageByAction: (navAction: NavigationAction) => void,
setIsSettingsModalOpen: (isOpen: boolean) => void,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency, how about

Suggested change
setIsSettingsModalOpen: (isOpen: boolean) => void,
setIsSettingsModalOpen: (newIsOpen: boolean) => void,

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
}
Expand All @@ -82,6 +88,7 @@
beginLineNumToLogEventNum: new Map<number, number>(),
exportProgress: null,
fileName: "",
isSettingsModalOpen: false,
logData: "No file is open.",
numEvents: 0,
numPages: 0,
Expand All @@ -94,6 +101,7 @@
exportLogs: () => null,
loadFile: () => null,
loadPageByAction: () => null,
setIsSettingsModalOpen: () => null,
setLogLevelFilter: () => null,
startQuery: () => null,
});
Expand Down Expand Up @@ -236,6 +244,8 @@
// 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);
Expand Down Expand Up @@ -274,6 +284,19 @@
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); },
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following the type change in src/typings/notifications.ts, how about

Suggested change
button: {
title: "Open Settings",
callback: () => { setIsSettingsModalOpen(true); },
},
primaryAction: {
children: "Settings",
startDecorator: <SettingsOutlinedIcon/>,
onClick: () => { setIsSettingsModalOpen(true); },
},

level: LOG_LEVEL.INFO,
message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.",

Check failure on line 295 in src/contexts/StateContextProvider.tsx

View workflow job for this annotation

GitHub Actions / lint-check

This line has a length of 139. Maximum allowed is 100
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix line length to meet style guidelines

The message line exceeds the maximum length of 100 characters. Consider breaking it into multiple lines.

-        message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.",
+        message: "Adding one can enhance the readability of your structured logs by " +
+                "customizing how fields are displayed.",
📝 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
message: "Adding one can enhance the readability of your structured logs by customizing how fields are displayed.",
message: "Adding one can enhance the readability of your structured logs by " +
"customizing how fields are displayed.",
🧰 Tools
🪛 GitHub Check: lint-check

[failure] 295-295:
This line has a length of 139. Maximum allowed is 100

timeoutMillis: 2 * DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we extract a const from this, place this next to DEFAULT_AUTO_DISMISS_TIMEOUT_MILLIS, then export, and import here?

title: "A format string has not been configured",
});
}
break;
case WORKER_RESP_CODE.NOTIFICATION:
postPopUp({
Expand Down Expand Up @@ -510,6 +533,7 @@
beginLineNumToLogEventNum: beginLineNumToLogEventNumRef.current,
exportProgress: exportProgress,
fileName: fileName,
isSettingsModalOpen: isSettingsModalOpen,
logData: logData,
numEvents: numEvents,
numPages: numPages,
Expand All @@ -522,6 +546,7 @@
exportLogs: exportLogs,
loadFile: loadFile,
loadPageByAction: loadPageByAction,
setIsSettingsModalOpen: setIsSettingsModalOpen,
setLogLevelFilter: setLogLevelFilter,
startQuery: startQuery,
}}
Expand Down
6 changes: 6 additions & 0 deletions src/services/formatters/YscopeFormatter/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import {LogEvent} from "../../../typings/logs";
import {
getFormattedField,
jsonValueToString,
removeEscapeCharacters,
replaceDoubleBacklash,
splitFieldPlaceholder,
Expand Down Expand Up @@ -37,6 +38,11 @@ class YscopeFormatter implements Formatter {
}

formatLogEvent (logEvent: LogEvent): string {
// Empty format string is special case where formatter returns all fields as JSON.
if ("" === this.#processedFormatString) {
return jsonValueToString(logEvent.fields);
}

const formattedLogFragments: string[] = [];
let lastIndex = 0;

Expand Down
9 changes: 9 additions & 0 deletions src/typings/notifications.ts
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.
*/
Expand All @@ -9,6 +17,7 @@ interface PopUpMessage {
message: string,
timeoutMillis: number,
title: string,
button?: PopUpButton,
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about

import {ButtonProps} from "@mui/joy";
  1. Use ButtonProps instead of our own PopUpButton.
  2. Rename the prop as primaryAction. (That way it makes sense for us to add a secondary action button in the future, though not strictly needed for now. )

}

/**
Expand Down
9 changes: 5 additions & 4 deletions src/utils/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about checking the strings'.length against 0?

result = "Log level key cannot be empty.";
Copy link

@coderabbitai coderabbitai bot Nov 22, 2024

Choose a reason for hiding this comment

The 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:

  1. The string comparison could be simplified using string length
  2. Consider validating formatString to distinguish between "intentionally empty" and "accidentally empty"
  3. Consider reordering validations to check all fields and return multiple errors

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;

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not neccesary

Copy link

Choose a reason for hiding this comment

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

@davemarco, understood.

}
break;
case CONFIG_KEY.INITIAL_TAB_NAME:
Expand Down
Loading