U/kkasp/tron 2370 do ya like dags #1032

KaspariK · 2025-03-03T22:24:11Z

I know this looks LLM generated but I swear it isn't. I just did a lot of things

What

At its core, this change is about replacing d3-force with a more reasonable DAG visualization. I don't think it was ever a great pick, but we probably didn't have anything as complicated as our largest jobs today so we likely boiled the frog a bit.

Main changes

Upgrading D3 (used for our JobRun timeline) from v3 to v7
Update timeline.coffee to work with D3v7
Remove D3 (d3-force, specifically) from graph.coffee (TRON-2370)
Replace d3-force with a more appropriate hierarchical visualization using dagre for the layout algorithm and cytoscape for the rendering (Issue Force tronweb action graph to be (more) visually topologically sorted #616)
- Since this isn't DOM searchable I added a search field
- Since you can get kind of lost on the DAG canvas I added a reset button to reset the DAG visualization + a cap on how much you can zoom in or out.

Why

This was bad and I wanted to make it less bad. This was called out by a number of teams in the orchestration tool review as a high priority pain point. Replacing d3-force was not that small of a task though since there isn't an immediate alternative available in D3.

Documentation

Commented the shit out of our frontend (the more relevant files like routes and graph) to hopefully make any future work less painful for people
Added ticket for getting state of crossjob actions

Bug fixes and miscellanea

Fixed bug where the timeline showed bizarre times when showing 0 JobRuns
Fixed bug where the timeline wasn't showing any times when showing 1 JobRun
Fixed bug where dates on the timeline would get crowded and overlap each other
Cleaned up console logs. Removed our own useless logs, fixed dev logs from LESS, fixed D3 logs, etc. The only ones I didn't get to were momentjs (TRON-2384) and some Permissions Policy header stuff.
Fixed bug with popup (where we display command and state for actions) being cut off by the graph modal
Updated our LESS mixins to reflect current Job, JobRun, and Action states
Added ticks along the y-axis to better track JobRuns when looking at more than a handful of JobRuns (TRON-2345)
Fixed bug where the config page only took up ~75% of the horizontal space of the page
Made the non-fullscreen DAG visualization wider. This can be beneficial to larger DAGs and I don't think the "Details" panel really needs to be that wide. The catch is that this can look kinda funny for Jobs that have a single action. I don't feel strongly about this one way or the other so if someone things it looks silly I'm happy to switch back.

Validation

I never got around to updating our Tronweb tests. Given the state of those tests I think calling it an "update" would be a stretch. I created TRON-2385 for a future world where we write actual Tronweb tests. In the meantime I have resorted to writing flawless code that I've verified with some manual testing.

GIFs

Behaviour	Old Tronweb	New Tronweb
Timeline with 0 or 1 JobRun	Before	After
Main DAG (repositioning, popup, reset, etc.)	Before	After
Fullscreen DAG (repositioning, popup, reset, etc.)	Before	After
JobRun DAG (with states in popup)	Before	After

Browsers

I've tried this on Arc, Chrome, Safari, and Firefox. No weirdness detected.

See it live!

I've left it running on devbox:8090/web.

If it isn't up I can restart it, or share my complex jobs if any reviewers want to run it themselves.

…dpates

… importance of ordering

… less than a few minutes of data. Add horizontal guidelines for better legibility. Add code comments

…essages in the console

…ual graph height adjustment. Update popup template to be a bit more generic for use between Job and JobRun

…r a new age

…m. Update styles for better popups and JobRun state visualization. Add search and reset buttons to the graph. Update fit logic to suit the modal size automatically. Many other fixes

…he main page graph behind it

KaspariK · 2025-03-05T22:11:57Z

While writing this I used a lot of in-line styles, which generally isn't that great. I'll see how painful it is to move these over, but in the meantime I think this is quite reviewable.

KaspariK · 2025-03-05T22:16:09Z

tronweb/coffee/graph.coffee

+        },
+        # Error states (.error mixin in LESS)
+        {
+            selector: '.failed, .unknown',


We highlight "unknown" action states as red now, but our graph is quite dumb so it isn't highlighting the node. I'm a bit worried about doing this in something like an ad pipeline job where crossjob triggers will show as red nodes. Maybe this should be some alternative colour

unknown as orange might be interesting?

KaspariK · 2025-03-07T00:06:33Z

tronweb/coffee/graph.coffee

+        {
+            selector: 'edge',
+            style: {
+                'curve-style': 'bezier'


Hey wait a minute, why aren't you working?

Context, I was expecting this to curve lines around nodes a bit based on cytoscape's demo, but I think I've misunderstood things here.

At any rate, it's probably more advisable to go with taxi edges which looks like this, or something like an unbundled bezier with dynamic curves which would look this.

EvanKrall

I'm not super familiar with Tron's frontend code so I'm maybe not qualified to offer an opinion on the code quality, but the functionality is way more useful than status quo, and it doesn't look like you're making the code any worse than it already is.

nemacysts

after going through the whole diff, i think some of the magic ui numbers already existed in the previous iteration of this code - so don't worry about my comment about documenting those too much :p

nemacysts · 2025-03-06T18:33:47Z

tronweb/coffee/actionrun.coffee

 window.modules = window.modules || {}
-module = window.modules.actionrun = {}
+window.modules.actionrun = module = {}


i assume this was just for consistency?

Yeah, not sure why we did it both ways. I just wanted to be consistent.

nemacysts · 2025-03-06T20:58:41Z

tronweb/coffee/config.coffee

    initialize: (options) =>
        @listenTo(@model, "sync", @render)

    tagName: "div"

-    className: "span8"
+    className: "span12"


heh, we should probably use real classnames - but this is no worse than the existing setup so i'll pretend i didn't see this :p

If only. Those are predefined by Bootstrap 2.3.2 for the grid system (see https://getbootstrap.com/2.3.2/scaffolding.html). SpanX with a total of 12 "columns" across the page. I just noticed we only spanned 8 and had 1/3 of the screen sitting unused.

I don't think we can actually change these. I think if we get to Bootstrap 3, 4, or 5 (maybe a bit ambitious 😛) we can replace these with the newer names.

nemacysts · 2025-03-06T21:00:33Z

tronweb/coffee/graph.coffee

+        },
+        # Error states (.error mixin in LESS)
+        {
+            selector: '.failed, .unknown',


unknown as orange might be interesting?

nemacysts · 2025-03-06T21:04:44Z

tronweb/coffee/graph.coffee

+                if nodeClass
+                    node.addClass(nodeClass)
+
+            # 50 ms is enough time for the graph to render before we resize and fit it. Is there a better way? Yeah, probably.


there's probably a browser event we can hook into/listen for - but this is probably fine for now

nemacysts · 2025-03-06T21:05:31Z

tronweb/coffee/graph.coffee

-        @force.start()
-        @buildSvgLinks(links)
-        @buildSvgNodes(@model)
+        @$el.html('')


i assume this is to clear any potential existing stuff on the page?

nemacysts · 2025-03-06T21:07:11Z

tronweb/coffee/graph.coffee

+                # Create container for controls and add search and reset fields
+                controlsHtml = '<div class="graph-controls" style="display: inline-flex; align-items: center; margin-right: 45px;">'
+                controlsHtml += '<span class="graph-search" style="display: inline-flex; align-items: center;"><input type="text" placeholder="Search nodes..." style="width:150px; padding:3px; margin: 0; height: 24px; font-weight: normal; vertical-align: middle;"></span>'
+                controlsHtml += '<button class="reset-graph-btn btn btn-clear tt-enable" title="Reset Graph" data-placement="top" style="margin-left: 10px;"><i class="icon-refresh icon-white"></i></button>'


i assume there's probably a better way to do this too - but imo, it's probably worth shipping this out and then improving this later (or maybe even potentially re-writing all this in our internal ui repo :p)

nemacysts · 2025-03-06T21:09:04Z

tronweb/coffee/graph.coffee

ty for all the comments!

i think one suggestion might be to extract some of the constants out (or add even more comments :p) for a couple things like tooltip adjustment values and whatnot)

nemacysts · 2025-03-06T21:10:02Z

tronweb/coffee/job.coffee

@@ -192,7 +197,7 @@ class window.JobView extends Backbone.View
                    <span id="refresh"></span>
                </h1>
            </div>
-            <div class="span5 outline-block">
+            <div class="span4 outline-block">


it'd be nice to actually name these - but i assume that's likely an even bigger change

As above, so below.

…wn as a new colour on the graph

KaspariK added 17 commits February 26, 2025 12:23

Add description for views.coffee and fix typos older than my cat

1c319af

Add description for models.coffee and remove unnecessary console logs

c63f968

Add description for routes.coffee and remove unnecessary console log

2c10040

Add description for job.coffee and remove old TODOs

10318c6

Add updated version of D3. Add cytoscape and dagre for eventual DAG u…

79b037d

…dpates

Remove D3 v3 in favour of v7. Update index imports and comment on the…

79ccfd4

… importance of ordering

Update timeline for D3 v7. Fix handling of boundaries when looking at…

cf2a092

… less than a few minutes of data. Add horizontal guidelines for better legibility. Add code comments

Update module initialization to be consistent across code

974a909

Update span for config panel so that it actually takes up the full grid

fdc50f8

Update LESS to use production mode so we aren't logging development m…

6c06d3e

…essages in the console

Formatting and some basic comments

661192b

Update bootstrap graph sizing of details and action graph. Remove man…

d1cc001

…ual graph height adjustment. Update popup template to be a bit more generic for use between Job and JobRun

Update mixins to better reflect current state. Update popup styles fo…

cc4f3a9

…r a new age

Replace d3-force with cytoscape rendering and a dagre layout algorith…

1cd9392

…m. Update styles for better popups and JobRun state visualization. Add search and reset buttons to the graph. Update fit logic to suit the modal size automatically. Many other fixes

Remove D3 force styles. Remove commented LESS

a0a82be

Add explanatory comment around min and maz zoom

ee64b84

Fix child propagation issue where clicks on the odal graph impacted t…

9391c8c

…he main page graph behind it

KaspariK marked this pull request as ready for review March 5, 2025 22:10

KaspariK requested a review from a team as a code owner March 5, 2025 22:10

KaspariK commented Mar 5, 2025

View reviewed changes

KaspariK commented Mar 7, 2025

View reviewed changes

EvanKrall previously approved these changes Mar 7, 2025

View reviewed changes

nemacysts previously approved these changes Mar 11, 2025

View reviewed changes

Add TODOs for future graph edge changes. Tidy up some CSS. Add .unkno…

2909892

…wn as a new colour on the graph

KaspariK dismissed stale reviews from nemacysts and EvanKrall via 2909892 March 13, 2025 16:19

EvanKrall approved these changes Mar 21, 2025

View reviewed changes

KaspariK merged commit 4d57085 into master Mar 21, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

U/kkasp/tron 2370 do ya like dags #1032

U/kkasp/tron 2370 do ya like dags #1032

KaspariK commented Mar 3, 2025 •

edited

Loading

KaspariK commented Mar 5, 2025

KaspariK Mar 5, 2025 •

edited

Loading

nemacysts Mar 6, 2025

KaspariK Mar 7, 2025

KaspariK Mar 7, 2025

EvanKrall left a comment

nemacysts left a comment

nemacysts Mar 6, 2025

KaspariK Mar 11, 2025

nemacysts Mar 6, 2025

KaspariK Mar 11, 2025

nemacysts Mar 11, 2025

nemacysts Mar 6, 2025

nemacysts Mar 6, 2025

nemacysts Mar 6, 2025

nemacysts Mar 6, 2025

nemacysts Mar 6, 2025

nemacysts Mar 6, 2025

KaspariK Mar 11, 2025

U/kkasp/tron 2370 do ya like dags #1032

U/kkasp/tron 2370 do ya like dags #1032

Conversation

KaspariK commented Mar 3, 2025 • edited Loading

What

Main changes

Why

Documentation

Bug fixes and miscellanea

Validation

GIFs

Browsers

See it live!

KaspariK commented Mar 5, 2025

KaspariK Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanKrall left a comment

Choose a reason for hiding this comment

nemacysts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaspariK commented Mar 3, 2025 •

edited

Loading

KaspariK Mar 5, 2025 •

edited

Loading