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

pingzh · 2019-01-03T01:50:53Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3623
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.

Description

Here are some details about my PR, including screenshots of any UI changes:

Support download logs by attempts from UI

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

Passes flake8

@KevinYang21

codecov-io · 2019-01-03T02:42:45Z

Codecov Report

Merging #4425 into master will decrease coverage by 0.01%.
The diff coverage is 81.08%.

@@            Coverage Diff             @@
##           master    #4425      +/-   ##
==========================================
- Coverage   78.59%   78.58%   -0.02%     
==========================================
  Files         204      204              
  Lines       16453    16482      +29     
==========================================
+ Hits        12932    12953      +21     
- Misses       3521     3529       +8

Impacted Files	Coverage Δ
airflow/www_rbac/views.py	`73.5% <66.66%> (-0.16%)`	⬇️
airflow/utils/helpers.py	`82.75% <85.71%> (+0.14%)`	⬆️
airflow/www/views.py	`69.82% <93.33%> (+0.14%)`	⬆️
airflow/models/__init__.py	`92.55% <0%> (-0.05%)`	⬇️

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 8d5ffef...55a211c. Read the comment docs.

feng-tao · 2019-01-03T07:37:50Z

Does the feature work when the log is in a remote location(like some s3 directory)?

pingzh · 2019-01-03T18:00:55Z

Does the feature work when the log is in a remote location(like some s3 directory)?

Hi @feng-tao , that is a great question. It should work since my changes do not involve any logger handlers.

pingzh · 2019-01-03T23:31:19Z

Does the feature work when the log is in a remote location(like some s3 directory)?

Hi @feng-tao I have tested it against the elasticsearch log handler. It works fine.

feng-tao

thanks. Just left some small comments.

feng-tao · 2019-01-03T23:35:38Z

airflow/utils/helpers.py

@@ -297,3 +297,16 @@ def parse_template_string(template_string):
        return None, Template(template_string)
    else:
        return template_string, None
+
+
+def render_log_filename(ti, try_number, filename_template):


nit: please provides a docstring for the function.

feng-tao · 2019-01-03T23:39:36Z

airflow/www/templates/airflow/dag.html

@@ -320,6 +326,29 @@ <h4 class="modal-title" id="dagModalLabel">
            $("#div_btn_subdag").show();
            subdag_id = "{{ dag.dag_id }}."+t;
        }
+
+      $("#try_index > li").remove()


nit: semicolon in the end?

feng-tao · 2019-01-03T23:39:45Z

airflow/www/templates/airflow/dag.html

@@ -320,6 +326,29 @@ <h4 class="modal-title" id="dagModalLabel">
            $("#div_btn_subdag").show();
            subdag_id = "{{ dag.dag_id }}."+t;
        }
+
+      $("#try_index > li").remove()
+      var startIndex = (try_numbers > 2 ? 0 : 1)


nit: semicolon in the end?

feng-tao · 2019-01-03T23:39:52Z

airflow/www/templates/airflow/dag.html

+          "&task_id=" + encodeURIComponent(task_id) +
+          "&execution_date=" + encodeURIComponent(execution_date) +
+          "&metadata=null" +
+          "&format=file"


nit: semicolon in the end?

feng-tao · 2019-01-03T23:41:15Z

airflow/www/views.py

@@ -770,7 +774,12 @@ def get_logs_with_metadata(self, session=None):
        task_id = request.args.get('task_id')
        execution_date = request.args.get('execution_date')
        dttm = pendulum.parse(execution_date)
-        try_number = int(request.args.get('try_number'))
+        if request.args.get('try_number', None) is not None:


small nit: request.args.get('try_number', None) could be request.args.get('try_number') as the default should be None

feng-tao · 2019-01-03T23:42:54Z

airflow/www_rbac/templates/airflow/dag.html

+          subdag_id = "{{ dag.dag_id }}."+t;
+      }
+
+      $("#try_index > li").remove()


same for the semicolon

pingzh · 2019-01-04T00:44:56Z

Thanks for your feedback, @feng-tao. I have addressed them.

feng-tao · 2019-01-04T04:40:17Z

thanks @pingzh

msumit · 2019-01-04T05:29:09Z

@pingzh @feng-tao sorry for late comment, but won't it be better if we provide download logs feature at the "View Logs" page itself, instead of from the model dialog? As IMO people would anyway want to view logs first and then take the decision of downloading it?

pingzh · 2019-01-04T05:33:20Z

@msumit great point. One big reason for this feature is for large log files, it is expensive to pull the large logs and render in the web UI.

msumit · 2019-01-04T06:30:09Z

@pingzh totally agree with the problem of large logs, but I think that's a diff problem to solve, and can be solved easily, for example, see this #3992.

feng-tao · 2019-01-04T06:47:13Z

@msumit , I think your point is valid, but sometimes the log file is very hard to render and view if the file is too large(not sure of the status or the implementation detail for #3992). The log file is very critical for debugging production issue on why the task not running. And adding this action provides to task instance UI panel(all the operations for task instance) provides a way to download the file based on different try number. Hence I think this pr provides value on solving this issue.

pingzh · 2019-01-04T06:55:43Z

@msumit Thanks for the info. In my mind, for large log files, tailing or range lines does not provide lots of insights when trying to find something. And it looks like #3992 only supports file log handler at least for now.

It is also a good idea to add a download btn after users are in the "View Logs" page.

kaxil · 2019-01-05T16:48:22Z

Can you please resolve an error that was in the PR but because of broken Travis it wasn't detected:

======================================================================
46) ERROR: test_get_logs_with_metadata_as_download_file (tests.www_rbac.test_views.TestLogView)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/www_rbac/test_views.py line 569 in test_get_logs_with_metadata_as_download_file
      response = self.app.get(url)
   AttributeError: 'Flask' object has no attribute 'get'

Please fix it in the new PR.

pingzh · 2019-01-06T04:58:53Z

@kaxil sorry about that. Here is the PR to fix it: #4446

Can you please resolve an error that was in the PR but because of broken Travis it wasn't detected:

======================================================================
46) ERROR: test_get_logs_with_metadata_as_download_file (tests.www_rbac.test_views.TestLogView)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/www_rbac/test_views.py line 569 in test_get_logs_with_metadata_as_download_file
      response = self.app.get(url)
   AttributeError: 'Flask' object has no attribute 'get'

Please fix it in the new PR.

feng-tao reviewed Jan 4, 2019

View reviewed changes

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

55a211c

feng-tao merged commit 6f616f0 into apache:master Jan 4, 2019

pingzh deleted the download-log-based-on-master branch January 4, 2019 04:41

pingzh mentioned this pull request Jan 6, 2019

[AIRFLOW-3636] Fix a test introduced in #4425 #4446

Merged

6 tasks

feng-tao pushed a commit that referenced this pull request Jan 6, 2019

[AIRFLOW-3636] Fix a test introduced in #4425 (#4446)

9021b42

ashb pushed a commit that referenced this pull request Feb 21, 2019

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

6f360ab

ashb pushed a commit that referenced this pull request Feb 21, 2019

[AIRFLOW-3636] Fix a test introduced in #4425 (#4446)

6adad35

ashb pushed a commit to ashb/airflow that referenced this pull request Mar 19, 2019

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

4579a00

ashb pushed a commit to ashb/airflow that referenced this pull request Mar 19, 2019

[AIRFLOW-3636] Fix a test introduced in apache#4425 (apache#4446)

dc8385b

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

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

9198a3e

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-3636] Fix a test introduced in apache#4425 (apache#4446)

8367ae7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pingzh commented Jan 3, 2019 •

edited

Loading

codecov-io commented Jan 3, 2019 •

edited

Loading

feng-tao commented Jan 3, 2019 •

edited

Loading

pingzh commented Jan 3, 2019

pingzh commented Jan 3, 2019

feng-tao left a comment

feng-tao Jan 3, 2019

pingzh Jan 4, 2019

feng-tao Jan 3, 2019

pingzh Jan 4, 2019

feng-tao Jan 3, 2019

pingzh Jan 4, 2019

feng-tao Jan 3, 2019

pingzh Jan 4, 2019

feng-tao Jan 3, 2019

pingzh Jan 4, 2019

feng-tao Jan 3, 2019

pingzh Jan 4, 2019

pingzh commented Jan 4, 2019

feng-tao commented Jan 4, 2019

msumit commented Jan 4, 2019

pingzh commented Jan 4, 2019

msumit commented Jan 4, 2019

feng-tao commented Jan 4, 2019

pingzh commented Jan 4, 2019

kaxil commented Jan 5, 2019

pingzh commented Jan 6, 2019

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

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

Conversation

pingzh commented Jan 3, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

codecov-io commented Jan 3, 2019 • edited Loading

Codecov Report

feng-tao commented Jan 3, 2019 • edited Loading

pingzh commented Jan 3, 2019

pingzh commented Jan 3, 2019

feng-tao 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

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

Choose a reason for hiding this comment

pingzh commented Jan 4, 2019

feng-tao commented Jan 4, 2019

msumit commented Jan 4, 2019

pingzh commented Jan 4, 2019

msumit commented Jan 4, 2019

feng-tao commented Jan 4, 2019

pingzh commented Jan 4, 2019

kaxil commented Jan 5, 2019

pingzh commented Jan 6, 2019

pingzh commented Jan 3, 2019 •

edited

Loading

codecov-io commented Jan 3, 2019 •

edited

Loading

feng-tao commented Jan 3, 2019 •

edited

Loading