-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Move the traces and metrics code under a common observability package #56187
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
jason810496
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.
Nice! Thanks for the cleanup!
LGTM for the Airflow-Core and Providers change, but I'm not sure about TaskSDK part.
I will leave it to someone more familiar to answer.
ferruzzi
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.
Approved pending Ash weighing in on the SDK part.
|
@jason810496 @ferruzzi Thank you for the review! |
2ae0132 to
bf6b976
Compare
|
@jason810496 @ferruzzi @ashb It's ready for review. The CI is green. The step for running the task sdk integration tests seems to be flaky. https://github.com/apache/airflow/actions/runs/18505099702/job/52734949908?pr=56187 I ran it locally and it worked, so I triggered the CI again and it passed. To help you understand the changes faster
|
jason810496
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.
Thanks for the update! The overall change LGMT, but there are still question about the dependencies for Airflow-Core and TaskSDK.
jscheffl
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.
Wow - this is also a big one!
Also having two sanity questions but no general objections after good review by others. Would be important to clarify minor open points and then LGTM!
#protm
amoghrajesh
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.
This PR is an absolute delight. Thanks for collaborating with myself and @potiuk on this one and being our first real developer who wasn't involved in designing the shared module approach for client server separation. This helped us identify gaps in the process and fix them as needed.
The changes look good to me minus the comments I sent you offline regarding the task sdk docs, preemptively approving now assuming those changes will be made prior to merge and the last few comments I have will be handled.
This PR will go down as a PR of the month candidate for me.
#protm
|
@xBis7 the failure on your PR: is unrelated and is due to a fastAPI bump it seems. |
b317f91 to
cd0a464
Compare
@amoghrajesh I see, it's on main as well. |
|
The fix is merged, rebasing it. |
@amoghrajesh I was about to do it. Thanks. |
|
@amoghrajesh @potiuk Both failures seem random. I ran the redis step locally and it worked. |
|
Just rebased again, hopefully its green and ready to merge! |
|
@amoghrajesh @potiuk Thank you for all the help! @jason810496 @ferruzzi @jscheffl Thank you for the reviews! |
|
Wooohooo |
|
#protm |
…apache#56187) - Create shared/observability with base metrics and traces implementations - Add task-sdk observability module with own Stats and Trace wrappers - task SDK no longer imports from airflow-core observability - Each component reads its own configuration - Export only Trace in public API (Stats is internal) --------- Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
|
Well done, and thanks for sticking with it. That was a long trek, but it turned out great. |
|
@ferruzzi Thank you! |
This patch is refactoring the
metricsand thetracespackages and moves them under a common package namedobservability.This is done so that it will be easier to add common files in the future.
The need for these changes came from this discussion
#56150 (comment)
^ 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 airflow-core/newsfragments.