Use deltas in GScan

[kinow]

These changes close a lot of issues, listed below. Do not worry about assigning to me, setting milestone, etc. After the final review, and when this issue is closed, then I will go through each issue linked below, will assign and set the milestone 👍

• close Use deltas in GScan #542 (main issue)
• close Use query variables in GraphQL instead of string replace #234
  • Now there is a JS object with the variables in SubscriptionQuery class, that gets filled from the VueRouter + VuexStore info via the graphql.js mixin 👍
  • code review
• close WorkflowService doesn't work with nodesEdges, edges, or any other top level entry #209
  • Now we declare the query and callbacks. Previously, there was some code in WorkflowService & GQuery that was calling setWorkflows(data.workflow), but now the callbacks receive the data as is, and can do whatever they need with it, be it nodes, workflows, deltas, etc.
  • code review
• close Reduce duplicated code when using WorkflowService #160
  • Achieved that by using Mixins. Should be easy to port to Vue 3 too (thought about that when trying a few different approaches).
  • code review
• close Use graphql-tag instead of Strings for GraphQL queries, fragments #356
  • code review
• partially address expose the cylc views standalone #376 (@oliver-sanders I've got an idea to improve it)
  • We are now adding views directly to the widgets. So any view (with the right mixins) can be loaded in the Workflow view & component.
  • code review
• close Prevent unnecessary subscription requests #388
  • By coordinating in mixins when a subscription is merged/created, and only starting it in the VueRouter navigation guard (subscriptionView mixin) we guarantee that we don't start subscriptions that are immediately going to be stopped.
  • review with browser network & other developer tools
• close WorkflowService/GQuery fetches more data than what it needs when GScan + a view/component are used #416
  • We are not mixing the GScan and Tree queries any longer, they are kept as separated GraphQL web sockets subscriptions.
  • code review or use browser network & other developer tools
• close Confirm we don't have subscriptions outliving the views/use, to avoid performance issues, and add unit/e2e tests #453
  • e2e tests added, also simplified with the mixins.
  • code review
• close Avoid data duplication in added deltas #661
  • The workflows data (GScan, Dashboard, WorkflowsTable) is in Vuex store's workflows.workflows. The workflow data is stored in workflows.lookup (workflow subscription lookup with id as key, and GraphQL data as value), and Tree view data is stored in workflows.workflow. The Tree view is re-using the data in workflows.lookup when it creates workflows.workflow, avoiding data duplication.
  • code review and/or vue-dev-utils browser extension, inspecting the Vuex store when browsing different views
• close Create a single object to receive the deltas of workflow-subscriptions #684
  • See above.
• close alert: repeated errors are ignored #625
  • Noticed that after dismissing the alert, I was not getting notified about the new alerts (that I knew were supposed to happen). Remembered about this issue, and added a call to setAlert(null) when the user dismisses the alert. Note: if the user does not dismiss the alert, once a new alert appears, it will replace the existing alert 👍
  • code review
• close gscan: installed workflows #688
  • Depends.. it should close what was originally reported, but due to recent discussion, we may either remove the issue from this list, or create a follow-up issue.
  • code review
• close UI shows loading state forever if no workflows exist #671
  • One more fixed in this branch 🎉 was triaging issues, then decided to test this branch. A query does not have an initial data burst, so the UI kept waiting forever for GScan and Dashboard queries, loading until it got some data. But now as everything in the UI is using deltas after this branch (cc @hjoliver just FWIW, you can confirm by looking at the queries.js file in this branch, at what's exported… there should be only subscription queries with deltas) the issue does not happen anymore.

Requirements check-list

• I have read CONTRIBUTING.md and added my name as a Code Contributor.
• Contains logically grouped changes (else tidy your branch by rebase).
• Does not contain off-topic changes (use other PRs for other changes).
• Already covered by existing tests.
• Appropriate change log entry included.
• No documentation update required.

Notes from discussions on Element and comments here:

• we can use stateTotals to retrieve the states in the workflow, with their count too
• when showing states, we need to filter the ones that are applicable to job (i.e. filter with the JobStatus enum!)
• we can handle when workflows are moved/renamed

[oliver-sanders]

And from what I remember, we are not able to merge deltas subscriptions.

Yeah, it gets messy. Conceptually it's easy:

subs:
    1: {a, b, {c}}
    2: {a: {d}}

incoming delta:
   {A, {C}}

internal deltas:
    1: {A, {C}}
    2: {A}

But in practice GraphQL isn't that abstract, need to spend some time on this one soonish to see if it's achievable easily. It's filters that really mess it up, e.g. filtering by task state but I guess that could be handled in a simple dumb manner just merging what we can:

subs:
    1:
        filters: {workflow: 'workflows: ["five"]' taskProxy: 'state: ["failed"]'}
        sub: {a, b, {c}}
    2:
        filters: {workflow: 'workflows: ["six"]'}
        sub: {a, {d}}

[oliver-sanders]

GScan doesn't seem to respond to the addition or removal of stopped workflows. I think it might not be responding to the full range of flow change events. Perhaps we need to add more info to the deltas?

Steps to reproduce:

1. Create a workflow called five (but don't run it).
2. Load the UI, you should see it in the sidebar.
3. Rename five -> six.

The five flow should disappear and six should be added.

Something strange is happening in the dashboard too which might be related. The number of stopped workflows keeps flickering between two

different values.

I think one matches the number visible in GScan (correct when the page loaded) and the other matches the current number of flows.

[kinow]

Oh, never thought about renaming workflows. The UIS probably doesn't send a message telling the UIS to remove that workflow, or if does, I think the UI doesn't handle that 😥

The old code, that is not deltas-based, would handle that as it always re-rendered the whole list, so I think we can't push that to a follow-up. Will have to think about this one and confirm whether there's a solution in the UI. Otherwise will create and link an issue in UIS.

Thanks for the testing @oliver-sanders !

[oliver-sanders]

Made it to the end 😓.

Looks great, the queries and subs are nicely centralised and the mixins should make view development easier 👍.

Still need to dive into the subscription merging (unexpected bonus 🎉). To help me review this could you comment about what the subscription merging is supposed to merge and what it isn't (i.e. what subscriptions we should expect to have for different views).

Still need to do some hands on testing with:

• Lookup / tree structures.
• Subscription merging.
• Delta processing.

I have only found a couple of small snags so far which are to do with the life cycle of subscriptions.

Here's the diff I used for testing (you may need to correct me if my method is wrong):

diff --git a/src/services/workflow.service.js b/src/services/workflow.service.js
index 455f60e8..ccdf521c 100644
--- a/src/services/workflow.service.js
+++ b/src/services/workflow.service.js
@@ -203,6 +203,8 @@ class WorkflowService {
     } catch (e) {
       subscription.handleViewState(ViewState.ERROR, e)
     }
+
+    console.warn('# START', this.subscriptions)
   }
 
   /**
@@ -271,6 +273,7 @@ class WorkflowService {
       store.dispatch(actionName).then(() => {})
     })
     delete this.subscriptions[subscription.query.name]
+    console.warn('# STOP', this.subscriptions)
   }
 
   // --- Merging GraphQL queries...

Note: This logs the subscriptions AFTER a subscription has been started or stopped.

And here are my results:

Experiment 1: Move from the Dashboard to workflow page.

The "root" subscription is cancelled and re-started causing GScan to flicker.

Experiment 2: Open new tree views

(Note: to replicate this I had to start on the Dashboard page)

The "root" subscription is canceled and re-started.

[kinow]

Having some spare time this week after the work on this PR, and don't want to add more PR's that will cause this PR to have to be rebased or its merge delayed. So decided to cover a part of the new code that I thought wasn't too important to add tests initially (but doesn't hurt to have more tests right :) Last commit only includes a handful new unit tests, no changes to the code.

[kinow]

Here's the diff I used for testing (you may need to correct me if my method is wrong):

@oliver-sanders I think your diff is correct. And the results of your test look correct to me. Initially, I re-used the GScan query for Dashboard & WorkflowTables. Which resulted in less subscriptions being stopped and started again, but then I thought it would be best to keep things as they are for now, and focus on getting the GScan component and the subscriptions in place first, leaving a few teaks like this one for later.

Probably to be addressed when we work on the new issues for query merging after this PR.

[oliver-sanders]

I think the first example requires some extra logic to handle nicely (suggestion below), the second example (opening a new tree view) looks like a bug on this branch causing the root sub to be cancelled when trying to add a workflow sub?

Perhaps the solution to the first issue is to make the "unregister subscription" logic schedule a callback rather than cancelling the sub straight away. The callback can then check whether any new view has subscribed to the sub in the mean time.

This way the GScan subscription wouldn't get cancelled. This leads onto the idea of keeping workflow subs active for a configured period so that users can switch back and forth between workflows with minimum waiting. Related to #392.

[kinow]

looks like a bug on this branch causing the root sub to be cancelled when trying to add a workflow sub?

I thought the root subscription had been cancelled because the Dashboard query had been removed, resulting in a call to recompute and then triggering the stop/start in the subscription.

I will take a look tomorrow, but you are correct, it's supposed to stop root only iff the root's query changed, not because of any changes to the workflow query.

[kinow]

@oliver-sanders found the bug that was causing the GScan root subscription to be stopped and started again! The subscription was marked as reload = true because of the merge of GScan + Dashboard queries.

The issue was that I was never setting reload = false 😅

Fixed, and found another bug where I was not saving the result of union's of collections. Fix is really small, but I will probably need a bit more of time to write tests for these 2 issues I just found, to prevent them from happening again 👍

Thanks!!!

[kinow]

Done! If tests pass in CI, it should not stop/start the root subscription when going from Dashboard to the Tree view or Workflow with Tree view 👍

[oliver-sanders]

Brilliant that fixes both of my scenarios 💯 .

[oliver-sanders]

Great 🍾, really nice code, should give us a pattern to grow into.

[kinow]

Thanks @oliver-sanders !

[hjoliver]

I'm having a good play with this, probably can't improve on @oliver-sanders ' thorough review. Looks great so far, massive effort to close so many issues, thanks @kinow 💐

[hjoliver]

Very nice 👍