-
Notifications
You must be signed in to change notification settings - Fork 16.4k
feat(api/endpoint): allow filtering eventlogs by attributes #34417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api/endpoint): allow filtering eventlogs by attributes #34417
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
6bd57a7 to
b6a6c67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we use strictly less then before in this type of queries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest commit. Thanks for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already a dag id param:
airflow/airflow/api_connexion/openapi/v1.yaml
Lines 4797 to 4803 in 6c649ae
| DAGID: | |
| in: path | |
| name: dag_id | |
| schema: | |
| type: string | |
| required: true | |
| description: The DAG ID. |
so I don't know if we should use it or create a new one, @pierrejeambrun wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should re-use it if this is exatly the same thing. No need to duplicate I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this dag_id parameter works for path and not as query parameter. In addition, it's set as required while eventlogs's filtering parameters aren't strictly required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, maybe we can prefix that with Filter and put them in the Filter section where other filters reside in the spec. Also maybe a generic name such as FilterDAGID so we can easily reuse it. (shouldn't be specific to EventLog I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameters are now converted to filters in latest commit. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for task id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I've replied in above review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try to use different values? (ex event_1, dag_1, task_1, ...) instead of having the same value for all the variables, without that we cannot ensure that the filter is really working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest commit. Thanks for the review!
5aa520e to
4af14ac
Compare
4af14ac to
b3d1b8a
Compare
|
hello there, can someone help reviewing and merge if deemed sufficient? Thanks! |
bbovenzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Closes: #34210
feat(api/endpoint): Allow filtering for Event Logs at
/eventlogsendpoint using the entity's attributes.Tests added and API schema updated
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.