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 18 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
95 changes: 70 additions & 25 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,41 @@ const loadPageByCursor = (
});
};

/**
* If the new log event number is within the page boundaries, update the url with the nearest log
* event and return true. If the new log event number is outside page boundaries, do not update and
* return false.
davemarco marked this conversation as resolved.
Show resolved Hide resolved
*
* @param logEventNum
* @param logEventNumsOnPage
* @return Whether the log event number is within page boundaries.
davemarco marked this conversation as resolved.
Show resolved Hide resolved
*/
const updateUrlIfEventOnPage = (
logEventNum: number,
logEventNumsOnPage: number[]
): boolean => {
if (!isWithinBounds(logEventNumsOnPage, logEventNum)) {
davemarco marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

const nearestIdx = findNearestLessThanOrEqualElement(
logEventNumsOnPage,
logEventNum
);

// First explicit cast since typescript thinks `nearestIdx` can be null, but
// it can't as `logEventNum` must be a value in `logEventNumsOnPage` array.
// 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;

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 +225,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 +261,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 +325,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 +350,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,18 +381,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)) {
// Do not request a new page, if the log event can be found on the current page.
davemarco marked this conversation as resolved.
Show resolved Hide resolved
return;
}

Expand Down Expand Up @@ -380,12 +424,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
85 changes: 59 additions & 26 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";

Check warning on line 1 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

Run autofix to sort these imports!

Check failure on line 1 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

'Nullable' is defined but never used
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,9 +19,12 @@
import {
getEventNumCursorData,
getLastEventCursorData,
getMatchingLogEventNum,

Check failure on line 22 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

'getMatchingLogEventNum' is defined but never used
getNewNumPages,

Check failure on line 23 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

'getNewNumPages' is defined but never used
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,
getEmptyPage,
loadFile,
} from "./utils";

Check failure on line 27 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

Parse errors in imported module './utils': ',' expected. (20:40)


/**
Expand Down Expand Up @@ -128,6 +133,19 @@
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: 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 @@ -168,31 +186,42 @@
* 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): {

Check failure on line 189 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

Method 'loadPage' has too many lines (61). Maximum allowed is 60
beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap,
cursorLineNum: number
logEventNum: number
logs: string,
pageNum: number
numPages: number
} {
console.debug(`loadPage: cursor=${JSON.stringify(cursor)}`);

const filteredLogEventMap = this.#decoder.getFilteredLogEventMap();

const numEvents: number = filteredLogEventMap ?
filteredLogEventMap.length :
this.#numEvents;

if (0 === numEvents) {
return getEmptyPage();
}

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

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

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 +240,20 @@
currentLine += msg.split("\n").length - 1;
});

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

return {
beginLineNumToLogEventNum: beginLineNumToLogEventNum,
cursorLineNum: 1,
logEventNum: matchingLogEventNum,
logs: messages.join(""),
numPages: newNumPages,
pageNum: newPageNum,
};
}
Expand All @@ -226,43 +262,40 @@
* Gets the data that corresponds to the cursor.
*
* @param cursor
* @return Log event numbers for:
* @param numEvents
* @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
#getCursorData (cursor: CursorType, numEvents: number): {
davemarco marked this conversation as resolved.
Show resolved Hide resolved
pageBeginIdx: number,
pageEndIdx: number,
matchingIdx: number,
} {
const {code, args} = cursor;

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,
this.#decoder.getFilteredLogEventMap(),
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent implementation of getFilteredLogEventMap across decoders may lead to paginated view issues.

  • ClpIrDecoder.ts: getFilteredLogEventMap returns null and is marked as not implemented.
🔗 Analysis chain

Verify usage of getFilteredLogEventMap in event number cursor handling.

Ensure that the filtered log event map is accurately utilized when calculating indices for event number cursors to prevent inconsistencies in paginated views.

Run the following script to check references to getFilteredLogEventMap:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `getFilteredLogEventMap` is correctly used in cursor data calculations.

# Test: Search for usages of `getFilteredLogEventMap` in context with cursor handling methods.
rg --type ts 'getFilteredLogEventMap' -A 10 -B 5

Length of output: 6383

);

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

export default LogFileManager;

Check failure on line 301 in new-log-viewer/src/services/LogFileManager/index.ts

View workflow job for this annotation

GitHub Actions / lint-check

File has too many lines (301). Maximum allowed is 300
Loading
Loading