You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/03/05 16:40:56 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #14625: Support remote logging in elasticsearch with filebeat 7
jedcunningham opened a new pull request #14625:
URL: https://github.com/apache/airflow/pull/14625
closes: #13755
This adds support to choose different fields for `host` and `offset`. Filebeat 7 changed the defaults, and while one could probably move them back, making it configurable makes more sense.
https://www.elastic.co/guide/en/beats/libbeat/current/breaking-changes-7.0.html
`offset` -> `log.offset`
`host.name` -> `host.hostname` (And based on #13755, something also uses a str `host`, not sure what though)
This still needs test coverage.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#issuecomment-791633808
I think this is a very internal details and the user should not configure it explicitly. If possible, we should check which version of the service we are connecting to and act accordingly. When this is not possible, we recommend introducing a more descriptive configuration option, e.g. use_filebeat_7_headers or something similar.
Also, we should update the documentation and describe these configuration options.
http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow-providers-elasticsearch/latest/logging.html
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] TRManderson commented on pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
TRManderson commented on pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#issuecomment-819281508
Code LGTM too, seems like a nice minimal way to get exactly what's needed for filebeat 7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #14625:
URL: https://github.com/apache/airflow/pull/14625
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#issuecomment-791681180
If anything I think we may want to go the other way and make it more configurable as filebeats is just one way to get logs into elasticsearch. I feel like we should try and be agnostic especially since Airflow isn't directly involved with getting the logs there in the first place.
Thanks for the feedback though @mik-laj, I'll chew on this a bit more.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#issuecomment-858115010
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#discussion_r650031960
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -71,6 +72,8 @@ def __init__( # pylint: disable=too-many-arguments
write_stdout: bool,
json_format: bool,
json_fields: str,
+ host_field: str,
+ offset_field: str,
Review comment:
So, we have a bit of a chicken and egg problem here.
The config is going to be added in 2.1.1, but the ES logger is now in a provider, and that provider could be installed against Airflow 2.1.0.
So I think we have to do this
```suggestion
host_field: str = "host",
offset_field: str = "offset",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#discussion_r650039064
##########
File path: airflow/providers/elasticsearch/log/es_task_handler.py
##########
@@ -71,6 +72,8 @@ def __init__( # pylint: disable=too-many-arguments
write_stdout: bool,
json_format: bool,
json_fields: str,
+ host_field: str,
+ offset_field: str,
Review comment:
oh good call
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #14625: Support remote logging in elasticsearch with filebeat 7
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14625:
URL: https://github.com/apache/airflow/pull/14625#discussion_r648693279
##########
File path: airflow/config_templates/config.yml
##########
@@ -1999,6 +1999,20 @@
type: string
example: ~
default: "asctime, filename, lineno, levelname, message"
+ - name: host_field
+ description: |
+ The field where host name is stored (normally either `host` or `host.name`)
Review comment:
```suggestion
The field where host name is stored (normally either ``host`` or ``host.name``)
```
##########
File path: airflow/config_templates/config.yml
##########
@@ -1999,6 +1999,20 @@
type: string
example: ~
default: "asctime, filename, lineno, levelname, message"
+ - name: host_field
+ description: |
+ The field where host name is stored (normally either `host` or `host.name`)
+ version_added: 2.1.1
+ type: string
+ example: ~
+ default: "host"
+ - name: offset_field
+ description: |
+ The field where offset is stored (normally either `offset` or `log.offset`)
Review comment:
```suggestion
The field where offset is stored (normally either ``offset`` or ``log.offset``)
```
Since it is `.rst`
##########
File path: airflow/config_templates/default_airflow.cfg
##########
@@ -992,6 +992,12 @@ json_format = False
# Log fields to also attach to the json output, if enabled
json_fields = asctime, filename, lineno, levelname, message
+# The field where host name is stored (normally either `host` or `host.name`)
+host_field = host
+
+# The field where offset is stored (normally either `offset` or `log.offset`)
+offset_field = offset
Review comment:
```suggestion
# The field where host name is stored (normally either ``host`` or ``host.name``)
host_field = host
# The field where offset is stored (normally either ``offset`` or ``log.offset``)
offset_field = offset
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org