[AIRFLOW-1325][WIP]Airflow streaming log backed by ElasticSearch by allisonwang · Pull Request #2469 · apache/airflow

allisonwang · 2017-07-21T22:22:10Z

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Airflow-1325 issues and references them in the PR title.

Description

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

Please refer to Airflow-1325 for more details. Highlights of this change include:

Enable ElasticSearch as Airflow logging backend
Real-time auto-tailing streaming log
Log syntax highlighting with Highlight.js and webworker (this will be included in the subsequent PR but not this one)

This PR will solve the problem of loading big log files that freeze the browser. Instead of loading entire log files when the page renders, webserver will read and render logs by batch.
Note this PR develop on top of changes in #2464.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
- Unit test for helper functions
- Test on local environment with docker ElasticSearch, Logstash and Filebeat pipeline (v5.4.0)
- Test on production environment with ElasticSearch and Logstash cluster

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
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"

aoen · 2017-07-21T23:23:00Z

Nit: grammar
"Logging backend URL"
is fine

Would be good to explain a bit more too, e.g. that this is used by the webserver to fetch logs.

allisonwang · 2017-07-21T23:48:00Z

Need to modify CI to exclude license checking for two static resource files.

aoen

Make webworkers highlight only newly loaded logs instead of entire HTML tag.

This seems concerning if we have to reprocess the whole tag every time, wouldn't this be very slow for large logs (e.g. 1 gig)? I think it would be good to be able to toggle color via config flag regardless though.

aoen · 2017-07-21T23:24:43Z

(read this after reading the comments below) Can we encapsulate this file with a class? It should also have a descriptive top-level comment, since there is that confusion we discussed of what is used to write to logs vs reading them.

aoen · 2017-07-21T23:30:37Z

Would be good to make this comment a bit more descriptive, it's not immediately clear what it's used for (e.g. that this is what users specify in the configuration file).

Ultimately those will be removed and instantiation will be moved into logging configuration as a handler.

aoen · 2017-07-21T23:31:55Z

Does this have to be global? Can we cache at the webserver instance level instead?

aoen · 2017-07-21T23:35:02Z

won't LOGGING_BACKEND be undefined if it was not previously cached and the logging backend URL is not defined in the configuration? Would be good to make it explicitly return None if this is what is intended in this case. Would also be good to tests for this.

aoen · 2017-07-21T23:37:43Z

It seems like this method can be combined with get_logging_backend (i.e. get_logging_backend is called directly instead of cached_logging_backend, and then fetches the cached version if it is available).

_get_logging_backend is a helper function forcached_logging_backend. It follows the pattern of cached_app in Airflow : https://github.com/apache/incubator-airflow/blob/master/airflow/www/app.py#L158

aoen · 2017-07-21T23:39:13Z

s/schema/logging_backend or logging_backend_class

aoen · 2017-07-21T23:40:53Z

AirflowException instead of TypeError

aoen · 2017-07-21T23:47:08Z

Let's make this configurable.

will make everything configurable in v2!

Should be max lines right?

MAX_LINES_PER_PAGE

aoen · 2017-07-21T23:53:26Z

Can we elaborate on the TODO? Also why critical level? Shouldn't it be debug level (which would actually hide it more aggressively than critical)

ES's logs will be added to task instance logs and display in Airflow UI.

aoen · 2017-07-22T00:00:12Z

Did we check license compatibility with apache for all of the third party files? This one seems to be fine (MIT).
http://apache.org/legal/resolved.html#category-a

I checked both open source files. Highlight.js is under BSD 3 which should be ok based on the Apache link above.

aoen · 2017-07-22T00:06:49Z

Can we pass parameters explicitly here?

aoen · 2017-07-22T00:07:37Z

Might be good to keep this consistent with the UI (1-indexed) to avoid confusion. When passing page later you can subtract 1 if necessary.

aoen · 2017-07-22T00:10:56Z

Can we pass parameters explicitly?

aoen · 2017-07-22T00:26:52Z

The delay seems like it should be configurable since it puts stress on the backend.

bolkedebruin · 2017-07-22T10:47:17Z

Folks, can you please explain the relationship with #2464 ? To me this seems to create technical debt upon technical debt.

What I would expect is something like:

[AIRFLOW-1385] Make Airflow task logging configurable #2464 merged
Abstraction for streaming logging (is that required at all?)
Abstraction for streaming reading
ElasticsearchHandler

This PR builds on https://issues.apache.org/jira/browse/AIRFLOW-1332, which it shouldn't. It was deemed not the right approach and only there for transition. This builds upon the technical implementation of the transitional code without a clear path to #2464 .

cc: @mistercrunch @criccomini

aoen · 2017-07-24T18:50:19Z

What's the error handling around Elastic search failures (e.g. if the worker didn't actually log the logs the webserver expects in elastic search)?

aoen · 2017-07-24T21:57:53Z

@bolkedebruin Thinking about this specific PR a bit more I think you are right, this amount of tech debt is not acceptable and doesn't really fix any tech debt like the other PR was (adding retries to log path). Having the tech debt from only the Elastic Search logging backend is OK, but if others use the abstraction then it will create considerable amount of work later, and it's hard to justify why Elastic Search should be a special case. We have decided to fork internally and once we finish testing/releasing the fork we will try to iterate on the changes in open source (the logging abstraction first, and this PR second). The issue is Allison only has ~2.5 weeks left and I think it will be very tight if we try to merge the logging abstraction. Let's leave/close this PR for now and hopefully Allison or myself will revisit it and the logging abstraction PR shortly.

codecov-io · 2017-07-25T01:22:53Z

Codecov Report

Merging #2469 into master will decrease coverage by 0.28%.
The diff coverage is 35.22%.

@@            Coverage Diff             @@
##           master    #2469      +/-   ##
==========================================
- Coverage   69.43%   69.14%   -0.29%     
==========================================
  Files         146      150       +4     
  Lines       11351    11439      +88     
==========================================
+ Hits         7881     7909      +28     
- Misses       3470     3530      +60

Impacted Files	Coverage Δ
.../logging_backends/elasticsearch_logging_backend.py	`0% <0%> (ø)`
airflow/www/views.py	`66.39% <21.05%> (-0.61%)`	⬇️
airflow/logging_backends/base_logging_backend.py	`60% <60%> (ø)`
airflow/logging_backend.py	`61.53% <61.53%> (ø)`
airflow/utils/imports.py	`88.88% <88.88%> (ø)`
airflow/jobs.py	`76.73% <0%> (-0.39%)`	⬇️
... and 1 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 426b6a6...4cba481. Read the comment docs.

bolkedebruin · 2017-07-25T05:56:28Z

@aoen Gotcha. That makes sense. I don't mind spending time on logging especially with the work @allisonwang has already done, but not before the summer holidays probably. I do think we are pretty close to merging, because although it might not be perfect yet, it will give us the right starting point. Then we can start pushing ppl towards using it.

FWIW: I understand what you guys want to accomplish here, but imho you are tying too much of Elasticsearch into Airflow. That includes the try_number. If we would properly track start_dates in a task_instance_log table you could use a standard EFK/ELK stack and still be able to split on try_number in the user interface. This would reduce the complexity of implementing logging (and being compatible with everything else) while still reaching the same goals.

aoen · 2017-07-25T18:53:50Z

If I understood your point about try number correctly (i.e. you don't like the idea of adding it to the task_instance table), then I really, really feel try number is critical to Airflow regardless (we were talking about it with Alex/Max internally + having a separate table that stores all task instance tries append-only).

Here is the expert from my mailing list email in case you missed it.


The concept of "attempt" is needed regardless of logging, the way retries are stored/handled right now is not very sane.
- Old TI state is permanently deleted
- In the task logs you get "Try 1/6"... 2/6... 3/6... 1/6... 2/6 in logs which doesn't make sense (if it's the Nth time the task is running it should be logged as the Nth time). I recall other strange behaviors in these log lines too (maybe something like Try 4/3).
- The "primary key" for a task instance run is not complete (which is what Allison's logging change needs), you could say that TaskInstance should only keep track of the latest TaskInstance run, but we still want to store all tries for a task instance somewhere in the database, and we still need to key this off of "attempt".

I don't like start_dates, there aren't really guarantees around them being completely unique (which try_number gives us for free), they are a poor approximation for a unique key. Feel free to reply to the open source thread and we can discuss along with Max/Alex/other committers though.

bolkedebruin · 2017-07-25T20:39:18Z

Hi @aoen my main issue is that this information should not be stored in the task_instance table but rather in a task_instance_log table. Which is your append only table. My suggestion of a primary key on 'task_id, dag_id, execution_date, start_date' is the same as having a try_number in this table which you should be storing anyway.

So attempt/try_number yes, but not how it is done now. Storing it in the task_instance table doesn't make logically sense. If you want to store the count of attempt/tries it should be named total_tries/total_attempts if you don't want to do aggregation or a max(start_date)

aoen · 2017-07-25T21:42:52Z

Ah I see, I think I agree (need to see the complete new schema to be sure), but that's a little bit more involved. We should be able to easily migrate from the current version to this new one, we have still made forward progress (most of Allison's logic will stay).

saguziel · 2017-07-25T23:19:45Z

Nit is that for a one line comment you can put the triple quotes on the same line

saguziel · 2017-07-25T23:32:13Z

may as well use dag_id=None, task_id=None... instead of kwargs

saguziel · 2017-07-25T23:34:44Z

I didn't do a deep read of this, but it doesn't stack overflow right?

Ideally, front-end should stop fetch logs from the back-end when tasks complete. But there will be latencies between log get flushed into disk and logs get indexed in ES. We will need a smarter way to tell frontend when to stop fetching logs.

I'm just curious about whether this page will eventually crash

saguziel · 2017-07-25T23:35:07Z

Maybe we should move to the APIs

Make sense! Those endpoints should be moved to API in the future.

saguziel · 2017-07-25T23:35:31Z

Does this return an error message to the client?

saguziel · 2017-07-25T23:36:20Z

This probably isn't the right place to put this comment, but we have some type of escaping somewhere, right?

You mean escaping characters in the returned message, which might cause decode and rendering issues?

Yeah. Like a <Script>alert</script> shouldnt work

saguziel · 2017-07-25T23:38:01Z

I think we need some basic tests for these endpoints too

Indeed, it would be really nice if we have some sort of unit test written for front-end and Flask side.

aoen reviewed Jul 21, 2017

View reviewed changes

allisonwang mentioned this pull request Jul 21, 2017

[AIRFLOW-1325] Use Elasticsearch as logging backend #2380

Closed

4 tasks

aoen reviewed Jul 22, 2017

View reviewed changes

aoen reviewed Jul 24, 2017

View reviewed changes

saguziel reviewed Jul 25, 2017

View reviewed changes

Comment thread airflow/www/views.py Outdated

saguziel Jul 25, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Does this return an error message to the client?

saguziel reviewed Jul 25, 2017

View reviewed changes

[AIRFLOW-1325] airflow streaming log backed by Elasticsearch

4cba481

allisonwang force-pushed the allison--streaming branch from 99dc77a to 4cba481 Compare July 26, 2017 22:38

allisonwang closed this Aug 10, 2017

allisonwang deleted the allison--streaming branch August 10, 2017 18:29

shahar1 mentioned this pull request Jul 10, 2026

Bump flask-appbuilder to 5.2.2 #69706

Closed

1 task

Uh oh!

Conversation

allisonwang commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Uh oh!

aoen Jul 21, 2017 • 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!

allisonwang commented Jul 21, 2017

Uh oh!

aoen left a comment

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!

aoen Jul 21, 2017 • 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!

allisonwang Jul 24, 2017 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

bolkedebruin commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aoen commented Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bolkedebruin commented Jul 25, 2017

Uh oh!

allisonwang commented Jul 21, 2017 •

edited

Loading

aoen Jul 21, 2017 •

edited

Loading

aoen Jul 21, 2017 •

edited

Loading

allisonwang Jul 24, 2017 •

edited

Loading

bolkedebruin commented Jul 22, 2017 •

edited

Loading

aoen commented Jul 24, 2017 •

edited

Loading

codecov-io commented Jul 25, 2017 •

edited

Loading