new-log-viewer: Implement log-level filtering. by davemarco · Pull Request #63 · y-scope/yscope-log-viewer

davemarco · 2024-08-29T19:20:50Z

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

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

davemarco · 2024-08-29T20:05:25Z

Need to handle case where decoder options can change

davemarco · 2024-08-30T15:19:27Z

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.

junhaoliao

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.

Co-authored-by: Junhao Liao <junhao@junhao.ca>

# Conflicts: # new-log-viewer/src/services/LogFileManager.ts # new-log-viewer/src/typings/decoders.ts

…OPTIONS.logLevelFilter=null in CONFIG_DEFAULT.

… switching in the renderer.

junhaoliao · 2024-09-24T06:31:48Z

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


Suggested change

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

return newLogEventNum;

return logEventNumOnPage.at(-1) as number;

junhaoliao · 2024-09-24T06:32:01Z

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


Suggested change

let newLogEventNum: Nullable<number> = null;

junhaoliao · 2024-09-24T06:33:23Z

+            // 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;


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;

junhaoliao · 2024-09-24T06:34:05Z

+                updateWindowUrlHashParams({
+                    logEventNum: newLogEventNum,
+                });


Suggested change

updateWindowUrlHashParams({

logEventNum: newLogEventNum,

});

updateWindowUrlHashParams({logEventNum: newLogEventNum});

junhaoliao · 2024-09-24T06:42:01Z

+                firstLogEventNumOnPage: firstLogEventNumOnPage.current,
+                lastLogEventNumOnPage: lastLogEventNumOnPage.current,


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[]>([]);

junhaoliao · 2024-09-24T06:43:32Z

+    }, [
+        getClosestLogEventNum,
+    ]);


Suggested change

}, [

getClosestLogEventNum,

]);

}, [getClosestLogEventNum]);

junhaoliao · 2024-09-24T06:47:23Z

+                const newPageNum = 1 + firstLogEventNumOnPage.current.findLastIndex(
+                    (value: number) => value <= newLogEventNum,
+                );


Shall we check the return value of findLastIndex() against -1 ?

junhaoliao · 2024-09-24T06:50:15Z

+        if (0 === numFilteredEvents) {
+            setPageNum(0);
+        }


Shall we use the constants from STATE_DEFAULT?

junhaoliao · 2024-09-24T07:12:56Z

-            return null;
+    setLogLevelFilter (logLevelFilter: LogLevelFilter): boolean {
+        this.#filterLogs(logLevelFilter);
+        this.#isFiltered = Boolean(logLevelFilter);


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 · 2024-09-24T07:13:51Z

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 · 2024-10-11T08:24:50Z

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

junhaoliao · 2024-10-18T01:10:46Z

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

davemarco added 3 commits August 29, 2024 18:43

move deserialization and loglevel parsing into buildIdx

c6c7c0c

small changes

1390047

small nits

e821c96

davemarco requested a review from junhaoliao August 29, 2024 19:20

davemarco marked this pull request as draft August 29, 2024 20:04

guard to reparse log events

b8bf240

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

removed console log error that might come up in normal behavior

0427054

junhaoliao requested changes Aug 31, 2024

View reviewed changes

davemarco and others added 2 commits August 30, 2024 22:20

Apply suggestions from code review

a9d1926

Co-authored-by: Junhao Liao <junhao@junhao.ca>

code review

4265e86

davemarco requested a review from junhaoliao August 31, 2024 02:50

junhaoliao mentioned this pull request Sep 1, 2024

new-log-viewer: Add ClpIrDecoder. #62

Merged

junhaoliao reviewed Sep 2, 2024

View reviewed changes

Comment thread new-log-viewer/package.json Outdated

junhaoliao added 12 commits September 2, 2024 13:01

Merge branch 'main' into d-dev2

f6aabce

# Conflicts: # new-log-viewer/src/services/LogFileManager.ts # new-log-viewer/src/typings/decoders.ts

Revert changes in package.json & package-lock.json.

6d593ae

Refactor jsonl log event parsing and handling.

50fc54d

Support setting log level filter via decoder options.

ccee8b5

update docs for JsonlDecoder.setDecoderOptions().

a3eb8b8

Avoid making decoderOptions partial in the ConfigMap and add DECODER_…

82fc233

…OPTIONS.logLevelFilter=null in CONFIG_DEFAULT.

Add log level filtering to the UI and decoders.

61578a6

Separate the concern of numEvents & numFilteredEvents to correct page…

1abdeb8

… switching in the renderer.

lint

a03d6ea

Add pagination boundaries to log events.

dae8488

Revert changes in package-lock.json.

e2b1784

lint.

b9d6e76

junhaoliao changed the title ~~new-log-viewer: Implement buildIdx() interface method in JSONL decoder.~~ new-log-viewer: Implement log-level filtering. Sep 3, 2024

davemarco commented Sep 3, 2024

View reviewed changes

Comment thread new-log-viewer/src/components/Layout.tsx Outdated

junhaoliao reviewed Sep 24, 2024

View reviewed changes

davemarco mentioned this pull request Sep 25, 2024

new-log-viewer: Add event cursor, refactor state context, and redesign load page methods to support discontinuous log event numbers in preparation for log level filtering. #76

Merged

This was referenced Sep 26, 2024

new-log-viewer: Integrate latest clp-ffi-js with support for log-level filtering; Add log-level selector to StatusBar. #77

Merged

new-log-viewer: Replace fallback font with Inter. #78

Merged

davemarco mentioned this pull request Oct 4, 2024

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

Merged

Henry8192 mentioned this pull request Oct 7, 2024

new-log-viewer: Add log query support in StateContextProvider. #80

Merged

junhaoliao closed this Oct 18, 2024

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

		return newLogEventNum;

		firstLogEventNumOnPage: firstLogEventNumOnPage.current,
		lastLogEventNumOnPage: lastLogEventNumOnPage.current,

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

Conversation

davemarco commented Aug 29, 2024 • edited by junhaoliao Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Description

Validation performed

Uh oh!

davemarco commented Aug 29, 2024

Uh oh!

davemarco commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junhaoliao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

junhaoliao Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkrodrigues commented Sep 24, 2024

Uh oh!

junhaoliao commented Oct 11, 2024

Uh oh!

junhaoliao commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davemarco commented Aug 29, 2024 •

edited by junhaoliao

Loading

davemarco commented Aug 30, 2024 •

edited

Loading

junhaoliao Sep 24, 2024 •

edited

Loading

junhaoliao Sep 24, 2024 •

edited

Loading