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

Histogram is Back in Zui #2472

Merged
merged 30 commits into from
Sep 1, 2022
Merged

Histogram is Back in Zui #2472

merged 30 commits into from
Sep 1, 2022

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented Aug 3, 2022

The Histogram is Back in Zui

In an effort to ship sooner, I've dropped some of my original requirements. The histogram can be shown and hidden manually for each tab using "App Menu" => "View" => "Show Histogram". The histogram will also be hidden if we can't find a "time" range for the pool. Other than that, we don't try to determine if a pool is a zeek pool or not. If a user want's non-zeek data and are annoyed with the histogram, they can hide it.

Brushing the histogram will never update the Zed text directly, it will only update the time range pin. Updating the Zed is prone to errors as we do not have a robust way to do it. This is a restraint that simplifies the app by making it predictable.

This histogram is just like the old one. It counts by the "ts" time field, and the string "_path" field.

  • Grab the correct span from the query.
  • Conditionally render the histogram for zeek pools only.
  • Determine and store the fact that a pool is a zeek pool.
  • Brushing the histogram updates the zed query or the from pin.
  • Add a range picker to the from pin that we can update from the histogram brushes.
  • Add an option to hide the histogram even for zeek pools

Notes for the future:

We could add tags to a pool that could be used to determine features. We could add a form on the pool page to determine if something is a zeek log.

if (pool.tags.include("zeek"))

Think about configuring the histogram with colors for types and things.

Closes #2489
Closes #2483
Closes #2488

@philrz
Copy link
Contributor

philrz commented Aug 25, 2022

  • FIXED

I did some preliminary testing on this branch at @jameskerr's suggestion. The video below shows what appears to be a bug as of commit 363d9a1 in this branch.

Repro steps:

  1. Import all.zng, which is a ZNG file containing the Zeek records generated from the common test file all.pcap
  2. Click Query Pool
  3. Run a count() to confirm the record count at this default range matches the record count shown by running zq 'count()' all.zng at the shell, which it does
  4. Click the Back button to return to the original histogram/data
  5. Brush a portion of the histogram to narrow down to a subset of the data
  6. Run a count() to confirm a lower record count, which it is
  7. Click the Back button twice to attempt to return to the original histogram/data
    • This is where the issue shows up! Notice that the bars of the histogram look different from when we first saw this default range.
  8. Run a count(), which once again shows the full count of records. This seems to imply that the problem was with the rendering of the histogram and not the data from the query that made up the histogram.
  9. Cick the Back button to attempt to return to the original histogram data... and sure enough, the original view is back!
Repro.mp4

@jameskerr jameskerr requested a review from philrz August 31, 2022 15:21
@philrz
Copy link
Contributor

philrz commented Aug 31, 2022

  • FIXED

Here's a bug I found with imported queries that seems to be specific to this branch. Currently testing at commit f9f4c4d.

Repro steps:

  1. Import data sample.zng from this repo
  2. Import the Brimcap Queries from the Brimcap repo
  3. Attempt to apply a query in the current session
Repro.mp4

The stack dump:

TypeError: Cannot read properties of undefined (reading 'version')
    at ActiveQuery.isLatest (/Users/phil/work/brim/dist/app/core/models/active-query.js:24:85)
    at getType (/Users/phil/work/brim/dist/app/features/right-pane/history/history-item.js:96:16)
    at HistoryItem (/Users/phil/work/brim/dist/app/features/right-pane/history/history-item.js:143:18)
    at renderWithHooks (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:16141:18)
    at mountIndeterminateComponent (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:20838:13)
    at beginWork (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:22342:16)
    at beginWork$1 (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:27219:14)
    at performUnitOfWork (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:26392:12)
    at workLoopSync (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:26303:5)
    at renderRootSync (/Users/phil/work/brim/node_modules/react-dom/cjs/react-dom.development.js:26271:7)

@philrz
Copy link
Contributor

philrz commented Aug 31, 2022

  • Fixed

More testing of this branch at commit f9f4c4d. Here I'm trying to compare the experience with non-Zeek data.

Compare the two videos below. In both cases I'm starting from a completely pristine app that's never been launched before.

In the first, I import sample.zng from this repo and observe the working histogram. Then I import the non-Zeek prs.json from the Zed repo. Expectations were set at the top of this issue that the histogram would be hidden if we can't find a "time" range for the pool, and I expect the "prs.json" pool fits that description. I was therefore a little thrown by the fact that it had a persistent "Loading..." shown in the histogram area. I guess it kind of makes sense since there was previously a histogram there in the same Query Session, but even if so, the "Loading..." is still distracting. If the histogram area can't be automatically hidden at that point, then I'd suggest that maybe the "Loading..." be replaced with something indicating that the histogram area was intentionally not being populated.

I also noticed the uncaught error below in the DevTools console, so that may be related.

Repro1.mp4
Uncaught (in promise) Error: Pool has no span
    at Pool.everythingSpan (/Users/phil/work/brim/dist/app/core/pools/pool.js:65)
    at /Users/phil/work/brim/dist/js/state/Histogram/build-query.js:68
    at async /Users/phil/work/brim/dist/js/state/Histogram/build-query.js:43
    at async /Users/phil/work/brim/dist/js/state/Histogram/run-query.js:25

By comparison, if I make the "prs.json" the first and only data I work with in the pristine app, the histogram area never appears, though the error still shows up in DevTools. But I presume what I'm seeing in the main window is the intended experience?

Repro2.mp4

@philrz
Copy link
Contributor

philrz commented Aug 31, 2022

  • FIXED

More testing of this branch at commit f9f4c4d.

Here I import the data "all.zng" which are the logs generated by importing the common test data "all.pcap". After brushing the histogram I tried to manually edit one part of the time range, which caused the histogram to hang at "Loading..." DevTools showed an error:

Security Warning: Prevented navigation to file:///Users/phil/work/brim/dialog?field=ts&from=2015-03-17T12%3A49%3A53.832Z&to=2015-03-19T01%3A48%3A00.182Z
Repro.mp4

@philrz
Copy link
Contributor

philrz commented Aug 31, 2022

  • FIXED

Also, I wanted to test out how things would go if I manually added a Time Range pin first and then manipulated the histogram. At that point I bumped into known bug #2483 again. Since you have time range pins on the brain, maybe a good time to knock that one out? 😬

Copy link
Contributor

@philrz philrz left a comment

Choose a reason for hiding this comment

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

The fixes to the branch bugs reported in this PR all seem effective, and further banging did not reveal any additional bugs, so this is a functional 👍 from me!

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