Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count #5999

MichaelBuessemeyer · 2022-01-31T10:36:12Z

This PR tackles two small bugs. The first is, that the save queue tried to read a clean save state even when a timeout triggered the updates. This caused a lot of update actions send over the network. Now save updates triggered by a timeout only take the current changes and send these to the backend. It is no longer tried to reach a save state where the backend knows all changes the frontend has made so they are in sync.

The second part fixes that the load_custom_layout analytics event was triggered even when the active viewport was changed (e.g. via clicking). The is omits these updates. @normanrz @philippotto Could you think of anything else that should be ignored? Additionally, I could attach the action that triggered the analytics event (a very small object) to the event. This might give more insight if wanted. Such an action looks like this:

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

Testing the save request changes:

Open a tracing.
Open the network tab of the browser and clear it.
Move through the dataset for at least 30 seconds.
Search in the network tab with the string "update"
There should only be 2 update calls triggered aber 30 seconds of moving
Do the same thing on the master. There should be much more update request. They should continue being sent until you stop moving.

Testing the analytics events changes:

Open a tracing and the network tab.
Once the tracing is loaded, clear the network tab.
Then switch between viewports via clicking.
There should not be any analytics update with load_custom_layout as a key.
Do the same on the master. There should be some analytics events with the key load_custom_layout.

Issues:

(Please delete unneeded items, merge only when none are left open)

Updated (unreleased) changelog
Ready for review

MichaelBuessemeyer · 2022-01-31T11:47:38Z

@philippotto Do you want to / could you please take a look at this?

MichaelBuessemeyer · 2022-01-31T11:56:40Z

frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.js

@@ -313,7 +315,6 @@ class FlexLayoutWrapper extends React.PureComponent<Props, State> {
  }

  onLayoutChange = () => {
-    sendAnalyticsEvent("change_tracing_layout", { viewMode: this.props.layoutKey });


I removed the analytics here and moved it into onAction as each change to the layout, first triggers an action. This method gets information about what action was done and therefore can better decide whether to send an analytics event.

However, the onAction method is not called when the layout is completely rebuild. For that case the event is also send in rebuildLayout.

frontend/javascripts/test/sagas/save_saga.spec.js

philippotto

Great! Works very well 👍 Regarding the events, I noticed two further things which should probably be ignored.

frontend/javascripts/oxalis/model/sagas/save_saga.js

philippotto · 2022-02-03T13:46:07Z

frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.js

@@ -68,6 +68,8 @@ type State = {
  model: Model,
 };

+const ignoredLayoutChangesByAnalytics = ["FlexLayout_SetActiveTabset"];


I think, the following scenarios shouldn't send an event, either:

toggling the side bars

switching tabs (in the sidebar, the event is already ignored. but when moving XY and XZ next to each other, switching the tabs sends events)

But moving the viewport around and resizing them should still send the event? Or should the event only be send when switching between layouts?

Edit: And what about maximizing a viewport?

I think it depends on what exactly we want to track / use this tracking event for.

@normanrz Do you have an opinion what we want to track with the change_tracing_layout event?

I think it depends on what exactly we want to track / use this tracking event for.

Yes, definitely. I don't have a strong opinion about what we really want to track, but I'm leaning towards tracking only stuff which is not "obvious" (e.g., it seems obvious that users will click on tabs to switch between them). With regard to maximizing, resizing and switching layouts, I'm not too sure that this is obvious for all users. Therefore, these events seem more interesting to me. However, in the end, this is also not very high-pri / not worth delving into how to filter the events best. So, maybe opt for the easier route if it makes sense (e.g., only track switches between layouts).

philippotto

Excellent!

MichaelBuessemeyer · 2022-02-07T10:53:03Z

With regard to maximizing, resizing and switching layouts, I'm not too sure that this is obvious for all users. Therefore, these events seem more interesting to me.

I just added a commit that adds an additional analytics event that tracks when the user changed between any two layouts. This allows tracking whether multiple layouts are actively used / switched between or whether a user usually stays at only one active layout changing this one over and over again.
Do you think this makes sense @philippotto? If not, I can just revert this commit.

MichaelBuessemeyer · 2022-02-07T10:55:07Z

frontend/javascripts/oxalis/view/layouting/flex_layout_wrapper.js

  componentDidUpdate(prevProps: Props) {
    const { layoutName, layoutKey } = this.props;
    if (layoutName !== prevProps.layoutName || layoutKey !== prevProps.layoutKey) {
+      sendAnalyticsEvent("switched_layout", {
+        from: { viewMode: prevProps.layoutKey, layoutName: prevProps.layoutName },
+        to: {
+          viewMode: this.props.layoutKey,
+          layoutName: this.props.layoutName,
+        },
+      });
      this.rebuildLayout();
    }
  }


Here is the change 👀

philippotto · 2022-02-07T10:58:40Z

I just added a commit that adds an additional analytics event that tracks when the user changed between any two layouts. This allows tracking whether multiple layouts are actively used / switched between or whether a user usually stays at only one active layout changing this one over and over again.
Do you think this makes sense @philippotto? If not, I can just revert this pr.

Great! Makes total sense 👍

philippotto · 2022-02-14T12:36:32Z

@MichaelBuessemeyer What's the status here? I think, the PR can be merged?

MichaelBuessemeyer · 2022-02-14T15:09:47Z

@MichaelBuessemeyer What's the status here? I think, the PR can be merged?

Whoopsie, I forgot this pr.

…ssos into docs * 'docs' of github.com:scalableminds/webknossos: * 'master' of github.com:scalableminds/webknossos: Split cells via Min Cut (#5885) Clean up backend util package (#6048) Guard against empty saves (#6052) Time tracking: Do not fail on empty timespans list (#6051) Fix clip button changing position (#6050) Include ParamFailure values in error chains (#6045) Fix non-32-aligned bucket requests (#6047) Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count (#5999) Bump cached-path-relative from 1.0.2 to 1.1.0 (#5994) Volume annotation download: zip with BEST_SPEED (#6036) Sensible scalebar values (#6034) Faster CircleCI builds (#6040) move to Google Analytics 4 (#6031) Fix nightly (fix tokens, upgrade puppeteer) (#6032) Add neuron reconstruction job backend and frontend part (#5922) Allow uploading multi-layer volume annotations (#6028)

* docs: Split cells via Min Cut (#5885) Clean up backend util package (#6048) Guard against empty saves (#6052) Time tracking: Do not fail on empty timespans list (#6051) Fix clip button changing position (#6050) Include ParamFailure values in error chains (#6045) Fix non-32-aligned bucket requests (#6047) Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count (#5999) Bump cached-path-relative from 1.0.2 to 1.1.0 (#5994) Volume annotation download: zip with BEST_SPEED (#6036) Sensible scalebar values (#6034) Faster CircleCI builds (#6040) move to Google Analytics 4 (#6031) Fix nightly (fix tokens, upgrade puppeteer) (#6032) Add neuron reconstruction job backend and frontend part (#5922) Allow uploading multi-layer volume annotations (#6028)

MichaelBuessemeyer added 3 commits January 31, 2022 11:30

don't enforce save state when saving is triggered by a timeout

9d5ed7d

add changelog entry

47f6ab7

Do not send changed tracing layout analytics event

aa71c7a

MichaelBuessemeyer self-assigned this Jan 31, 2022

MichaelBuessemeyer added bug frontend labels Jan 31, 2022

MichaelBuessemeyer changed the title ~~Don't enforce save state when saving is triggered by a timeout~~ Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count Jan 31, 2022

MichaelBuessemeyer requested a review from philippotto January 31, 2022 11:47

Merge branch 'reduce-layout-changed-analytics' into reduce-save-requests

4530579

MichaelBuessemeyer commented Jan 31, 2022

View reviewed changes

MichaelBuessemeyer added 2 commits January 31, 2022 13:07

fix analytics event not being send when toggeling the borders

d0965d4

fix save saga tests

82f1aed

MichaelBuessemeyer commented Jan 31, 2022

View reviewed changes

frontend/javascripts/test/sagas/save_saga.spec.js Show resolved Hide resolved

Merge branch 'master' into reduce-save-requests

cc86df3

philippotto reviewed Feb 3, 2022

View reviewed changes

apply feedback & don't track viewport tab selection and border toggles

a1fec6e

philippotto approved these changes Feb 4, 2022

View reviewed changes

track switching between layouts explicitly

7a84f0a

MichaelBuessemeyer commented Feb 7, 2022

View reviewed changes

Merge branch 'master' into reduce-save-requests

22b48cd

MichaelBuessemeyer force-pushed the reduce-save-requests branch from 59b2e10 to 22b48cd Compare February 7, 2022 10:56

Merge branch 'master' into reduce-save-requests

976bcca

MichaelBuessemeyer enabled auto-merge (squash) February 10, 2022 09:19

Merge branch 'master' into reduce-save-requests

5cb7fc4

MichaelBuessemeyer disabled auto-merge February 10, 2022 16:17

Merge branch 'master' into reduce-save-requests

1d3c77d

philippotto and others added 2 commits February 14, 2022 13:36

Merge branch 'master' into reduce-save-requests

bac3f65

Merge branch 'master' into reduce-save-requests

c0b664b

MichaelBuessemeyer enabled auto-merge (squash) February 14, 2022 15:09

MichaelBuessemeyer merged commit 3831806 into master Feb 14, 2022

MichaelBuessemeyer deleted the reduce-save-requests branch February 14, 2022 15:33

philippotto mentioned this pull request Feb 16, 2022

Guard against empty saves #6052

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count #5999

Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count #5999

MichaelBuessemeyer commented Jan 31, 2022 •

edited

Loading

MichaelBuessemeyer commented Jan 31, 2022

MichaelBuessemeyer Jan 31, 2022

philippotto left a comment

philippotto Feb 3, 2022

MichaelBuessemeyer Feb 3, 2022 •

edited

Loading

philippotto Feb 3, 2022

philippotto left a comment

MichaelBuessemeyer commented Feb 7, 2022 •

edited

Loading

MichaelBuessemeyer Feb 7, 2022

philippotto commented Feb 7, 2022

philippotto commented Feb 14, 2022

MichaelBuessemeyer commented Feb 14, 2022

Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count #5999

Don't enforce save state when saving is triggered by a timeout and reduce tracing layout analytics event count #5999

Conversation

MichaelBuessemeyer commented Jan 31, 2022 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

MichaelBuessemeyer commented Jan 31, 2022

MichaelBuessemeyer Jan 31, 2022

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

philippotto Feb 3, 2022

Choose a reason for hiding this comment

MichaelBuessemeyer Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

philippotto Feb 3, 2022

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Feb 7, 2022 • edited Loading

MichaelBuessemeyer Feb 7, 2022

Choose a reason for hiding this comment

philippotto commented Feb 7, 2022

philippotto commented Feb 14, 2022

MichaelBuessemeyer commented Feb 14, 2022

MichaelBuessemeyer commented Jan 31, 2022 •

edited

Loading

MichaelBuessemeyer Feb 3, 2022 •

edited

Loading

MichaelBuessemeyer commented Feb 7, 2022 •

edited

Loading