UI: add full graph toggle by jlukenoff · Pull Request #2569 · MarquezProject/marquez

jlukenoff · 2023-08-05T16:12:26Z

Problem

There are some reasonable scenarios where users would want to view only the direct successors and predecessors of the current selected node in the lineage graph. This PR enables users to toggle a "Full Graph" view, when deselected, the graph will render only the nodes that are along the selected paths. When deselected and changing selected nodes, we expect that the graph should re-render with any new nodes that weren't along the selected path previously.

Closes: #2558

Solution

Add a showFullGraph property to the lineage state and enables a toggle on the UI.
Add a removeUnselectedNodes method to Lineage that can remove any unselected nodes from the graph.

One-line summary: add a toggle to the Lineage UI to let users switch between viewing the full graph and a view with only the selected paths.

Checklist

You've signed-off your work
Your changes are accompanied by tests (if relevant)
Your change contains a small diff and is self-contained
You've updated any relevant documentation (if relevant)
You've included a one-line summary of your change for the CHANGELOG.md (Depending on the change, this may not be necessary).
You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
You've included a header in any source code files (if relevant)

codecov · 2023-08-05T16:18:40Z

Codecov Report

Merging #2569 (34ae39b) into main (1d46b66) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #2569   +/-   ##
=========================================
  Coverage     83.26%   83.26%           
  Complexity     1286     1286           
=========================================
  Files           243      243           
  Lines          5934     5934           
  Branches        279      279           
=========================================
  Hits           4941     4941           
  Misses          846      846           
  Partials        147      147

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

davidsharp7 · 2023-08-05T19:08:06Z

Yeah looks good - only suggestion I would make is to change the background colour of the toggle to Marquez Green when its on so its a little easier to determine state.

jlukenoff · 2023-08-05T21:37:32Z

Yeah looks good - only suggestion I would make is to change the background colour of the toggle to Marquez Green when its on so its a little easier to determine state.

Good idea, updated it to use the primary green color when enabled.

davidsharp7 · 2023-08-05T21:44:13Z

Yeah looks good - only suggestion I would make is to change the background colour of the toggle to Marquez Green when its on so its a little easier to determine state.

Good idea, updated it to use the primary green color when enabled.

Yeah perfect! Might need to get wider feedback but I wonder if its worth inverting the toggle so that by default its shows everything and activating the toggle shows only the critical path.

jlukenoff · 2023-08-05T22:42:59Z

That’s a good point, that may make more sense since the full graph has always been the default and this is something new users can enable. I’m having trouble thinking of a label for the switch that would make sense that isn’t too wordy though. Perhaps “Simple Graph” or something like that?

Agree it would be great to get some wider feedback.

davidsharp7 · 2023-08-05T23:01:01Z

That’s a good point, that may make more sense since the full graph has always been the default and this is something new users can enable. I’m having trouble thinking of a label for the switch that would make sense that isn’t too wordy though. Perhaps “Simple Graph” or something like that?

Agree it would be great to get some wider feedback.

What about “Show Critical Path”?

davidsharp7 · 2023-08-06T07:20:05Z

Yeah I think we can just reverse the logic and it should work fine

I've added the sections that can be reversed on your branch (could be wrong as this the first time I have ever looked at typescript/react/node)

davidsharp7 · 2023-08-06T07:21:14Z

  bottomBarHeight: (window.innerHeight - HEADER_HEIGHT) / 3,
-  depth: 5
+  depth: 5,
+  showFullGraph: true


showFullGraph: false

davidsharp7 · 2023-08-06T07:22:00Z

            empty_body: 'Try selecting a node through search or the jobs or datasets page.',
-            graph_depth_title: 'Graph Depth'
+            graph_depth_title: 'Graph Depth',
+            full_graph_label: 'Full Graph'


full_graph_label: 'Show Critical Path'

Eh, this just starts to feel like a lot of words to express something relatively simple. I tend to err on the side of "less is more" with these things. I really like the ArgoCD version of this where the text labels show up as a tooltip when you hover over the options. I think that helps preserve the real-estate the configs take up. Maybe we want to think about something like that if we plan to add more configs to this part of the screen.

Yeah I’m in two minds about that. Like the idea of having a pop up on hover to display basic ( maybe even facets like data quality?) info about the node. Maybe we need an additional menu on the header for global type variables like display and depth?

I love the discussion here! My hot take is I would agree with @jlukenoff on the wordiness of the lineage graph options (they'll be more added in the future). We know we are interacting with the lineage graph, so I think we can go one step further to address the wordiness of the options:

Full - toggle between critical path and the full graph overlaid on top of critical path graph (default: false)

Depth - full graph traversal depth

I added shortening "Graph Depth", but that can be a follow up PR.

As a follow up, the lineage API should support the different graph options (Full, Critical) to avoid filtering on the UI. But, in the UI is a great first step!

davidsharp7 · 2023-08-06T07:22:44Z

+
+    // Always rebuild the graph if the selected node changes and we aren't showing the full graph
+    // Since there may be more or less nodes to change
+    if (this.props.selectedNode !== prevProps.selectedNode && !this.props.showFullGraph) {


if (this.props.selectedNode !== prevProps.selectedNode && this.props.showFullGraph) { this.buildGraphAll(this.props.lineage.graph) }

davidsharp7 · 2023-08-06T07:23:19Z

      }
    }
+
+    if (!this.props.showFullGraph) {


if (this.props.showFullGraph) { this.removeUnselectedNodes() }

davidsharp7 · 2023-08-06T09:52:51Z

Promise this is my last comment - there is a slight bug when you select "etl_delivery_7_days" - notice that there are 2 connected nodes even though they are not on the critical path. I think its because technically you can't filter out those nodes because they are part of the critical path at some point. I think a work around is to add something like this

    this.setState({
      graph: g,
      edges: this.props.showFullGraph ? this.getEdges().filter(edge => edge.isSelected === true) : this.getEdges(),
      nodes: g.nodes().map(v => g.node(v))
    })

jlukenoff · 2023-08-06T22:14:38Z

Promise this is my last comment - there is a slight bug when you select "etl_delivery_7_days" - notice that there are 2 connected nodes even though they are not on the critical path. I think its because technically you can't filter out those nodes because they are part of the critical path at some point. I think a work around is to add something like this

I'm not sure I understand, doesn't the fact that they are connected by the selected path mean they are on the critical path? Could you point out which nodes in your screenshot you think shouldn't be displayed?

jlukenoff · 2023-08-06T23:14:08Z

Had to do a pretty big refactor to account for #2563 but the functionality is still the same. Plus it's worth it to inherit the react upgrade 😄

davidsharp7 · 2023-08-06T23:30:42Z

Promise this is my last comment - there is a slight bug when you select "etl_delivery_7_days" - notice that there are 2 connected nodes even though they are not on the critical path. I think its because technically you can't filter out those nodes because they are part of the critical path at some point. I think a work around is to add something like this

I'm not sure I understand, doesn't the fact that they are connected by the selected path mean they are on the critical path? Could you point out which nodes in your screenshot you think shouldn't be displayed?

So if you select etl_delivery_7_days notice how there is a grey line between public.customers and email_discounts. Given the job selected that is not on the critical path and shouldn’t appear. Simplest solution is to also filter out anything where isSelected is False.

Filter out isSelected is False if "Full Graph" is off.

jlukenoff · 2023-08-07T15:35:20Z

So if you select etl_delivery_7_days notice how there is a grey line between public.customers and email_discounts. Given the job selected that is not on the critical path and shouldn’t appear. Simplest solution is to also filter out anything where isSelected is False.

I think I see your point but as a user I would prefer to see edges that exist in this view if the relevant nodes are being displayed. I think not doing so obfuscates potentially useful information from the user without drastically simplifying the experience compared to displaying the relationship between nodes along the critical path even if those relationships aren't part of the critical path.

wslulciuc · 2023-08-07T16:53:55Z

So if you select etl_delivery_7_days notice how there is a grey line between public.customers and email_discounts. Given the job selected that is not on the critical path and shouldn’t appear. Simplest solution is to also filter out anything where isSelected is False.

@davidsharp7 I wanted to add to @jlukenoff response. In the Full graph mode, we'll want to show all dataset and job dependencies, but the connections between nodes in the graph don't need to be expanded by default. That is, they can be grouped and collapsed, allowing the user to discover and explore the graph as needed. For some insight into where I see the interaction of the graph going, see #2014

phixMe · 2023-08-07T18:56:31Z

  beforeEach(() => {
    g = initGraph()
-    buildGraphAll(g, mockGraphWithCycle, (gResult: graphlib.Graph<MqNode>) => {
+    buildGraphAll(g, mockGraphWithCycle, false, selectedNode, (gResult: graphlib.Graph<MqNode>) => {


Can we add a test case here for the node count on the fullGraph option?

davidsharp7 · 2023-08-07T20:09:58Z

So if you select etl_delivery_7_days notice how there is a grey line between public.customers and email_discounts. Given the job selected that is not on the critical path and shouldn’t appear. Simplest solution is to also filter out anything where isSelected is False.

@davidsharp7 I wanted to add to @jlukenoff response. In the Full graph mode, we'll want to show all dataset and job dependencies, but the connections between nodes in the graph don't need to be expanded by default. That is, they can be grouped and collapsed, allowing the user to discover and explore the graph as needed. For some insight into where I see the interaction of the graph going, see #2014

Yeah I don't think its even a binary choice as each of those views is legitimate i.e

I am changing dataset X I just need to see the critical path to understand impacts.
I am changing dataset X and I want to explore my lineage - expand/collapse
I want to see my entire data eco system.

wslulciuc · 2023-08-07T21:52:26Z

@davidsharp7 I think you make good points, and agree a more broad discussion on the graph view can be had, but I would move the discussion to an issue. Mind opening an issue to elaborate on the graph view in the UI and ways to improve the user expereince?

As for this PR, being able to toggle between a "Full" and "Critical Path" view is the main feature added, so I would merge as is with pending feedback from @phixMe. It's a start and we can follow up with improvements!

wslulciuc

I left some minor feedback on labeling the graph options, but otherwise 👍

Signed-off-by: John Lukenoff <jlukenoff@gmail.com>

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

phixMe

Thanks for adding the unit test. Looks great!

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

jlukenoff · 2023-08-08T18:28:46Z

+      for (let i = 0; i < predecessors.length; i++) {
        if (predecessors[i]) {
-          paths.push([predecessors[i] as unknown as string, node])
-          getPredecessors(predecessors[i] as unknown as string)
+          paths.push([(predecessors[i] as unknown) as string, node])
+          getPredecessors((predecessors[i] as unknown) as string)


Not sure how long we've been doing this but it looks like we've been iterating over the characters in the node id 😬 ? Update this to traverse the predecessors array and did the same for getSuccessors

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

jlukenoff · 2023-08-08T18:44:39Z

Just made a few styling tweaks that I found after adjusting to shorter labels for these configs. This should be good to merge once tests pass (I don't have access to merge yet)

* lint Signed-off-by: John Lukenoff <jlukenoff@gmail.com> * refactor Signed-off-by: John Lukenoff <jlukenoff@gmail.com> * fix test Signed-off-by: John Lukenoff <jlukenoff@gmail.com> * add tests Signed-off-by: John Lukenoff <johnlukenoff@asana.com> * convert depthconfig to formcontrol label Signed-off-by: John Lukenoff <johnlukenoff@asana.com> * unused import Signed-off-by: John Lukenoff <johnlukenoff@asana.com> --------- Signed-off-by: John Lukenoff <jlukenoff@gmail.com> Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

boring-cyborg Bot added the web label Aug 5, 2023

jlukenoff force-pushed the web/add-full-graph-toggle branch from de90eae to c858987 Compare August 5, 2023 16:19

davidsharp7 reviewed Aug 6, 2023

View reviewed changes

jlukenoff force-pushed the web/add-full-graph-toggle branch from fcd53a7 to b6d5c2f Compare August 6, 2023 23:12

phixMe reviewed Aug 7, 2023

View reviewed changes

wslulciuc approved these changes Aug 7, 2023

View reviewed changes

jlukenoff and others added 5 commits August 8, 2023 09:01

lint

dcd9c6b

Signed-off-by: John Lukenoff <jlukenoff@gmail.com>

refactor

a0cc294

Signed-off-by: John Lukenoff <jlukenoff@gmail.com>

fix test

d7b3bb3

Signed-off-by: John Lukenoff <jlukenoff@gmail.com>

wip

f1e5869

Signed-off-by: John Lukenoff <jlukenoff@gmail.com>

add tests

98fc0a8

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

jlukenoff force-pushed the web/add-full-graph-toggle branch from a1e8b73 to 98fc0a8 Compare August 8, 2023 16:44

phixMe approved these changes Aug 8, 2023

View reviewed changes

convert depthconfig to formcontrol label

c0ba2d2

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

jlukenoff commented Aug 8, 2023

View reviewed changes

unused import

34ae39b

Signed-off-by: John Lukenoff <johnlukenoff@asana.com>

wslulciuc merged commit 86f957f into MarquezProject:main Aug 8, 2023

jlukenoff deleted the web/add-full-graph-toggle branch August 8, 2023 19:42

Conversation

jlukenoff commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Checklist

Uh oh!

codecov Bot commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

davidsharp7 commented Aug 5, 2023

Uh oh!

jlukenoff commented Aug 5, 2023

Uh oh!

davidsharp7 commented Aug 5, 2023

Uh oh!

jlukenoff commented Aug 5, 2023

Uh oh!

davidsharp7 commented Aug 5, 2023

Uh oh!

davidsharp7 commented Aug 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlukenoff Aug 6, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

wslulciuc Aug 7, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsharp7 commented Aug 6, 2023

Uh oh!

jlukenoff commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlukenoff commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidsharp7 commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlukenoff commented Aug 7, 2023

Uh oh!

wslulciuc commented Aug 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsharp7 commented Aug 7, 2023

Uh oh!

wslulciuc commented Aug 7, 2023

Uh oh!

wslulciuc left a comment

Choose a reason for hiding this comment

Uh oh!

phixMe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlukenoff commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jlukenoff commented Aug 5, 2023 •

edited

Loading

codecov Bot commented Aug 5, 2023 •

edited

Loading

jlukenoff Aug 6, 2023 •

edited

Loading

wslulciuc Aug 7, 2023 •

edited

Loading

jlukenoff commented Aug 6, 2023 •

edited

Loading

jlukenoff commented Aug 6, 2023 •

edited

Loading

davidsharp7 commented Aug 6, 2023 •

edited

Loading