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 11 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
90 changes: 68 additions & 22 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 @@ -170,6 +174,25 @@ const loadPageByCursor = (
});
};

/**
* If the log event number changed, update the URL.
*
* @param logEventNum
* @param newLogEventNum
* @return Whether the log event number changed.
*/
const updateUrlIfModified = (logEventNum: number, newLogEventNum: Nullable<number>): boolean => {
if (newLogEventNum !== logEventNum) {
updateWindowUrlHashParams({
logEventNum: newLogEventNum,
});

return true;
}

return false;
};

/**
* Provides state management for the application. This provider must be wrapped by
* UrlContextProvider to function correctly.
Expand All @@ -186,15 +209,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 +245,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 @@ -283,6 +309,16 @@ const StateContextProvider = ({children}: StateContextProviderProps) => {
handleMainWorkerResp,
]);

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

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

Choose a reason for hiding this comment

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

Shall we group these with the other useEffects?


const loadPageByAction = useCallback((navAction: NavigationAction) => {
if (null === mainWorkerRef.current) {
console.error("Unexpected null mainWorkerRef.current");
Expand All @@ -298,14 +334,16 @@ 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}},
kirkrodrigues marked this conversation as resolved.
Show resolved Hide resolved
davemarco marked this conversation as resolved.
Show resolved Hide resolved
logLevelFilter: newLogLevelFilter,
});
};

// Synchronize `logEventNumRef` with `logEventNum`.
useEffect(() => {
Expand All @@ -327,16 +365,23 @@ 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 (isWithinBounds(logEventNumsOnPage, clampedLogEventNum)) {
if (updateUrlIfModified(logEventNum, clampedLogEventNum)) {
return;
}

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

// First explicit cast since typescript thinks `nearestIdx` can be null, but
// it can't as `clampedLogEventNum` must be inside `logEventNumsOnPage`. Second explicit
// cast since typescript thinks `logEventNumsOnPage` can be empty, but it can't as
// isWithinBounds would have returned false.
const nearestLogEventNum = logEventNumsOnPage[nearestIdx as number] as number;
if (updateUrlIfModified(logEventNum, nearestLogEventNum)) {
return;
}

return;
Expand Down Expand Up @@ -380,12 +425,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
79 changes: 55 additions & 24 deletions new-log-viewer/src/services/LogFileManager/index.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import {Nullable} from "../../typings/common";
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unused import 'Nullable'

The import Nullable from "../../typings/common" is not used in this file and can be safely removed to clean up the code.

🧰 Tools
🪛 GitHub Check: lint-check

[warning] 1-1:
Run autofix to sort these imports!


[failure] 1-1:
'Nullable' is defined but never used

import {
Decoder,
DecoderOptionsType,
} from "../../typings/decoders";
import {MAX_V8_STRING_LENGTH} from "../../typings/js";
import {LogLevelFilter} from "../../typings/logs";
import {
BeginLineNumToLogEventNumMap,
CURSOR_CODE,
Expand All @@ -17,6 +19,8 @@ import JsonlDecoder from "../decoders/JsonlDecoder";
import {
getEventNumCursorData,
getLastEventCursorData,
getMatchingLogEventNum,
getNewNumPages,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unused imports 'getMatchingLogEventNum' and 'getNewNumPages'

The functions getMatchingLogEventNum and getNewNumPages are imported but not used in this file. Please remove these unused imports to avoid confusion and potential issues.

🧰 Tools
🪛 GitHub Check: lint-check

[failure] 22-22:
'getMatchingLogEventNum' is defined but never used


[failure] 23-23:
'getNewNumPages' is defined but never used

getPageNumCursorData,
loadFile,
} from "./utils";
Expand Down Expand Up @@ -128,6 +132,19 @@ class LogFileManager {
this.#decoder.setFormatterOptions(options);
}

/**
* Sets the log level filter.
*
* @param logLevelFilter
* @throws {Error} If changing the log level filter couldn't be set.
*/
setLogLevelFilter (logLevelFilter: LogLevelFilter) {
const result: boolean = this.#decoder.setLogLevelFilter(logLevelFilter);
davemarco marked this conversation as resolved.
Show resolved Hide resolved
if (false === result) {
throw new Error("Failed to set log level filter for current decoder.");
}
Copy link

@coderabbitai coderabbitai bot Oct 4, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing error message for clarity.

When throwing an error due to a failure in setting the log level filter, including the attempted logLevelFilter value can provide more context for debugging.

Apply this change to improve the error message:

    if (false === result) {
-       throw new Error("Failed to set log level filter for current decoder.");
+       throw new Error(`Failed to set log level filter ${logLevelFilter} for current 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
if (false === result) {
throw new Error("Failed to set log level filter for current decoder.");
}
if (false === result) {
throw new Error(`Failed to set log level filter ${logLevelFilter} for current decoder.`);
}

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

Copy link

Choose a reason for hiding this comment

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

@junhaoliao, glad to hear that! (^.^)/

}

/**
* Loads log events in the range
* [`beginLogEventIdx`, `beginLogEventIdx + EXPORT_LOGS_CHUNK_SIZE`), or all remaining log
Expand Down Expand Up @@ -174,25 +191,29 @@ class LogFileManager {
logEventNum: number
logs: string,
pageNum: number
numPages: number
} {
console.debug(`loadPage: cursor=${JSON.stringify(cursor)}`);

const {
pageBeginLogEventNum,
pageEndLogEventNum,
matchingLogEventNum,
pageBeginIdx,
davemarco marked this conversation as resolved.
Show resolved Hide resolved
pageEndIdx,
matchingIdx,
} = this.#getCursorData(cursor);

const filteredLogEventMap = this.#decoder.getFilteredLogEventMap();
const useFilter: boolean = null !== filteredLogEventMap;

const results = this.#decoder.decodeRange(
pageBeginLogEventNum - 1,
pageEndLogEventNum - 1,
false
pageBeginIdx,
pageEndIdx,
useFilter
);

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

const messages: string[] = [];
Expand All @@ -211,13 +232,20 @@ class LogFileManager {
currentLine += msg.split("\n").length - 1;
});

const newPageNum: number = getChunkNum(pageBeginLogEventNum, this.#pageSize);
const matchingLogEventNum = getMatchingLogEventNum(matchingIdx, filteredLogEventMap);
const newPageNum: number = getChunkNum(pageBeginIdx + 1, this.#pageSize);
const newNumPages: number = getNewNumPages(
this.#numEvents,
this.#pageSize,
filteredLogEventMap,
);

return {
beginLineNumToLogEventNum: beginLineNumToLogEventNum,
cursorLineNum: 1,
logEventNum: matchingLogEventNum,
logs: messages.join(""),
numPages: newNumPages,
pageNum: newPageNum,
};
}
Expand All @@ -226,39 +254,42 @@ class LogFileManager {
* Gets the data that corresponds to the cursor.
*
* @param cursor
* @return Log event numbers for:
* @return Index for:
davemarco marked this conversation as resolved.
Show resolved Hide resolved
* - the range [begin, end) of the page containing the matching log event.
* - the log event number that matches the cursor.
* @throws {Error} if the type of cursor is not supported.
*/
#getCursorData (cursor: CursorType): {
pageBeginLogEventNum: number,
pageEndLogEventNum: number,
matchingLogEventNum: number
pageBeginIdx: number,
pageEndIdx: number,
matchingIdx: Nullable<number>
} {
const {code, args} = cursor;
const filteredLogEventMap = this.#decoder.getFilteredLogEventMap();
const numEvents: number = filteredLogEventMap ?
filteredLogEventMap.length :
this.#numEvents;
Copy link
Member

Choose a reason for hiding this comment

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

Shall we name this.#numEvents as this.#numTotalEvents?

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'm okay to do this. shall we rename in front-end as well? In front-end, it is always the total.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sounds like a good idea.

Copy link
Contributor Author

@davemarco davemarco Oct 6, 2024

Choose a reason for hiding this comment

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

for now I just changed to activeNumEvents in this function. There are many many instances of numEvents in the code.


if (0 === numEvents) {
return {pageBeginIdx: 0, pageEndIdx: 0, matchingIdx: null};
}
switch (code) {
case CURSOR_CODE.PAGE_NUM:
return getPageNumCursorData(
args.pageNum,
args.eventPositionOnPage,
this.#numEvents,
this.#pageSize
numEvents,
this.#pageSize,
);

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

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

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