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: Implement log-level filtering. #63

Closed
wants to merge 88 commits into from

Conversation

davemarco
Copy link
Contributor

@davemarco davemarco commented Aug 29, 2024

References

new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62

Description

  • Implemented buildIdx() to do deserialization in JSONL decoder. Previously, deserialization was done inside constructor. In addition, now we are parsing log level inside in buildIdx() and not in decode().
  • Implement log level filtering with JSON logs.
  • TODO: CLP IRv1 log-level filtering is to be added once the equivalent is implemented in clp-ffi-js. The implementation in clp-ffi-js will start once we agree with the decoder handling in this PR.

Validation performed

  1. Referred to Add support for loading files, decoding them as JSONL, and formatting them using a Logback-like format string. #46 , launched debug server.
  2. Referred to new-log-viewer: Integrate Monaco Editor for enhanced log viewing. #54, tested page switching with Monaco editor with filters off.
  3. Referred to new-log-viewer: Integrate Monaco Editor for enhanced log viewing. #54, tested page switching with single (TRACE) log level filter.
  4. Referred to new-log-viewer: Integrate Monaco Editor for enhanced log viewing. #54, tested page switching with multiple (TRACE, DEBUG) log level filter.

@davemarco davemarco requested a review from junhaoliao August 29, 2024 19:20
@davemarco davemarco marked this pull request as draft August 29, 2024 20:04
@davemarco
Copy link
Contributor Author

Need to handle case where decoder options can change

@davemarco
Copy link
Contributor Author

davemarco commented Aug 30, 2024

Despite the fact that not necessary to update PR since the the page is always refreshed (i.e. logs are rebuilt) when decoder option change. I added a guard to reparse logs when decoder options change in case option design changes in future.

@davemarco davemarco marked this pull request as ready for review August 30, 2024 15:20
Copy link
Member

@junhaoliao junhaoliao left a comment

Choose a reason for hiding this comment

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

Great job! I took a brief look and the code looks mostly good to me. Let's clean up the code with ESLint and we can do a more thorough round of review.

new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/LogFileManager.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/decoders/JsonlDecoder.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/decoders/JsonlDecoder.ts Outdated Show resolved Hide resolved
new-log-viewer/src/services/decoders/JsonlDecoder.ts Outdated Show resolved Hide resolved
@junhaoliao junhaoliao changed the title new-log-viewer: Implement buildIdx() interface method in JSONL decoder. new-log-viewer: Implement log-level filtering. Sep 3, 2024
Comment on lines +144 to +146
newLogEventNum = logEventNumOnPage.at(-1) as number;

return newLogEventNum;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
newLogEventNum = logEventNumOnPage.at(-1) as number;
return newLogEventNum;
return logEventNumOnPage.at(-1) as number;

// that actually exists on the page.
const getClosestLogEventNum = useCallback(
(beginLineNumToLogEventNum: BeginLineNumToLogEventNumMap) => {
let newLogEventNum: Nullable<number> = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let newLogEventNum: Nullable<number> = null;

Comment on lines +149 to +163
// Find first logEventNum smaller or equal to user provided logEventNum.
for (let i = logEventNumOnPage.length; 0 < i; i--) {
if ((logEventNumOnPage[i] as number) <= logEventNumRef.current) {
newLogEventNum = logEventNumOnPage[i] as number;
break;
}
}

if (!newLogEventNum) {
// If no nums on page are smaller than user logEventNum, user logEventNum is the
// smallest so we should just return the smallest value on the page.
newLogEventNum = logEventNumOnPage[0] as number;
}

return newLogEventNum;
Copy link
Member

@junhaoliao junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

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

Suggested change
// Find first logEventNum smaller or equal to user provided logEventNum.
for (let i = logEventNumOnPage.length; 0 < i; i--) {
if ((logEventNumOnPage[i] as number) <= logEventNumRef.current) {
newLogEventNum = logEventNumOnPage[i] as number;
break;
}
}
if (!newLogEventNum) {
// If no nums on page are smaller than user logEventNum, user logEventNum is the
// smallest so we should just return the smallest value on the page.
newLogEventNum = logEventNumOnPage[0] as number;
}
return newLogEventNum;
// Find first logEventNum smaller or equal to user provided logEventNum.
const closestLogEventNum = logEventNumOnPage.findLast((logEventNum) => logEventNum <= logEventNumRef.current);
// Return the closest found or fallback to the smallest value on the page.
return closestLogEventNum ?? logEventNumOnPage[0] as number;

Comment on lines +191 to +193
updateWindowUrlHashParams({
logEventNum: newLogEventNum,
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
updateWindowUrlHashParams({
logEventNum: newLogEventNum,
});
updateWindowUrlHashParams({logEventNum: newLogEventNum});

Comment on lines +344 to +345
firstLogEventNumOnPage: firstLogEventNumOnPage.current,
lastLogEventNumOnPage: lastLogEventNumOnPage.current,
Copy link
Member

Choose a reason for hiding this comment

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

Just so we don't forget this discussion: https://github.com/y-scope/yscope-log-viewer/pull/63/files/7bd410b3514b70e3ff86d0cde2cec5e7fa0f8aea#r1768810124

I'm fine if we want to init the refs with literals. e.g.,

    const firstLogEventNumOnPage = useRef<number[]>([]);
    const lastLogEventNumOnPage = useRef<number[]>([]);

Comment on lines +211 to +213
}, [
getClosestLogEventNum,
]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, [
getClosestLogEventNum,
]);
}, [getClosestLogEventNum]);

Comment on lines +184 to +186
const newPageNum = 1 + firstLogEventNumOnPage.current.findLastIndex(
(value: number) => value <= newLogEventNum,
);
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 check the return value of findLastIndex() against -1 ?

Comment on lines +310 to +312
if (0 === numFilteredEvents) {
setPageNum(0);
}
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 use the constants from STATE_DEFAULT?

return null;
setLogLevelFilter (logLevelFilter: LogLevelFilter): boolean {
this.#filterLogs(logLevelFilter);
this.#isFiltered = Boolean(logLevelFilter);
Copy link
Member

@junhaoliao junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

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

Following the discussion at https://github.com/y-scope/yscope-log-viewer/pull/63/files#r1768464251 , I feel this can be more readable:

Suggested change
this.#isFiltered = Boolean(logLevelFilter);
this.#isFiltered = (null !== logLevelFilter);

@kirkrodrigues
Copy link
Member

Thanks for the additional comments, @junhaoliao. Since this PR ended up being quite large and we are reworking the design, @davemarco is going to submit smaller PRs rather than trying to refactor this one completely.

@junhaoliao
Copy link
Member

@kirkrodrigues @davemarco i think we can close this now, right?

@junhaoliao
Copy link
Member

Closing this since it has been implemented in the other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants