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

new-log-viewer: Finish support for filtering by log-level and associated refactoring. #89

Merged
merged 45 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
9f002a8
first draft
davemarco Oct 3, 2024
f66481f
small changes
davemarco Oct 3, 2024
8edcbf6
small change
davemarco Oct 3, 2024
275edc0
clean up stuff
davemarco Oct 3, 2024
1accf00
some cleanup
davemarco Oct 3, 2024
75b5dec
move to index from num
davemarco Oct 4, 2024
4d79444
more changes
davemarco Oct 4, 2024
e6fc92f
things seem to work
davemarco Oct 4, 2024
5915bd1
fix lint
davemarco Oct 4, 2024
a1983d5
small change
davemarco Oct 4, 2024
e3b2b36
small changes
davemarco Oct 4, 2024
03f1670
Apply suggestions from code review
davemarco Oct 4, 2024
e904a28
Merge branch 'integrate' of github.com:davemarco/yscope-log-viewer-fo…
davemarco Oct 4, 2024
b112382
small change
davemarco Oct 4, 2024
e7ad6eb
clean up state context code
davemarco Oct 4, 2024
c13df92
fix lint
davemarco Oct 4, 2024
2cc0803
restructure to improve readability
davemarco Oct 4, 2024
04b2515
remove less old functions
davemarco Oct 4, 2024
a420d7e
cleanup
davemarco Oct 4, 2024
914a188
lint fix
davemarco Oct 4, 2024
45c2e42
small change
davemarco Oct 4, 2024
aec63d5
small change
davemarco Oct 4, 2024
934c7c6
Apply suggestions from code review
davemarco Oct 5, 2024
e4e4aee
local changes
davemarco Oct 5, 2024
26b0bf3
Merge branch 'integrate' of github.com:davemarco/yscope-log-viewer-fo…
davemarco Oct 5, 2024
6528c56
junhao review
davemarco Oct 5, 2024
e53a904
fix comments
davemarco Oct 5, 2024
3016d14
small change
davemarco Oct 5, 2024
93a46f9
fix lint
davemarco Oct 6, 2024
807066b
change to active num events
davemarco Oct 6, 2024
d03da58
move use Effect
davemarco Oct 6, 2024
dd9c92e
add decoder options
davemarco Oct 6, 2024
a87a9fc
Apply suggestions from code review
davemarco Oct 6, 2024
a4f7a0f
Apply suggestions from code review
davemarco Oct 7, 2024
b1e26e2
Apply suggestions from code review
davemarco Oct 7, 2024
ae97ab3
kirk comment
davemarco Oct 7, 2024
bdfd2e0
change falsy value from 0 to null for log event num
davemarco Oct 8, 2024
80ccfec
change to numActiveEvents
davemarco Oct 8, 2024
963cbd5
add concept of activeLogCollectionEvent
davemarco Oct 8, 2024
0808a7b
remove decoder options
davemarco Oct 8, 2024
6551edf
fix lint
davemarco Oct 8, 2024
217693a
change comment
davemarco Oct 8, 2024
6488bf8
small change
davemarco Oct 8, 2024
edbf7e1
Apply suggestions from code review
davemarco Oct 9, 2024
eb706d3
kirk review
davemarco Oct 9, 2024
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
101 changes: 74 additions & 27 deletions new-log-viewer/src/contexts/StateContextProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import React, {
import LogExportManager, {EXPORT_LOG_PROGRESS_VALUE_MIN} from "../services/LogExportManager";
import {Nullable} from "../typings/common";
import {CONFIG_KEY} from "../typings/config";
import {LogLevelFilter} from "../typings/logs";
import {SEARCH_PARAM_NAMES} from "../typings/url";
import {
BeginLineNumToLogEventNumMap,
Expand All @@ -32,9 +33,10 @@ import {
getConfig,
} from "../utils/config";
import {
clamp,
getChunkNum,
} from "../utils/math";
findNearestLessThanOrEqualElement,
isWithinBounds,
} from "../utils/data";
import {clamp} from "../utils/math";
import {
updateWindowUrlHashParams,
updateWindowUrlSearchParams,
Expand All @@ -56,6 +58,7 @@ interface StateContextType {
exportLogs: () => void,
loadFile: (fileSrc: FileSrcType, cursor: CursorType) => void,
loadPageByAction: (navAction: NavigationAction) => void,
setLogLevelFilter: (newLogLevelFilter: LogLevelFilter) => void,
}
const StateContext = createContext<StateContextType>({} as StateContextType);

Expand All @@ -74,6 +77,7 @@ const STATE_DEFAULT: Readonly<StateContextType> = Object.freeze({
exportLogs: () => null,
loadFile: () => null,
loadPageByAction: () => null,
setLogLevelFilter: () => null,
});

interface StateContextProviderProps {
Expand Down Expand Up @@ -166,10 +170,47 @@ const loadPageByCursor = (
) => {
workerPostReq(worker, WORKER_REQ_CODE.LOAD_PAGE, {
cursor: cursor,
decoderOptions: getConfig(CONFIG_KEY.DECODER_OPTIONS),
});
};

/**
* Updates the log event number in the URL to `logEventNum` if it's within the bounds of
* `logEventNumsOnPage`.
*
* @param logEventNum
* @param logEventNumsOnPage
* @return Whether `logEventNum` is within the bounds of `logEventNumsOnPage`.
*/
const updateUrlIfEventOnPage = (
logEventNum: number,
logEventNumsOnPage: number[]
): boolean => {
if (false === isWithinBounds(logEventNumsOnPage, logEventNum)) {
return false;
}

const nearestIdx = findNearestLessThanOrEqualElement(
logEventNumsOnPage,
logEventNum
);

// Since `isWithinBounds` returned `true`, then:
// - `logEventNumsOnPage` must bound `logEventNum`.
// - `logEventNumsOnPage` cannot be empty.
// - `nearestIdx` cannot be `null`.
//
// Therefore, we can safely cast:
// - `nearestIdx` from `Nullable<number>` to `number`.
// - `logEventNumsOnPage[nearestIdx]` from `number | undefined` to `number`.
const nearestLogEventNum = logEventNumsOnPage[nearestIdx as number] as number;

updateWindowUrlHashParams({
logEventNum: nearestLogEventNum,
});

return true;
};

/**
* Provides state management for the application. This provider must be wrapped by
* UrlContextProvider to function correctly.
Expand All @@ -186,15 +227,17 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {
const [fileName, setFileName] = useState<string>(STATE_DEFAULT.fileName);
const [logData, setLogData] = useState<string>(STATE_DEFAULT.logData);
const [numEvents, setNumEvents] = useState<number>(STATE_DEFAULT.numEvents);
const [numPages, setNumPages] = useState<number>(STATE_DEFAULT.numPages);
const [pageNum, setPageNum] = useState<number>(STATE_DEFAULT.pageNum);
const beginLineNumToLogEventNumRef =
useRef<BeginLineNumToLogEventNumMap>(STATE_DEFAULT.beginLineNumToLogEventNum);
const [exportProgress, setExportProgress] =
useState<Nullable<number>>(STATE_DEFAULT.exportProgress);

// Refs
const logEventNumRef = useRef(logEventNum);
const numPagesRef = useRef<number>(STATE_DEFAULT.numPages);
const pageNumRef = useRef<number>(STATE_DEFAULT.pageNum);
const numPagesRef = useRef<number>(numPages);
const pageNumRef = useRef<number>(pageNum);
const logExportManagerRef = useRef<null|LogExportManager>(null);
const mainWorkerRef = useRef<null|Worker>(null);

Expand All @@ -220,7 +263,8 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {
break;
case WORKER_RESP_CODE.PAGE_DATA: {
setLogData(args.logs);
pageNumRef.current = args.pageNum;
setNumPages(args.numPages);
setPageNum(args.pageNum);
beginLineNumToLogEventNumRef.current = args.beginLineNumToLogEventNum;
updateWindowUrlHashParams({
logEventNum: args.logEventNum,
Expand Down Expand Up @@ -253,7 +297,7 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {
workerPostReq(
mainWorkerRef.current,
WORKER_REQ_CODE.EXPORT_LOG,
{decoderOptions: getConfig(CONFIG_KEY.DECODER_OPTIONS)}
null
);
}, [
numEvents,
Expand Down Expand Up @@ -298,20 +342,32 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {
loadPageByCursor(mainWorkerRef.current, cursor);
}, []);

// On `numEvents` update, recalculate `numPagesRef`.
useEffect(() => {
if (STATE_DEFAULT.numEvents === numEvents) {
const setLogLevelFilter = (newLogLevelFilter: LogLevelFilter) => {
if (null === mainWorkerRef.current) {
return;
}

numPagesRef.current = getChunkNum(numEvents, getConfig(CONFIG_KEY.PAGE_SIZE));
}, [numEvents]);
workerPostReq(mainWorkerRef.current, WORKER_REQ_CODE.SET_FILTER, {
cursor: {code: CURSOR_CODE.EVENT_NUM, args: {eventNum: logEventNumRef.current ?? 1}},
logLevelFilter: newLogLevelFilter,
});
};

// Synchronize `logEventNumRef` with `logEventNum`.
useEffect(() => {
logEventNumRef.current = logEventNum;
}, [logEventNum]);

// Synchronize `pageNumRef` with `numPages`.
useEffect(() => {
pageNumRef.current = pageNum;
}, [pageNum]);

// Synchronize `numPagesRef` with `numPages`.
useEffect(() => {
numPagesRef.current = numPages;
}, [numPages]);

// On `logEventNum` update, clamp it then switch page if necessary or simply update the URL.
useEffect(() => {
if (null === mainWorkerRef.current) {
Expand All @@ -327,18 +383,8 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {

const clampedLogEventNum = clamp(logEventNum, 1, numEvents);

// eslint-disable-next-line no-warning-comments
// TODO: After filter is added, will need to find the largest <= log event number on the
// current page. Once found, we update the event number in the URL instead of sending a new
// request since the page has not changed.

if (logEventNumsOnPage.includes(clampedLogEventNum)) {
if (clampedLogEventNum !== logEventNum) {
updateWindowUrlHashParams({
logEventNum: clampedLogEventNum,
});
}

if (updateUrlIfEventOnPage(clampedLogEventNum, logEventNumsOnPage)) {
// No need to request a new page since the log event is on the current page.
return;
}

Expand Down Expand Up @@ -380,12 +426,13 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {
fileName: fileName,
logData: logData,
numEvents: numEvents,
numPages: numPagesRef.current,
pageNum: pageNumRef.current,
numPages: numPages,
pageNum: pageNum,

exportLogs: exportLogs,
loadFile: loadFile,
loadPageByAction: loadPageByAction,
setLogLevelFilter: setLogLevelFilter,
}}
>
{children}
Expand Down
91 changes: 52 additions & 39 deletions new-log-viewer/src/services/LogFileManager/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,16 @@ import {
DecoderOptionsType,
} from "../../typings/decoders";
import {MAX_V8_STRING_LENGTH} from "../../typings/js";
import {LogLevelFilter} from "../../typings/logs";
import {
BeginLineNumToLogEventNumMap,
CURSOR_CODE,
CursorData,
CursorType,
EMPTY_PAGE_RESP,
FileSrcType,
WORKER_RESP_CODE,
WorkerResp,
} from "../../typings/worker";
import {EXPORT_LOGS_CHUNK_SIZE} from "../../utils/config";
import {getChunkNum} from "../../utils/math";
Expand Down Expand Up @@ -128,6 +133,19 @@ class LogFileManager {
this.#decoder.setFormatterOptions(options);
}

/**
* Sets the log level filter.
*
* @param logLevelFilter
* @throws {Error} If the log level filter couldn't be set.
*/
setLogLevelFilter (logLevelFilter: LogLevelFilter) {
const result = this.#decoder.setLogLevelFilter(logLevelFilter);
if (false === result) {
throw new Error("Failed to set log level filter for the decoder.");
}
}
Comment on lines +136 to +147
Copy link

@coderabbitai coderabbitai bot Oct 9, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Include 'logLevelFilter' value in error message for better debugging

To enhance the clarity of the error message when setting the log level filter fails, consider including the value of logLevelFilter. This provides more context and aids in debugging.

Apply this diff to improve the error message:

        if (false === result) {
-            throw new Error("Failed to set log level filter for the decoder.");
+            throw new Error(`Failed to set log level filter '${logLevelFilter}' for the decoder.`);
        }
📝 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
/**
* Sets the log level filter.
*
* @param logLevelFilter
* @throws {Error} If the log level filter couldn't be set.
*/
setLogLevelFilter (logLevelFilter: LogLevelFilter) {
const result = this.#decoder.setLogLevelFilter(logLevelFilter);
if (false === result) {
throw new Error("Failed to set log level filter for the decoder.");
}
}
/**
* Sets the log level filter.
*
* @param logLevelFilter
* @throws {Error} If the log level filter couldn't be set.
*/
setLogLevelFilter (logLevelFilter: LogLevelFilter) {
const result = this.#decoder.setLogLevelFilter(logLevelFilter);
if (false === result) {
throw new Error(`Failed to set log level filter '${logLevelFilter}' for the decoder.`);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

silly rabbit tricks are for kids. The log level filter is not printable. I think its okay as is

Copy link

Choose a reason for hiding this comment

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

@davemarco, understood. Thank you for the clarification.

(_/)
( •︵•)
/   \


✏️ Learnings added
Learnt from: davemarco
PR: y-scope/yscope-log-viewer#89
File: new-log-viewer/src/services/LogFileManager/index.ts:136-147
Timestamp: 2024-10-09T15:38:36.380Z
Learning: The `logLevelFilter` is not printable, so error messages should not include its value.

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.


/**
* Loads log events in the range
* [`beginLogEventIdx`, `beginLogEventIdx + EXPORT_LOGS_CHUNK_SIZE`), or all remaining log
Expand Down Expand Up @@ -168,33 +186,32 @@ class LogFileManager {
* numbers, and the line number of the first line in the cursor identified event.
* @throws {Error} if any error occurs during decode.
*/
loadPage (cursor: CursorType): {
beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap,
cursorLineNum: number
logEventNum: number
logs: string,
pageNum: number
} {
loadPage (cursor: CursorType): WorkerResp<WORKER_RESP_CODE.PAGE_DATA> {
console.debug(`loadPage: cursor=${JSON.stringify(cursor)}`);
const filteredLogEventMap = this.#decoder.getFilteredLogEventMap();
const numActiveEvents: number = filteredLogEventMap ?
filteredLogEventMap.length :
this.#numEvents;

if (0 === numActiveEvents) {
return EMPTY_PAGE_RESP;
}
const {
pageBeginLogEventNum,
pageEndLogEventNum,
matchingLogEventNum,
} = this.#getCursorData(cursor);

pageBegin,
pageEnd,
matchingEvent,
} = this.#getCursorData(cursor, numActiveEvents);
const results = this.#decoder.decodeRange(
pageBeginLogEventNum - 1,
pageEndLogEventNum - 1,
false
pageBegin,
pageEnd,
null !== filteredLogEventMap,
);

if (null === results) {
throw new Error("Error occurred during decoding. " +
`pageBeginLogEventNum=${pageBeginLogEventNum}, ` +
`pageEndLogEventNum=${pageEndLogEventNum}`);
`pageBegin=${pageBegin}, ` +
`pageEnd=${pageEnd}`);
}

const messages: string[] = [];
const beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap = new Map();
let currentLine = 1;
Expand All @@ -210,14 +227,20 @@ class LogFileManager {
beginLineNumToLogEventNum.set(currentLine, logEventNum);
currentLine += msg.split("\n").length - 1;
});

const newPageNum: number = getChunkNum(pageBeginLogEventNum, this.#pageSize);
const newNumPages: number = getChunkNum(numActiveEvents, this.#pageSize);
const newPageNum: number = getChunkNum(pageBegin + 1, this.#pageSize);
const matchingLogEventNum = 1 + (
null !== filteredLogEventMap ?
(filteredLogEventMap[matchingEvent] as number) :
matchingEvent
);

return {
beginLineNumToLogEventNum: beginLineNumToLogEventNum,
cursorLineNum: 1,
logEventNum: matchingLogEventNum,
logs: messages.join(""),
numPages: newNumPages,
pageNum: newPageNum,
};
}
Expand All @@ -226,39 +249,29 @@ class LogFileManager {
* Gets the data that corresponds to the cursor.
*
* @param cursor
* @return Log event numbers for:
* - the range [begin, end) of the page containing the matching log event.
* - the log event number that matches the cursor.
* @param numActiveEvents
* @return Cursor data.
* @throws {Error} if the type of cursor is not supported.
*/
#getCursorData (cursor: CursorType): {
pageBeginLogEventNum: number,
pageEndLogEventNum: number,
matchingLogEventNum: number
} {
#getCursorData (cursor: CursorType, numActiveEvents: number): CursorData {
const {code, args} = cursor;
switch (code) {
case CURSOR_CODE.PAGE_NUM:
return getPageNumCursorData(
args.pageNum,
args.eventPositionOnPage,
this.#numEvents,
this.#pageSize
numActiveEvents,
this.#pageSize,
);

case CURSOR_CODE.LAST_EVENT:
return getLastEventCursorData(
this.#numEvents,
this.#pageSize
);

return getLastEventCursorData(numActiveEvents, this.#pageSize);
case CURSOR_CODE.EVENT_NUM:
return getEventNumCursorData(
args.eventNum,
this.#numEvents,
this.#pageSize
numActiveEvents,
this.#pageSize,
this.#decoder.getFilteredLogEventMap(),
);

default:
throw new Error(`Unsupported cursor type: ${code}`);
}
Expand Down
Loading
Loading