You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pankajastro (via GitHub)" <gi...@apache.org> on 2024/03/23 09:44:31 UTC
[PR] Make elastic search index_pattern more configurable [airflow]
pankajastro opened a new pull request, #38423:
URL: https://github.com/apache/airflow/pull/38423
closes: https://github.com/apache/airflow/issues/37999
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1552007608
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -372,12 +392,15 @@ def _format_msg(self, hit: Hit):
# Just a safe-guard to preserve backwards-compatibility
return hit.message
- def _es_read(self, log_id: str, offset: int | str) -> ElasticSearchResponse | None:
+ def _es_read(
+ self, log_id: str, offset: int | str, ti: TaskInstance | None
Review Comment:
```suggestion
self, log_id: str, offset: int | str, ti: TaskInstance
```
Don't we always have a ti?
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -153,6 +154,9 @@ def __init__(
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
Review Comment:
```suggestion
index_patterns: str = conf.get("elasticsearch", "index_patterns"),
```
Might help with typing checks - this can't actually be none.
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
+ """
+ Get index patterns by calling index_patterns_callable or None.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using %s index_patterns_callable", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
Review Comment:
We are no longer handling exceptions here?
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
+ """
+ Get index patterns by calling index_patterns_callable or None.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using %s index_patterns_callable", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
+ self.log.debug("Using %s index_patterns", self.index_patterns)
Review Comment:
If we keep this log, we should probably reword it.
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
Review Comment:
```suggestion
def _get_index_patterns(self, ti: TaskInstance | None) -> str:
```
Won't this always be a string now?
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
+ """
+ Get index patterns by calling index_patterns_callable or None.
Review Comment:
```suggestion
Get index patterns by calling index_patterns_callable, if provided, or the configured `index_patterns`.
```
Or similar.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1551866970
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -158,10 +158,19 @@ config:
index_patterns:
description: |
Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ The index_patterns_callable takes precedence over this.
version_added: 2.6.0
type: string
example: something-*
default: "_all"
+ index_patterns_callable:
+ description: |
+ A string representing the full path to the Python callable path which accept TI object and
+ return comma separated list of index patterns. This will takes precedence over index_patterns.
+ version_added: 2.9.0
Review Comment:
Those are just hold overs from when this config was in core. My immediate reaction is it should be changed to the version it was added into the provider, but I haven't given this problem much thought.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1552066975
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
+ """
+ Get index patterns by calling index_patterns_callable or None.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using %s index_patterns_callable", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
Review Comment:
I'm good either way. Happy to let it fail.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609710790
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +216,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str:
+ """
+ Get index patterns by calling index_patterns_callable, if provided, or the configured index_patterns.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using index_patterns_callable: %s", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
Review Comment:
Ohh missed it. I was looking for a code example. My bad.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609678726
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +216,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str:
+ """
+ Get index patterns by calling index_patterns_callable, if provided, or the configured index_patterns.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using index_patterns_callable: %s", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
Review Comment:
We have documented this in [provider.yaml](https://github.com/apache/airflow/pull/38423/files#diff-9c59fdec9c2afa5ff01b344180d154746a25ee13aa1cf4b748199ad51c998c7a) and docs will be generated for the same. Just TI needed at the moment
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
historically if a param comes from the config file then it is not added in doc string you can see `index_patterns`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609681260
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
Review Comment:
I removed this based on feedback https://github.com/apache/airflow/pull/38423#discussion_r1552014090 PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609628899
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +216,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str:
+ """
+ Get index patterns by calling index_patterns_callable, if provided, or the configured index_patterns.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using index_patterns_callable: %s", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
Review Comment:
We should document the signature of this function in docs.
Also, are there other variable that might be helpful in creating the index pattern for es? or is it just TI?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609617108
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
Shouldn't the type here be `typing.Callable` or this is the path to callable? if so maybe we can change the name of var?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609619659
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
Maybe we should add these params in the doc string.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609617108
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
Shouldn't the type here be `typing.Callable`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1552064440
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
Review Comment:
I think we should expect a string back. If they want to fall back to index_patterns config, they can do that on their own imo.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609681260
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
Review Comment:
This removed based on feedback https://github.com/apache/airflow/pull/38423#discussion_r1552014090 PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609730866
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +216,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str:
+ """
+ Get index patterns by calling index_patterns_callable, if provided, or the configured index_patterns.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using index_patterns_callable: %s", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
Review Comment:
I have updated it in commit a59826c0a444ddf2bb7c0af84de829c692e7a705
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1552034125
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
+ """
+ Get index patterns by calling index_patterns_callable or None.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using %s index_patterns_callable", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
Review Comment:
I think if `index_patterns_callable` is configured incorrectly then worth failing for better visibility or do want to just catch and log it instead of crashing?
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +218,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str | None:
Review Comment:
depend on what callable return. I'm assuming that the callable will return a string so this function will return a string either by calling index_patterns_callable or configured index_patterns. or do you think callable can return None too?
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -153,6 +154,9 @@ def __init__(
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
Review Comment:
do you mean to suggest for `index_patterns_callable` param? if not then if we remove fallback then it would be breaking change right?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609636322
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
Review Comment:
+1
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -388,16 +407,17 @@ def _es_read(self, log_id: str, offset: int | str) -> ElasticSearchResponse | No
}
}
+ index_patterns = self._get_index_patterns(ti)
Review Comment:
Is index_patterns not required as class attribute?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609684585
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -388,16 +407,17 @@ def _es_read(self, log_id: str, offset: int | str) -> ElasticSearchResponse | No
}
}
+ index_patterns = self._get_index_patterns(ti)
Review Comment:
No, it should not
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609533227
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -213,6 +216,21 @@ def format_url(host: str) -> str:
return host
+ def _get_index_patterns(self, ti: TaskInstance | None) -> str:
+ """
+ Get index patterns by calling index_patterns_callable, if provided, or the configured index_patterns.
+
+ :param ti: A TaskInstance object or None.
+ """
+ if self.index_patterns_callable:
+ self.log.debug("Using index_patterns_callable: %s", self.index_patterns_callable)
+ module_path, index_pattern_function = self.index_patterns_callable.rsplit(".", 1)
Review Comment:
We should use `import_string` from airflow.utils.module_loading` for better error handling
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1594433582
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -159,10 +159,19 @@ config:
index_patterns:
description: |
Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ The index_patterns_callable takes precedence over this.
version_added: 2.6.0
type: string
example: something-*
default: "_all"
+ index_patterns_callable:
+ description: |
+ A string representing the full path to the Python callable path which accept TI object and
+ return comma separated list of index patterns. This will takes precedence over index_patterns.
+ version_added: 5.4.0
Review Comment:
```suggestion
version_added: 5.5.0
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609617108
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
~~Shouldn't the type here be `typing.Callable` or this is the path to callable? if later maybe we can change the name of var?~~
Looks like they are path to callable.
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
We should add these params in the doc string.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609729670
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
If want to add I'll add all of them in a separate PR
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro merged PR #38423:
URL: https://github.com/apache/airflow/pull/38423
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609541430
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
Review Comment:
Do we need to remove the fallback?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1551795357
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -372,12 +397,15 @@ def _format_msg(self, hit: Hit):
# Just a safe-guard to preserve backwards-compatibility
return hit.message
- def _es_read(self, log_id: str, offset: int | str) -> ElasticSearchResponse | None:
+ def _es_read(
+ self, log_id: str, offset: int | str, ti: TaskInstance | None = None
Review Comment:
Since this is private, lets not let TI be optional.
```suggestion
self, log_id: str, offset: int | str, ti: TaskInstance | None
```
Can TI even be none?
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -158,10 +158,19 @@ config:
index_patterns:
description: |
Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ The index_patterns_callable takes precedence over this.
version_added: 2.6.0
type: string
example: something-*
default: "_all"
+ index_patterns_callable:
+ description: |
+ A string representing the full path to the Python callable path which accept TI object and
+ return comma separated list of index patterns. This will takes precedence over index_patterns.
+ version_added: 2.9.0
Review Comment:
Not core version. ES provider version. Probably 5.4?
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -388,16 +416,17 @@ def _es_read(self, log_id: str, offset: int | str) -> ElasticSearchResponse | No
}
}
+ index_patterns = _get_index_patterns(self.index_patterns_callable, ti) or self.index_patterns
Review Comment:
It might be a little nicer to have _get_index_patterns be a method instead so the fallback to index_patterns can happen there. Keeps the logic of callable vs original config in 1 spot.
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -114,6 +115,26 @@ def _ensure_ti(ti: TaskInstanceKey | TaskInstance, session) -> TaskInstance:
raise AirflowException(f"Could not find TaskInstance for {ti}")
+def _get_index_patterns(
+ index_patterns_callable: str | None = None, ti: TaskInstance | None = None
Review Comment:
```suggestion
index_patterns_callable: str | None, ti: TaskInstance
```
I don't think we need to make these optional args.
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -114,6 +115,26 @@ def _ensure_ti(ti: TaskInstanceKey | TaskInstance, session) -> TaskInstance:
raise AirflowException(f"Could not find TaskInstance for {ti}")
+def _get_index_patterns(
+ index_patterns_callable: str | None = None, ti: TaskInstance | None = None
+) -> str | None:
+ """
+ Get index patterns by calling index_patterns_callable or None.
+
+ :param index_patterns_callable: A string representing the full path to the callable itself.
+ :param ti: A TaskInstance object or None.
+ """
+ if not index_patterns_callable:
+ return None
+ try:
+ module_path, index_pattern_function = index_patterns_callable.rsplit(".", 1)
+ module = importlib.import_module(module_path)
+ index_pattern_callable_obj = getattr(module, index_pattern_function)
+ return index_pattern_callable_obj(ti)
+ except Exception:
+ return None
Review Comment:
Happy to return None here, but I don't think we should swallow the exception. If something does fail, it'll be very hard to determine why the expected index_patterns from the callable wasn't used.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1552066248
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -153,6 +154,9 @@ def __init__(
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
Review Comment:
No, for `index_patterns`. Fallback in this context is useless, as there is a default in the provider config. Not breaking to remove it here. Most fallback usage is useless these days.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1551811192
##########
airflow/providers/elasticsearch/provider.yaml:
##########
@@ -158,10 +158,19 @@ config:
index_patterns:
description: |
Comma separated list of index patterns to use when searching for logs (default: `_all`).
+ The index_patterns_callable takes precedence over this.
version_added: 2.6.0
type: string
example: something-*
default: "_all"
+ index_patterns_callable:
+ description: |
+ A string representing the full path to the Python callable path which accept TI object and
+ return comma separated list of index patterns. This will takes precedence over index_patterns.
+ version_added: 2.9.0
Review Comment:
I too think we should keep ES provider version here but I checked the release history and we do not have ES provider 2.6.0 but for config `index_patterns` we have version 2.6.0.
so if think we should update the version for `index_patterns` and others too in a separate PR?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609617108
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
Shouldn't the type here be `typing.Callable` or this is the path to callable? if later maybe we can change the name of var?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make elastic search index_pattern more configurable [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #38423:
URL: https://github.com/apache/airflow/pull/38423#discussion_r1609709279
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -152,7 +153,8 @@ def __init__(
offset_field: str = "offset",
host: str = "http://localhost:9200",
frontend: str = "localhost:5601",
- index_patterns: str | None = conf.get("elasticsearch", "index_patterns", fallback="_all"),
+ index_patterns: str = conf.get("elasticsearch", "index_patterns"),
+ index_patterns_callable: str = conf.get("elasticsearch", "index_patterns_callable", fallback=""),
Review Comment:
IMO, If it always comes from a config file, we can remove it as a param? Otherwise, devs can still use and create the class object passing it. I see no harm in adding it to docs.
But TBH I'm not sure what the norm is here :)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org