[AIRFLOW-620] Add log refresh button to TI's log view page by msumit · Pull Request #1875 · apache/airflow

msumit · 2016-11-09T18:25:08Z

Dear Airflow Maintainers,

Please accept this PR that addresses the following issues:

https://issues.apache.org/jira/browse/AIRFLOW-620

Testing Done:

Manually tested on all major browsers.

This will add log refresh button to TI's log view page, thus eliminating the need of whole page reload to get the latest logs of that TI.

msumit · 2016-11-09T18:39:54Z

codecov-io · 2016-11-09T19:12:36Z

Current coverage is 66.20% (diff: 100%)

Merging #1875 into master will increase coverage by 0.01%

@@             master      #1875   diff @@
==========================================
  Files           128        129     +1   
  Lines          9840       9952   +112   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6513       6589    +76   
- Misses         3327       3363    +36   
  Partials          0          0

Powered by Codecov. Last update f192770...1dd2f62

dgies · 2016-11-10T00:05:12Z

I like the idea but for long logs the user needs to scroll all the way up to refresh, and all the way down to see new data. I would prefer to put the refresh button underneath the log data.

dgies · 2016-11-10T00:06:05Z

airflow/www/templates/airflow/log.html

Move the refresh button here so users can refresh when viewing the log tail

gwax · 2016-11-10T18:51:53Z

👍

dgies · 2016-11-10T20:19:53Z

airflow/www/templates/airflow/log.html

Could you please also add window.scrollTo(0,document.body.scrollHeight); below this line? The refresh sometimes pushes the scrolling back to the top of the page.

I suspect that @dgies's comment is the implementation of what I am about to ask for, but essentially, I'd like a scrolling tailed log that is refreshed by this button and also automatically at a fixed cadence (e.g. every 2-3 seconds with some sort of back off if there is no new data).

I considered that but you'd want to make sure to only refresh when the task is running, which requires polling the task state as well, and probably some special handling around retries. Auto refresh could be a performance problem if the log becomes very long.

yeah, I started keeping that in mind only, but thought of keeping it manual for now. If we do auto refresh then we've to provide a option (checkbox) to disable that as well.

dgies · 2016-11-11T14:46:25Z

+1

r39132 · 2016-11-11T17:50:56Z

I thought I commented on this, but looks like I didn't.

I'd really like a "tail N lines" feature in the log view -- essentially, automatically refreshing the log every S seconds and showing the last N lines (both configurable) to the view port.

I +1 @dgies comment about moving the refresh button to the bottom of the screen. Manually refreshing is okay, but automatically refreshing feels like the right solution.

msumit · 2016-11-15T08:22:56Z

@r39132 in that case we should do something like this:

If a TI is running, server will send the log text along with the pointer (no of bytes) upto which log has been read. In next call, UI will pass that pointer in ajax call and server will send only the new logs after that pointer along with the new pointer.

On UI rather than replacing the dom content, it'll just append to it. This process will continue until the TI finishes. On full page refresh UI will send no pointer and server will respond to full logs.

dgies · 2016-11-16T18:54:36Z

I propose we do not let the perfect become the enemy of the good and merge this as-is (after squashing). Automatic refreshes are trickier to get right and can be added in a new PR without invalidating what's been done already.

msumit · 2016-11-17T06:26:31Z

Yeah, that was my idea as well.. instead of doing all magic in once, do it incrementally..

ldct · 2016-11-19T21:41:24Z

airflow/www/views.py

I don't like relying on is_xhr to do this... it basically uses a non-standard header X-Requested-With (http://flask.pocoo.org/docs/0.11/api/#flask.Request.is_xhr) and will for eg break mysteriously if we move away from $.get

I think alternatives (from best to worst IMO) are

Using a separate (maybe behind our API) endpoint

Use a new query param

Set Accept: text/plain header

im not completely agree to the concern that it'll break if we move away from $.get.. as in case if we do that, we'll take care of backend as well.. its not like someone just gonna change to UI code without altering the backend code as well..

the new api and extra params are overkill for something very simple like that..

but what's the advantage of using nonstandard features like is_xhr?

@zodiac is_xhr may not be the best way, but good enough for this small feature. We of course need it to evolve in to auto-refresh/auto-tailing thing, at that time we can invest on full fledged new api.

ok. this code is still a bit weird so I suggest adding comments pointing out

log.html (the page this view renders) sometimes does an XHR request to itself, and we handle it here

link to http://flask.pocoo.org/docs/0.11/api/#flask.Request.is_xhr since this is a nonstandard feature

LGTM otherwise

ldct · 2016-11-19T21:41:56Z

airflow/www/templates/airflow/log.html

space between (msg) and {

ldct · 2016-11-19T21:42:07Z

airflow/www/templates/airflow/log.html

space between () and {

ldct · 2016-11-19T21:42:39Z

airflow/www/templates/airflow/log.html

move to previous line "click", function () { to reduce indentation

ldct · 2016-11-19T21:45:13Z

airflow/www/templates/airflow/log.html

indentation of ( and ) don't match. should use same format as .fail callback for consistency

ldct · 2017-02-23T02:25:18Z

@msumit shall we rebase, squash and merge this?

msumit · 2017-03-09T13:43:59Z

@zodiac sure, will rebase it this weekend..

This will add log refresh button to TI's log view page, thus eliminating the need of whole page reload to get the latest logs of that TI.

msumit · 2017-03-11T11:37:28Z

@zodiac can u do a final review and merge it?

r39132 · 2018-08-30T18:55:29Z

@msumit What do you want to do with this PR? Close and re-open a fresh one or rebase and ask for a review. If I don't hear back in a few days, I'll close this.

r39132 · 2018-08-31T06:45:20Z

closing for now.. reopen to update and rebase.

dgies reviewed Nov 10, 2016

View reviewed changes

airflow/www/templates/airflow/log.html Outdated

Copy link

dgies Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the refresh button here so users can refresh when viewing the log tail

dgies reviewed Nov 10, 2016

View reviewed changes

ldct reviewed Nov 19, 2016

View reviewed changes

airflow/www/templates/airflow/log.html Outdated

Copy link

Contributor

ldct Nov 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space between (msg) and {

ldct reviewed Nov 19, 2016

View reviewed changes

airflow/www/templates/airflow/log.html Outdated

Copy link

Contributor

ldct Nov 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space between () and {

ldct reviewed Nov 19, 2016

View reviewed changes

airflow/www/templates/airflow/log.html Outdated

Copy link

Contributor

ldct Nov 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to previous line "click", function () { to reduce indentation

ldct reviewed Nov 19, 2016

View reviewed changes

[AIRFLOW-620] Add log refresh button

bf4f1bc

This will add log refresh button to TI's log view page, thus eliminating the need of whole page reload to get the latest logs of that TI.

msumit force-pushed the AIRFLOW-620 branch from 1dd2f62 to bf4f1bc Compare March 11, 2017 11:32

r39132 closed this Aug 31, 2018

Conversation

msumit commented Nov 9, 2016

Uh oh!

msumit commented Nov 9, 2016

Uh oh!

codecov-io commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 66.20% (diff: 100%)

Uh oh!

dgies commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwax commented Nov 10, 2016

Uh oh!

dgies Nov 10, 2016 • 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!

r39132 Nov 11, 2016 • 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!

dgies commented Nov 11, 2016

Uh oh!

r39132 commented Nov 11, 2016

Uh oh!

msumit commented Nov 15, 2016

Uh oh!

dgies commented Nov 16, 2016

Uh oh!

msumit commented Nov 17, 2016

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!

ldct Dec 12, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

ldct commented Feb 23, 2017

Uh oh!

msumit commented Mar 9, 2017

Uh oh!

msumit commented Mar 11, 2017

Uh oh!

r39132 commented Aug 30, 2018

Uh oh!

r39132 commented Aug 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Nov 9, 2016 •

edited

Loading

dgies Nov 10, 2016 •

edited

Loading

r39132 Nov 11, 2016 •

edited

Loading

ldct Dec 12, 2016 •

edited

Loading