- Notifications
You must be signed in to change notification settings - Fork 513
[airflow] Make Airflow package GA #15287
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
Conversation
8f13395 to 48bf43c Compare | @alaudazzi PR is still in progress, but README can already be reviewed. Can you add reviewing the README file to your task list? |
|
alaudazzi 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.
Left a very minor suggestion, otherwise LGTM.
Co-authored-by: Arianna Laudazzi <46651782+alaudazzi@users.noreply.github.com>
5b98081 to ea096fb Compare | Fixed the CI and marked ready for review |
packages/airflow/manifest.yml Outdated
| conditions: | ||
| kibana: | ||
| version: "^8.13.0 || ^9.0.0" | ||
| version: "^8.17.0 || ^9.0.0" |
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.
Why the kibana version bump ?
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 used newer Kibanas to remake the dashboard from scratch, so I dropped those older ones. If the 8.13-8.16 needed, I can backport the dashboard (remake it in older kibana).
We also can only do bugfixes to beats 8.17 and newer, if something arises in future SDHs
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.
Do we need to support them?
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.
If there is no specific requirement to upgrade the version, we should be ok with keeping it as it is. So that any future bugs reach as many stacks as possible.
What do you mean by dashboard backport ? You can make dashboard in any stack version, just check that all panels work as it is in older stacks too.
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 made and exported the dashboard from 8.17, installing it in 8.13 fails
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.
Remade the dashboard in 8.13. Now it works on all ^8.13 || ^9.0 (verified)
packages/airflow/docs/README.md Outdated
| ## Data streams | ||
| ## Compatibility | ||
| | ||
| The Airflow package is tested with Airflow `2.4.0` and `3.0.6`. It should work with any `2.*` and `3.*` versions. |
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.
Hoping no backward compatibility breakages here between 2.0 and 3.0 ?
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.
no, I am using only metrics available in 2.0
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.
Are there new metrics added in 3.0 ?
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.
yes, there are few metrics added in some minor versions too
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.
Do we need to support them here ?
Since we are writing we support 3.x but are not supporting the newer metrics ?
This GA activity is also actually doing the service version upgrade activity as well :)
Which is good if we can address the question if we want to include the nwer metrics as well as we are clamining support for 3.x here.
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.
The was no 3.0 when the package was initially created. I remade the dashboard and also tested it with 3.0 too. So, the support means that nothing breaks.
So, is this a phrasing problem?
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 service version uograde when we say that we support a newer service version also, there are 2 aspects we check:
- Nothing existing breaks
- We support any new important metrics that were added between the 2 service versions . In this case 2.0, 3.0.
If we think there are newer metrics added and we are not taking care of that in this PR. We should keep support as 2.0 and add support for new metrics as a task under service version upgrade process.
cc: @lalit-satapathy
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.
if we add new metrics to the dashboard, then the panels using them will break if using older airflows
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.
@mykola-elastic : Since there are newer metrics added in 3.x and we are not taking care of them as part of GA. Lets keep compatibility for 2.x only.
We will move to 3.x as part of the service version upgrade, when we properly adhere to the process and handle the new metrics properly.
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.
updated readme: bcdeda8
ok?
| In dashboard, the DAG BagSize, DAG's on the y axis is the DAG count ? |
Yes, it is measured in DAGs. I didn't include the word "Count" as it is considered redundant in the new dashboard guidelines |
049f7d5 to bcdeda8 Compare | ## Data streams | ||
| ## Compatibility | ||
| | ||
| The Airflow package is tested with Airflow `2.4.0` and `3.0.6`. It will work with any `2.*` and `3.*` versions, but uses a smaller subset of metrics to be backwards-compatible with Apache Airflow 2.0. |
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.
Lets just mention compatibility with 2.x. Lets remove mention of 3.x completely from here.
Smaller subset line will be confusing for users.
So basically use what was existing before in compatibility.
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.
done, "The Airflow package is tested with Airflow 2.4.3. It should work with any 2.* version."
💚 Build Succeeded
History
|
| Package airflow - 1.0.0 containing this change is available at https://epr.elastic.co/package/airflow/1.0.0/ |







Work done in this PR so far (in progress):
1.0.0ecsversion to8.17.0[meta] Upgrade integrations to ECS 8.17 #11952undefinedon the integration configuration pageProposed commit message
See title.
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots