y-scope / yscope-log-viewer

A tool that can be used to view logs compressed in CLP's IR Stream format.
Apache License 2.0
11 stars 12 forks source link

new-log-viewer: Implement log-level filtering. #63

Closed davemarco closed 2 weeks ago

davemarco commented 2 months ago

References

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

Description

Validation performed

  1. Referred to #46 , launched debug server.
  2. Referred to #54, tested page switching with Monaco editor with filters off.
  3. Referred to #54, tested page switching with single (TRACE) log level filter.
  4. Referred to #54, tested page switching with multiple (TRACE, DEBUG) log level filter.
davemarco commented 2 months ago

Need to handle case where decoder options can change

davemarco commented 2 months ago

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 commented 2 months ago

@davemarco I haven't been able to take a careful look into the code yet, but it seems there's one issue whenever I was switching the log level filter from ["TRACE"] to ["DEBUG"] - there are exactly 200 [Renderer -> MainWorker] code=loadPage requests sent to the service worker, and occasionally the logEventNum does not update on editor cursor updates. Could you take a look before we proceed to the review?

Here's an example file I used - example.zip

pageSize was set to 10 and I started from http://localhost:3010/?filePath=http://localhost:3010/test/example.jsonl#logEventNum=10

davemarco commented 2 months ago

I believe there is something buggy with the log event num changing retriggering requests. I am looking into

davemarco commented 2 months ago

I committed changes which appear to fix the issue. Changing the log filter was actually sending out 2 load page requests and they were fighting for control over the logEventNum, which then sent out more requests. There was also a synchonization issue with some of firstLogEventNumPerPage: number[], lastLogEventNumPerPage: number[], states. I believe it works for now.

davemarco commented 1 month ago

There are some issues with page num now that merged. I am looking into solution.

davemarco commented 1 month ago

Added temporary select element for testing.

davemarco commented 1 month ago

Clean up select element and make more user friendly.

davemarco commented 1 month ago

FIxed bugs when changing page. Previous implementation did not consider filtered events.

davemarco commented 1 month ago

I changed to the new interface. I also went through and started fixing a bunch of small bugs. There are a decent amount of changes, but some are just the linter.

davemarco commented 1 month ago

Another thing I want to do, but not in code is remove the filepath useEffect, and do something like this

  const [prevFilePath, setprevFilePath] = useState(filePath);
  if (filePath !== prevFilePath) {
    setprevFilePath(filePath);
    loadFile(filePath, cursor);
  }

The current implementation is taking advantage of useRef on logEventNum, to avoid having logEventNum as a dependency. If it were a dependency, it would reload file of every logEventNum change. I think the code above is better as it dosent need useRef on logEventNum, and gets us one step closer to removing it from code and just using state

May also be possible to keep useEffect, but also have prevFilePath state, and just exit the effect if states are the same.

kirkrodrigues commented 1 month ago

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 commented 3 weeks ago

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

junhaoliao commented 2 weeks ago

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