Arvados

This story experienced some scope creep due to the need to rework websocket based refreshing to really solve the problem. Summary of major changes:

• An "arv-log-event" sent to an element with "arv-log-event-listener" class no longer automatically triggers a refresh. To get this behavior requires an additional "arv-refresh-on-log-event" class
• Tabs continue to refresh when they are clicked on, but the "refresh when clicked on" behavior can be disabled by specifying ":no_auto_refresh => true" for the tab pane in the show_panes_list of the controller.
• Tabs no longer refresh by default based on websocket events
• The log message log append function now directly tracks updates of the pipeline to determine which jobs are part of the pipeline instead of using the contents of the Components tab (which was actually broken because the contents of the components tab changed)
• Pipeline instances, jobs, and dashboard now refresh based on websocket events instead of on timers. The refresh behavior is throttled with a minimum time between refreshes to reduce load on the server.
• Workbench tests now start a websocket server in addition to the API server. This needs to run separately because it cannot run over HTTPS because the browser will silently fail to connect when the certificate is self-signed
• Added websocket tests for updating logs based on websocket events (including the scrollbar positioning, the original topic of the story) and refreshing pipeline, job and dashboard in response to websocket events.

• -            var iframe = $('iframe', this)[0];
  +                var iframe = $('iframe', e.target)[0];
  

Evidently this is worth testing, since it's apparently easy to break for a month and a half without noticing! It might(?) also be clearer to set var $pane = this at the top of the handlers rather than using a closure (that's what context does in the $.ajax call).

It's a bit confusing that the "replace data-object-uuids when pipeline changes" behavior is bundled with the "arv-log-event-handler-append-logs" pseudoclass and "append logs" behavior. I think it would be clearer (and really easy) to decouple this -- perhaps "arv-log-event-subscribe-to-pipeline-job-uuids"?

• +        setTimeout(function() {
  +            etarget.trigger('arv:pane:reload');
  +        });
  

• +// Panes can be in one of four states: not loaded (no state classes), pane-loading, pane-loading+pane-stale, pane-loaded
  

•          // When the user selects e.target tab, show a spinner instead of
           // old content while loading.
  +        etarget.removeClass('pane-loading');
  +        etarget.removeClass('pane-loaded');
  +        etarget.removeClass('pane-stale');
  +
           $pane.html('<div class="spinner spinner-32px spinner-h-center"></div>');
  

•         etarget.removeClass('pane-loaded');
          etarget.removeClass('pane-stale');
  

• // Fire when a tab is selected/clicked.
  $(document).on('shown.bs.tab', '[data-toggle="tab"]', function(event) {
      var tgt = $($(event.relatedTarget).attr('href'));
      ...
  

I think the etarget variable should be called something more descriptive (perhaps $tab) to [a] follow the convention of naming jQuery result sets $foo, and [b] focus on what it is rather than how we found it.

• -      pid = IO.read(if @websocket then WEBSOCKET_PID_PATH else SERVER_PID_PATH end).to_i
  +      pid = IO.read(@pidfile).to_i
  

Commented-out debug printfs should be removed (try git diff master... | grep console.log)

The new tests aren't passing for me -- haven't figured out why yet. (PermissionDenied → AccessForbidden, ha!)

WebsocketTest#test_dashboard_arv-refresh-on-log-event:
ArvadosApiClient::AccessForbiddenException: #<ArvadosModel::PermissionDeniedError: ArvadosModel::PermissionDeniedError> [API: 403]
    app/models/arvados_api_client.rb:147:in `api'
    app/models/arvados_base.rb:182:in `save'
    app/models/arvados_base.rb:158:in `create'
    test/integration/websockets_test.rb:141:in `block in <class:WebsocketTest>'

Tom Clegg wrote:

The first thing I did was "all jobs" → a job → clicked on a few tabs, and the Provenance page showed a "reload tab" button and an empty iframe instead of an error. Git seems to blame your commit, but not on this branch -- in 9be9761 on September 11, the following thing happened. (this == $pane here, and that's where the iframe is -- not e.target, which is the tab. See comment below about etarget.)

• [...]

Fixed and tested manually. However I'm not sure how to induce an error automatically in order to write a test.

Evidently this is worth testing, since it's apparently easy to break for a month and a half without noticing! It might(?) also be clearer to set var $pane = this at the top of the handlers rather than using a closure (that's what context does in the $.ajax call).

Changed to use $(this) instead.

It's a bit confusing that the "replace data-object-uuids when pipeline changes" behavior is bundled with the "arv-log-event-handler-append-logs" pseudoclass and "append logs" behavior. I think it would be clearer (and really easy) to decouple this -- perhaps "arv-log-event-subscribe-to-pipeline-job-uuids"?

Fixed.

Does this need a timeout period -- perhaps throttle - (now - loaded_at) -- to avoid wasting a lot of CPU?

• [...]

How did that ever work? Fixed.

A fifth state, pane-loaded+pane-stale, also seems to be part of the plan:

• [...]

Improved comments explaining the states.

This comment has become detached from the code it refers to:

• [...]
  Both sides of the if ($pane.hasClass('active')) condition start by doing this. Perhaps it should be outside the condition:
• [...]

Moved the code to remove the classes outside of the if ($pane.hasClass('active')) condition.

Can you explain what the new "tgt" stuff is doing here? It looks like it's doing something bootstrap already does by itself (set the "active" class on the current tab's content div):

• [...]

Added comments. Let me know if you need further clarification.

I think the etarget variable should be called something more descriptive (perhaps $tab) to [a] follow the convention of naming jQuery result sets $foo, and [b] focus on what it is rather than how we found it.

Renamed to $anchor

In apps/workbench/test/test_helper.rb → run(), you might as well set @pidfile to the pid file you're actually using, instead of repeating/rephrasing the pidfile logic later...

• [...]

Fixed.

Commented-out debug printfs should be removed (try git diff master... | grep console.log)

Fixed.

The new tests aren't passing for me -- haven't figured out why yet. (PermissionDenied → AccessForbidden, ha!)
[...]

Merged master and running tests now, I'll let you know.

Peter Amstutz wrote:

Tom Clegg wrote:

The first thing I did was "all jobs" → a job → clicked on a few tabs, and the Provenance page showed a "reload tab" button and an empty iframe instead of an error. Git seems to blame your commit, but not on this branch -- in 9be9761 on September 11, the following thing happened. (this == $pane here, and that's where the iframe is -- not e.target, which is the tab. See comment below about etarget.)

Fixed and tested manually. However I'm not sure how to induce an error automatically in order to write a test.

Punted to #4339.

Evidently this is worth testing, since it's apparently easy to break for a month and a half without noticing! It might(?) also be clearer to set var $pane = this at the top of the handlers rather than using a closure (that's what context does in the $.ajax call).

Changed to use $(this) instead.

$(this) is redundant -- just $(jqueryresult) returning jqueryresult every time. You could use this or just set var $pane = this.

Does this need a timeout period -- perhaps throttle - (now - loaded_at) -- to avoid wasting a lot of CPU?

• [...]

How did that ever work? Fixed.

Apparently the default timeout is zero, "next time you hit the event loop".

A fifth state, pane-loaded+pane-stale, also seems to be part of the plan:

• [...]

Improved comments explaining the states.

I suspect the "two additional states" section is misleading. My reading of the code suggests that the additional states are "pane-loading pane-stale" and "pane-loaded pane-reload-pending", but the comments say the additional states are "pane-stale" and "pane-reload-pending".

(With five state flags, this is starting to look like it should be done as data-tab-state="foo"... but I don't think we should go there today.)

Can you explain what the new "tgt" stuff is doing here? It looks like it's doing something bootstrap already does by itself (set the "active" class on the current tab's content div):

• [...]

Added comments. Let me know if you need further clarification.

Yes -- I'd still like to know why/when we disagree with bootstrap about which elements should have the "active" class.

I think the etarget variable should be called something more descriptive (perhaps $tab) to [a] follow the convention of naming jQuery result sets $foo, and [b] focus on what it is rather than how we found it.

Renamed to $anchor

Huh, "anchor" is a source or destination of a link, which isn't very specific. But at least the comment tells you what it is, so that's good.

In apps/workbench/test/test_helper.rb → run(), you might as well set @pidfile to the pid file you're actually using, instead of repeating/rephrasing the pidfile logic later...

• [...]

Fixed.

Well, not really. At ab874e9 you're still repeating the logic in two places. You can set the instance variable in run() like this, and remove the if @websocket stuff from find_server_pid.

•       if @websocket
          @pidfile = WEBSOCKET_PID_PATH
          _system('bundle', 'exec', 'passenger', 'start', '-d', '-p3333',
                  '--pid-file', @pidfile)
        else
          @pidfile = SERVER_PID_PATH
          make_ssl_cert
          _system('bundle', 'exec', 'rake', 'db:test:load')
          _system('bundle', 'exec', 'rake', 'db:fixtures:load')
          _system('bundle', 'exec', 'passenger', 'start', '-d', '-p3000',
                  '--pid-file', @pidfile,
                  '--ssl',
                  '--ssl-certificate', 'self-signed.pem',
                  '--ssl-certificate-key', 'self-signed.key')
        end
  

The new tests aren't passing for me -- haven't figured out why yet. (PermissionDenied → AccessForbidden, ha!)
[...]

Merged master and running tests now, I'll let you know.

Thanks

Tom Clegg wrote:

Punted to #4339.

Thanks.

Evidently this is worth testing, since it's apparently easy to break for a month and a half without noticing! It might(?) also be clearer to set var $pane = this at the top of the handlers rather than using a closure (that's what context does in the $.ajax call).

Changed to use $(this) instead.

$(this) is redundant -- just $(jqueryresult) returning jqueryresult every time. You could use this or just set var $pane = this.

Fixed.

I suspect the "two additional states" section is misleading. My reading of the code suggests that the additional states are "pane-loading pane-stale" and "pane-loaded pane-reload-pending", but the comments say the additional states are "pane-stale" and "pane-reload-pending".

(With five state flags, this is starting to look like it should be done as data-tab-state="foo"... but I don't think we should go there today.)

Expanded comments a bit, and I agree that a data- attribute would probably be nicer.

Can you explain what the new "tgt" stuff is doing here? It looks like it's doing something bootstrap already does by itself (set the "active" class on the current tab's content div):

• [...]

Added comments. Let me know if you need further clarification.

Yes -- I'd still like to know why/when we disagree with bootstrap about which elements should have the "active" class.

Tabs no longer refresh by default based on websocket messages (although they do refresh based on user interaction). On the components tab of the pipeline instance page, the refresh is part of the tab pane contents, instead of the tab itself. That means the notion of it being "active" or not is outside of the view of bootstrap. Since the reload event only fires if the element is "active", we need to set that element to be "active" explicitly when switching to the tab, and remove the "active" class when we switch away to another tab (otherwise it will continue to reload and put load on the server even though the contents are not displayed.)

Possibly another way to do this would be to decide whether to refresh the tab based on there being a parent node in the DOM that has the 'active' class.

I think the etarget variable should be called something more descriptive (perhaps $tab) to [a] follow the convention of naming jQuery result sets $foo, and [b] focus on what it is rather than how we found it.

Renamed to $anchor

Huh, "anchor" is a source or destination of a link, which isn't very specific. But at least the comment tells you what it is, so that's good.

Well, in the case of bootstrap tabs it actually is an HTML anchor, with generic "pane-anchor" it can technically be any element, but it does use an href to point to the node that's actually being manipulated.

Well, not really. At ab874e9 you're still repeating the logic in two places. You can set the instance variable in run() like this, and remove the if @websocket stuff from find_server_pid.

Done

Merged master and running tests now, I'll let you know.

Thanks

Tests should pass now.

Peter Amstutz wrote:

Yes -- I'd still like to know why/when we disagree with bootstrap about which elements should have the "active" class.

the refresh is part of the tab pane contents, instead of the tab itself

• Sometimes, $anchor is a tab link and $pane is the corresponding content div ("the bootstrap pane").
• Other times, $anchor is the bootstrap pane and $pane is a div inside that.
• Sometimes these overlap: the same div (e.g., id="job-status") can go through the "tab reload" function twice, once as $pane when the user clicks the tab, then another time as $pane, when a websocket event happens. Now there are two different data-loaded-at values.

I think this is way too complicated and hard to follow. Why is the extra level of indirection necessary?

• $anchor is the tab link, <a href="#foo" class="pane-anchor">.
• $pane is the tab content pane, <div id="foo">. Bootstrap manages the "active" flag for us.
• In cases like dashboard where there is no tab, $anchor is a hidden element beside the content div (or anywhere, really, except inside the content div), and $pane has a plain class="active" that nobody has to muck with.
• data-loaded-at attribute goes on $anchor.
• pane state flags go on $anchor.
• data-load-throttle attribute goes on $pane.
• arv-refresh-on-log-event class goes on $pane. The handler for this does $('.pane-anchor[href=#' + e.target.id + ']').trigger('arv:pane:reload').
• no need for pane-no-auto-reload (just don't add the arv-refresh-on-log-event class!).
• no need to split tab partials into "the one loaded by clicking" and "the one loaded by auto-refresh".

• skip arv:pane:reload on the "shown" event when $(e.target).hasClass('pane-loaded') (the tab is not stale, so there is no need to reload).
• Three seconds might be too aggressive as a default. Perhaps better to have a long default (15s) and ask for a shorter interval in the cases where we think it's worth the extra load?

Tests should pass now.

Hm, my test suite seems to be failing for unrelated reasons. Trying to see websockets in action in dev mode with my browser in the meantime...

Peter Amstutz wrote:

Tom Clegg wrote:

I think this is way too complicated and hard to follow. Why is the extra level of indirection necessary?

The anchor/panel distinction is entirely due to the fact that this grew out of the initial goal of doing AJAX loading of bootstrap tabs, extending that to refreshing tabs, and then even later extending it to use the same patterns for refreshing arbitrary sections of the DOM.

I understand the anchor/pane distinction, I'm just resisting the anchor/anchor-and-pane/pane distinction because I think the anchor/pane distinction is simpler and can still do what we want.

If you feel strongly that we need to further refactor this in this branch, then I would recommend having the content panel node just hold all the state to refresh itself. No more "anchor node" to hold the metadata.

Hm. I thought it would get too confusing to keep the loaded state inside the part of the DOM that gets replaced during an update.

• no need for pane-no-auto-reload (just don't add the arv-refresh-on-log-event class!).

"pane-no-auto-reload" is badly named, it actually suppresses the "reload on click" behavior, and is orthogonal to arv-refresh-on-log-event

Aha. Yes, that sounds like a feature we don't especially need, as you say below...

Done. That may eliminate the need for "pane-no-auto-reload"

Excellent

• Three seconds might be too aggressive as a default. Perhaps better to have a long default (15s) and ask for a shorter interval in the cases where we think it's worth the extra load?

Done.

Thanks!

Tom Clegg wrote:

I understand the anchor/pane distinction, I'm just resisting the anchor/anchor-and-pane/pane distinction because I think the anchor/pane distinction is simpler and can still do what we want.

I really don't follow what you think the difference between "anchor/anchor-and-pane/pane" and "anchor/pane" is.

Hm. I thought it would get too confusing to keep the loaded state inside the part of the DOM that gets replaced during an update.

Not exactly true. The part that gets replaced during an update are the children of the "pane" node. The "pane" node itself is persistent, although because that node is the target of various operations it seemed like it might be better to keep the state somewhere else out of an abundance of caution.

Done. That may eliminate the need for "pane-no-auto-reload"

Excellent

Removed in f809f3e

Thanks!

Are there any other actionable requests? Otherwise it seems like we should be moving towards merging this thing.

Peter Amstutz wrote:

Tom Clegg wrote:

I understand the anchor/pane distinction, I'm just resisting the anchor/anchor-and-pane/pane distinction because I think the anchor/pane distinction is simpler and can still do what we want.

I really don't follow what you think the difference between "anchor/anchor-and-pane/pane" and "anchor/pane" is.

"anchor-and-pane" is an element that serves as a $pane in the reload function when the user has clicked a tab, and serves as $anchor in the reload function when auto-refresh happens. I think it is bad news that such elements exist.

The main problem here (aside from the general "harder to understand than it needs to be") is that "load tab" and "auto-refresh tab" load different things and insert them in different places. This creates lots of different code paths for (afaict) no particular reason.

Hm. I thought it would get too confusing to keep the loaded state inside the part of the DOM that gets replaced during an update.

Not exactly true. The part that gets replaced during an update are the children of the "pane" node. The "pane" node itself is persistent, although because that node is the target of various operations it seemed like it might be better to keep the state somewhere else out of an abundance of caution.

Good point -- although its content changes, the pane node itself persists, so there's no need to store any state in the $anchor. Even better!

Done. That may eliminate the need for "pane-no-auto-reload"

Excellent

I will go ahead and remove that particular workaround, then.

Sounds good.

Are there any other actionable requests? Otherwise it seems like we should be moving towards merging this thing.

I don't find this acceptable to merge because it is needlessly complicated, provides too many opportunities for subtle bugs, and will be harder to work with in the future.

I wouldn't mind backing out the multi-level stuff myself as described above (with the store-all-state-in-pane-not-anchor change), if you prefer. That would probably be more efficient than explaining back and forth N more times.

Thanks

• Tom Clegg wrote:

"anchor-and-pane" is an element that serves as a $pane in the reload function when the user has clicked a tab, and serves as $anchor in the reload function when auto-refresh happens. I think it is bad news that such elements exist.

Ok, at least I understand your objection now.

The main problem here (aside from the general "harder to understand than it needs to be") is that "load tab" and "auto-refresh tab" load different things and insert them in different places. This creates lots of different code paths for (afaict) no particular reason.

I think you're overstating the case, as there are only two places with this pattern (a reloading pane inside a tab):

1. Showing pipeline components. In this case, the _show_components partial needs to decide whether to show the "running" partial or the "editable" partial. If we want the tab to load and refresh the "running" or "editable" partials directly, this logic needs to be hoisted into the controller.
2. Job status. In this case, the page consists of two elements, the job status (which refreshes) and a list of what pipelines use the job (which doesn't refresh). Reloading the whole tab would incur some unnecessary load by having to re-query pipelines. I suppose that could go into its own tab, although that doesn't do anything to curb our already excessive proliferation of tabs on many pages.

For the other three places that reloading is used, two of them are to keep the action buttons up to date with the job/pipeline state (because the buttons are outside the tabs they need to reload separately from tabs) and to reload the dashboard.

I wouldn't mind backing out the multi-level stuff myself as described above (with the store-all-state-in-pane-not-anchor change), if you prefer. That would probably be more efficient than explaining back and forth N more times.

Take it, then.

• All state goes on $pane. Reload events are sent to $pane. (The only time we work with the erstwhile "anchor" element is when listening for shown.bs.tab events).
• Ignore arv:pane:reload events bubbling up through the DOM.
• Avoid refreshed-element-within-refreshed-element (e.g., job status)
• Detect in the reloader whether content element is a tab pane, and check active flag if it is (instead of assuming $pane author remembered to set a static "active" flag when it isn't).
• Dynamic content can override throttle (e.g., throttle gets shorter when pipeline is running, longer when it finishes).
• Fix crash during error handling when xhr has no Content-Type
• Fix tests to use (new capybara feature!) assert_text, instead of assert page.has_text? which doesn't wait for ajax and therefore fails sometimes due to races.
• Fix some whitespace (4-char indent in event_log.js to match other js)

Comments on 52083e6 4084-log-pane-refresh-TC

• views/jobs/_show_status.html.erb and views/pipeline_instances/show.html.erb still use anchor/pane and need to be updated. (grep for "pane-anchor")
• Why is 'data-object-uuids' copied from arv-log-refresh-control to the refresh node, but it does find('.arv-log-refresh-control') for 'data-load-throttle'?
• Why is a 'pane-no-auto-refresh' class a dirty hack, but waiting 86486400000 milliseconds between refreshes a elegant refactor?

Review feedback: Given the size of the update for 4084, I limited my review to learn about this feature update and understand what was done rather than do a thorough review. I also was able to run a pipeline and see the log pane update with newer log entries and noticed that the log is appended to the bottom of the page without refreshing the entire page. Things looked good on my quick review.

Also:

views/pipeline_instances/_show_components.html.erb

throttle = @object.state == 'Running' ? 5000 : 15000

Should be

throttle = @object.state.start_with?('Running') ? 5000 : 15000

Peter Amstutz wrote:

Comments on 52083e6 4084-log-pane-refresh-TC

• views/jobs/_show_status.html.erb and views/pipeline_instances/show.html.erb still use anchor/pane and need to be updated. (grep for "pane-anchor")

Whoops. Fixed in f9a218a

• Why is 'data-object-uuids' copied from arv-log-refresh-control to the refresh node, but it does find('.arv-log-refresh-control') for 'data-load-throttle'?

It seemed reasonable to search the whole content div for DOM elements during relatively infrequent arv:pane:reload events, but I didn't want the event dispatch process to do that full subtree-starting-at-subscriber element search for N incoming events &times M subscribers, just to figure out which subscribers even want it. Thinking about the efficiency aspect more, though, the "throttled?" block in arv:pane:reload can be expected to process quite a few events too. Do you think it would be better to copy the throttle value as well during ajax:success, for efficiency or consistency?

• When the timer sends a deferred arv:pane:reload event, include some event data that indicates that it came via the timer.
• If $pane.hasClass('pane-reload-pending') and the current event didn't come from the timer, return.

WDYT? Worthwhile? I think this would also be a bit easier to read than checking the throttle every time.

• Why is a 'pane-no-auto-refresh' class a dirty hack, but waiting 86486400000 milliseconds between refreshes a elegant refactor?

I don't recall saying either of those things, so perhaps I'm not the right person to ask. Is this your way of saying you don't appreciate my 1001 Nights joke? Would you be happier with an "end of the Mayan calendar" joke?

We could add something like throttle="Infinity" (don't even bother setting a timer), or add a separate data attribute for "throttle is infinity / never reload", if you think that would be better. It does seem a bit silly to have a timer set for 1001 days instead of just ignoring the reload event, although afaict it gives identical performance with fewer LOC.

Peter Amstutz wrote:

...start_with?('Running')...

Ah, good catch. Fixed in 270b1fc

views/pipeline_instances/_show_log.html.erb also needs to be updated, it (ironically) has the same bug that the story originally set out to fix.

At a7256de... too bad about the copy & paste, but w/e. Let's merge this