[AIRFLOW-620] Feature to tail custom number of logs instead of rendering whole log #3992

phanindhra876 · 2018-10-03T05:19:54Z

Dear Airflow Maintainers,

This is a change in the way we render logs. This PR will give a dropdown of number of lines to be tailed with a refresh button. On refresh webserver fetch selected number of lines from local worker logs or remote worker logs. On can toggle this feature using config variable tail_logs in webserver, default number of lines to tail via config variable num_lines in webserver and items in dropdown can be controlled via config variable tail_lines_list in webserver in csv format

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 a dropdown of number of lines to be tailed and refresh button to get latest logs instead of refreshing the whole page.

codecov-io · 2018-10-03T05:50:12Z

Codecov Report

Merging #3992 into master will increase coverage by 0.5%.
The diff coverage is 38.27%.

@@            Coverage Diff            @@
##           master    #3992     +/-   ##
=========================================
+ Coverage   73.54%   74.04%   +0.5%     
=========================================
  Files         421      421             
  Lines       27724    27790     +66     
=========================================
+ Hits        20389    20577    +188     
+ Misses       7335     7213    -122

Impacted Files	Coverage Δ
airflow/bin/cli.py	`64.45% <10%> (-0.42%)`	⬇️
airflow/utils/helpers.py	`69.54% <3.44%> (-13.22%)`	⬇️
airflow/utils/log/file_task_handler.py	`83% <52.17%> (-6.42%)`	⬇️
airflow/www/views.py	`75.86% <89.47%> (+0.11%)`	⬆️
...etes_request_factory/kubernetes_request_factory.py	`72.82% <0%> (+4.34%)`	⬆️
airflow/configuration.py	`92.08% <0%> (+5.03%)`	⬆️
.../kubernetes_request_factory/pod_request_factory.py	`100% <0%> (+39.28%)`	⬆️
...rflow/contrib/operators/kubernetes_pod_operator.py	`100% <0%> (+40.54%)`	⬆️
airflow/contrib/kubernetes/kube_client.py	`75% <0%> (+41.66%)`	⬆️
airflow/contrib/kubernetes/pod_generator.py	`86.48% <0%> (+43.24%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92727f7...7d9c21e. Read the comment docs.

airflow/bin/cli.py

msumit · 2018-10-08T12:24:26Z

@phani8996 can you post some gifs on this feature in work?

phanindhra876 · 2018-10-08T17:30:53Z

@phani8996 can you post some gifs on this feature in work?

@msumit I am not able to upload gif. I am attaching images for reference.

ashb · 2018-10-09T13:25:57Z

@msumit This isn't the equivalent of tail -f -- it doesn't stream the logs in as they happen, it's just tail -n 300 etc.

ashb · 2018-10-09T13:28:58Z

airflow/bin/cli.py

+                lines = fl.readlines()
+                fl.close()
+                out = "".join(l for l in lines[-num_lines:])
+            line = "***** Showing only last {num_lines} lines from {filename} *****" \


Messages like this don't belong in here - that is something that the FileTaskHandler etc should add.

airflow/www/views.py

airflow/www/templates/airflow/ti_log.html

ashb · 2018-10-09T13:35:15Z

airflow/www/views.py

+                num_lines = conf.getint('webserver', 'num_lines')
+            metadata["num_lines"] = num_lines
+        if num_lines in tail_lines_list:
+            tail_lines_list.remove(num_lines)


What is this conditional for? I can't work it out

That's to avoid duplicates. lets say user have added in config that dropdown should contain entries 100,200,500,1000 and user picked 100 as default number of lines to tail when user lands on get_logs page. Without this condition airflow ending showing 2 entries for 100 in dropdown. This will ensure no duplicates in such case

ashb · 2018-10-09T13:35:19Z

airflow/www/views.py

+            tailing_required = conf.getboolean('webserver', 'tail_logs')
+            if tailing_required and conf.has_option('webserver', 'num_lines'):
+                num_lines = conf.getint('webserver', 'num_lines')
+            metadata["num_lines"] = num_lines


Please find a way of re-writing this. The nested conditions and checks make it hard to see what the intent here is. Perhaps some comment saying when/why we do things would help me understand this code.

I'm also notsure what the webserver -> tail_logs setting is for. Moving that to default config means you can add a comment to it

I am essentially using three fields for this feature. Boolean flag tail_logs in webserver to turn on and off this feature. tail_lines_list in webserver to show default entries in dropdown and finally num_lines in webserver to pick default number of lines to tail when user lands on get_logs page.

Relevant comments have been added. @ashb please review.

phanindhra876 · 2018-10-09T14:57:41Z

@msumit This isn't the equivalent of tail -f -- it doesn't stream the logs in as they happen, it's just tail -n 300 etc.

That's true sumit. This feature doesn't tail logs in fly mode. I am planning to add that in a separate PR as this brings a lot of changes to current log viewing mechanism and this maybe unstable and experimental.

airflow/bin/cli.py

Fokko · 2018-10-12T07:50:17Z

airflow/utils/log/file_task_handler.py

-            except Exception as e:
-                log = "*** Failed to load local log file: {}\n".format(location)
-                log += "*** {}\n".format(str(e))
+            if num_lines:


There is a lot of duplicate logic in this one with the tail_logs function.

Fokko · 2018-10-12T07:51:35Z

airflow/config_templates/default_airflow.cfg

+# Default number of lines to tail when page loads
+num_lines = 100
+# Entries to be shown in dropdown
+tail_lines_list = 50,100,200,500,1000


Remove these options, since they are also hardcoded in Javascript.

Fokko · 2018-10-12T07:51:59Z

airflow/config_templates/default_airflow.cfg

+# Flag to control tailing logs
+tail_logs = True
+# Default number of lines to tail when page loads
+num_lines = 100


Maybe up this a bit? A proper Java stacktrace can be bigger than 100 lines.

Fokko · 2018-10-12T07:52:23Z

airflow/bin/cli.py

+            logpath = "{logdir}/{filename}".format(logdir=logdir, filename=filename)
+            logsize = os.path.getsize(logpath)
+            if logsize >= 100 * 1024 * 1024:
+                p1 = subprocess.Popen(["tail", "-n " + str(num_lines), filename],


Do we really need a subprocess for this?

For bigger log files reading whole file and returning last few lines is impractical. Based on file size we will rendering by tail or file reading in python

phanindhra876 · 2018-10-28T09:56:27Z

Tests seems to be failing at places unrelated to my code changes. Do anyone has any idea what is causing and how to fix it?

Fokko · 2018-10-28T18:31:27Z

@phani8996 Can you rebase onto master? To make sure that you're up to date.

phanindhra876 · 2018-10-29T04:08:20Z

@phani8996 Can you rebase onto master? To make sure that you're up to date.

It is upto date with master.

ron819 · 2019-01-09T10:30:11Z

@phani8996 can you rebase?

…into tail_logs

phanindhra876 · 2019-01-23T09:38:16Z

@ashb Can you help out with travis. I don't understand why 2 test runs are failing.

ashb

Feedback based on reading the python code.

I haven't looked at the JS yet, and I'd like to look at it in a browser before we merge this.

airflow/utils/helpers.py

ashb · 2019-01-24T10:57:53Z

airflow/utils/helpers.py

+        if current_position == 0:
+            return None
+        fl.seek(-num_bytes, 1)
+        data = str(fl.read(num_bytes), 'utf-8', 'ignore')


Rather than doing this we should probably open the file in text mode in the first place

Opening file in text mode won't allow seeking wrt current position or end position. Text mode allows seeking only wrt file starting.

airflow/utils/helpers.py

ashb · 2019-01-24T11:00:51Z

airflow/utils/helpers.py

@@ -317,3 +317,45 @@ def render_log_filename(ti, try_number, filename_template):
                                    task_id=ti.task_id,
                                    execution_date=ti.execution_date.isoformat(),
                                    try_number=try_number)
+
+
+def tail_file(filepath, lines):


I would like to see a test of this function please, at least covering one happy/working case.

Added. Please check

airflow/utils/helpers.py

kurtqq · 2019-05-25T16:07:56Z

airflow/utils/helpers.py

@@ -334,7 +334,7 @@ def get_byte_range_file(fl, num_bytes):
        if current_position == 0:
            return None
        fl.seek(-num_bytes, os.SEEK_CUR)
-        data = str(fl.read(num_bytes), 'utf-8', 'ignore')
+        data = fl.read(num_bytes).decode('utf-8')


There is no need for Python2.7 support. It has been removed from the master branch

stale · 2019-09-03T18:42:37Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2019-11-20T16:52:20Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jeffkpayne reviewed Oct 7, 2018

View reviewed changes

airflow/bin/cli.py Outdated Show resolved Hide resolved

ashb reviewed Oct 9, 2018

View reviewed changes

ashb requested changes Oct 9, 2018

View reviewed changes

Fokko requested changes Oct 12, 2018

View reviewed changes

msumit mentioned this pull request Jan 4, 2019

[AIRFLOW-3623] Support download logs by attempts from UI #4425

Merged

6 tasks

phanindhra876 added 5 commits January 22, 2019 15:12

Tail logs using native functionality added

5d039c7

Merge branch 'master' of https://github.com/apache/incubator-airflow …

7d9a2f8

…into tail_logs

Enable tailing only for task log reader

f9aabff

Fix seeking issue with file when opened in read text mode

940e7b3

Merge branch 'master' of https://github.com/apache/airflow

d007246

phanindhra876 added 4 commits January 23, 2019 21:52

Merge branch 'master' of https://github.com/apache/airflow

a745ed0

Merge branch 'master' into AIRFLOW-620

68fe8cc

add pip517 flag in ci dockerfile

2e722ca

Fix logging path in remote serve logs

7d9c21e

ashb reviewed Jan 24, 2019

View reviewed changes

phanindhra876 added 4 commits January 24, 2019 22:25

Add unit tests for tail_file function

0373b4c

Merge branch 'master' of https://github.com/apache/airflow

aeb7031

Merge branch 'master' into AIRFLOW-620

b978b7c

add py27 compatibility to byte to string conversion

604fc87

dimberman added the stale-[4-2019] label Apr 17, 2019

kurtqq reviewed May 25, 2019

View reviewed changes

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 3, 2019

potiuk removed the stale-[4-2019] label Sep 7, 2019

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 7, 2019

kaxil force-pushed the master branch from 558f9f1 to d719e1f Compare October 2, 2019 15:31

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 20, 2019

stale bot closed this Nov 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-620] Feature to tail custom number of logs instead of rendering whole log #3992

[AIRFLOW-620] Feature to tail custom number of logs instead of rendering whole log #3992

phanindhra876 commented Oct 3, 2018

codecov-io commented Oct 3, 2018 •

edited

msumit commented Oct 8, 2018

phanindhra876 commented Oct 8, 2018 •

edited

ashb commented Oct 9, 2018

ashb Oct 9, 2018

ashb Oct 9, 2018

phanindhra876 Oct 9, 2018

ashb Oct 9, 2018

phanindhra876 Oct 9, 2018

phanindhra876 Oct 9, 2018

phanindhra876 commented Oct 9, 2018

Fokko Oct 12, 2018

Fokko Oct 12, 2018

Fokko Oct 12, 2018

Fokko Oct 12, 2018

phanindhra876 Oct 12, 2018

phanindhra876 commented Oct 28, 2018

Fokko commented Oct 28, 2018

phanindhra876 commented Oct 29, 2018

ron819 commented Jan 9, 2019

phanindhra876 commented Jan 23, 2019

ashb left a comment

ashb Jan 24, 2019

phanindhra876 Jan 24, 2019

ashb Jan 24, 2019

phanindhra876 Jan 24, 2019

kurtqq May 25, 2019

stale bot commented Sep 3, 2019

stale bot commented Nov 20, 2019

[AIRFLOW-620] Feature to tail custom number of logs instead of rendering whole log #3992

[AIRFLOW-620] Feature to tail custom number of logs instead of rendering whole log #3992

Conversation

phanindhra876 commented Oct 3, 2018

codecov-io commented Oct 3, 2018 • edited

Codecov Report

msumit commented Oct 8, 2018

phanindhra876 commented Oct 8, 2018 • edited

ashb commented Oct 9, 2018

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

phanindhra876 commented Oct 9, 2018

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

phanindhra876 commented Oct 28, 2018

Fokko commented Oct 28, 2018

phanindhra876 commented Oct 29, 2018

ron819 commented Jan 9, 2019

phanindhra876 commented Jan 23, 2019

ashb 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

stale bot commented Sep 3, 2019

stale bot commented Nov 20, 2019

codecov-io commented Oct 3, 2018 •

edited

phanindhra876 commented Oct 8, 2018 •

edited