-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Add event-flow visualization #3102
Conversation
The image should be square, ideally 1024*1024 to render well |
This is awesome btw! I'll try to review soon but I'm a little swamped at the moment. |
TS, | ||
EVENT_NAME, | ||
ENTITY_ID, | ||
} from '@data-ui/event-flow'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does '@%' do? where does the event-flow code live?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just a name-spaced package on npm, under the "@data-ui" user but mainly so all the @data-ui
packages are in the same repo.
it will live at https://github.com/williaster/data-ui/packages/event-flow but I haven't quite merged the chart into the master @data-ui repo, adding one more feature. this is the (massive 🙈) PR
The package is published now so the build should be fixed (you can demo it here). For visual clarity + performance, with larger number of events expected with use in Superset (10,000s) I also added functionality to "trim" nodes in the visualization based on a minimum event count threshold. This means that sequences are aggregated together but "leaf" nodes are hidden from view. |
a5f82ba
to
7d24928
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay on the review, but this is really amazing and the kind of visualization that is a key "selling" point for Superset. I have a few notes for otherwise LGTM!
I'm curious about your CSS in your module and want to confirm that it's namespace alright so it doesn't spill on other visualization (the same way treemap did on yours).
@@ -1273,5 +1273,11 @@ export const controls = { | |||
hidden: true, | |||
description: 'The number of seconds before expiring the cache', | |||
}, | |||
|
|||
generic_checkbox: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the name that will be use in the saved slice's json. Having something generic here will make it harder if/when trying to generate slice dynamically, and harder to read the json and understand what that maps to. Also references in the backend/frontend code won't be as explicit. Let's use a name that describe the actual feature this triggers.
superset/assets/package.json
Outdated
@@ -56,7 +57,7 @@ | |||
"jquery": "^3.2.1", | |||
"jsdom": "9.12.0", | |||
"lodash.throttle": "^4.1.1", | |||
"mapbox-gl": "^0.26.0", | |||
"mapbox-gl": "^0.27.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this was an attempt at fixing the npm install
bug. It's been fixed on master by using yarn
instead of npm install
so this is probably not needed anymore. Though if you've tested that the mapbox viz renders alright I'm not against the bump here even though it would belong in another PR ideally.
const hasData = json.data && json.data.length > 0; | ||
|
||
// the slice container overflows ~80px in explorer, so we have to correct for this | ||
const isExplorer = (/explore/).test(window.location.pathname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if I understand why, but I'm wondering if using slice.width()
and slice.height()
would address the 80px issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this to begin with but it doesn't seem to work/couldn't come up with any fix for the css. The panel dom looks like:
<div class="panel">
<div class="panel-heading">...</div>
<div class="panel-body">...</div>
</div>
panel-body
is what the slice is in and it seems to have the same height as the parent panel
. that means it overflows by the height of panel-heading
:/
fcab639
to
abc09ee
Compare
@mistercrunch I made the following changes
|
I fixed the coveralls token in master, please rebase+push |
…ments, add min event count form.
e72aebc
to
4aef4eb
Compare
3 similar comments
@mistercrunch done |
let me know if I should add a test for the coverage. |
Can you add a demo into Superset distribution? |
@vnnw yes this would be a good addition to get people up and going a little more easily. |
@mistercrunch @graceguo-supercat
This PR adds an event flow visualization for analysis of event sequences. The vis is mostly built in
@data-ui
with @vx and there is a light wrapper for Superset integration.Overall the vis supports the following:
note that builds, etc. will fail until the
@data-ui/event-flow
package is published (which I should get to later today), I just wanted to get some 👀 s on the PR