Skip to content
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

Split Celery logs into stdout/stderr #30485

Merged
merged 36 commits into from
Apr 27, 2023

Conversation

apilaskowski
Copy link
Contributor

Celery logs everything into Error stream, which is probably sufficient for most use-cases.
It might be more intuitive and more desired to have errors/criticals go to stderr, while low severity informations like info/warning go to stdout.

I have tested this solution, which I propose here with worker producing various severity levels of logs and they are correctly split into out and error streams.

I provided a solution which is backward compatible, so anyone that does not want to use it can just ignore it.

During pre-commit I discovered that config.yml needs to be updated and I am not sure how to perform that correctly.
We need to adjust config.yml for this to be complete, I wasn't sure if this should be made by hand or via some script.

@apilaskowski
Copy link
Contributor Author

Can I somehow request Reviewers, or are reviewers assigned automatically?
@kosteev @bjankie1

@potiuk
Copy link
Member

potiuk commented Apr 5, 2023

During pre-commit I discovered that config.yml needs to be updated and I am not sure how to perform that correctly. We need to adjust config.yml for this to be complete, I wasn't sure if this should be made by hand or via some script.

You should update the airflow/config_templates/config.yml.schema.json and let pre-commit do the job (then commit what's the result of pre-commit run).

@apilaskowski
Copy link
Contributor Author

apilaskowski commented Apr 5, 2023

@potiuk Do you mean to update airflow/config_templates/config.yml and then pre-commit will auto-generate default-airflow.cfg?

Should I set version to 2.6.0?

I think that now it is ok (I updated config.yml and default-airflow.cfg was automatic).

airflow/cli/commands/celery_command.py Outdated Show resolved Hide resolved
airflow/cli/commands/celery_command.py Outdated Show resolved Hide resolved
airflow/cli/commands/celery_command.py Outdated Show resolved Hide resolved
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
@potiuk
Copy link
Member

potiuk commented Apr 5, 2023

@potiuk Do you mean to update airflow/config_templates/config.yml and then pre-commit will auto-generate default-airflow.cfg?

Ah yeah. I copied wrong file. But you figured it out it seems :)

@apilaskowski
Copy link
Contributor Author

apilaskowski commented Apr 11, 2023

@potiuk @uranusjr @kosteev Do you think that we can merge this PR?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Pending working out the hardest part - i.e. naming :D

@apilaskowski
Copy link
Contributor Author

I updated the name.
I did had some pre-commit errors, but they were not related to the files that I changed.
@potiuk Do you think we can merge this now?

@potiuk
Copy link
Member

potiuk commented Apr 26, 2023

I updated the name. I did had some pre-commit errors, but they were not related to the files that I changed. @potiuk Do you think we can merge this now?

If pre-commts failed, they will need to be fixed. You are extremely behind the main so you MUST rebase first

@potiuk
Copy link
Member

potiuk commented Apr 26, 2023

First thing I do with every single PR of mine when I come back to it is to rebase it on top of the latest main and I heartily recommend making it into a habit to everyone:

This:

image

means that we absolutely should not merge that one because what runs in your CI pipeline now might fail badly when merged. Rebase early - rebase often is something I keep on repeating.

@apilaskowski
Copy link
Contributor Author

@potiuk
Is updating by merging (what I have done) here enough, or should I rebase by hand?

Now pre-commit worked without any unsolvable problems (which I pushed here).

@potiuk
Copy link
Member

potiuk commented Apr 26, 2023

No strict need to rebase (though I personally prefer it). If you merged and it looks good after conflict solving, it will work as welll (we follow "Squash and merge" workflow when merging, which means that all PRs are squashed into a single commit when merging, so it does not really matter if you rebased or merged your PR as long as it worked and conflicts were resolved.

@apilaskowski
Copy link
Contributor Author

Can you confirm that what pre-commit did is correct?
Because I am not sure if those files should be there or not.
They were added by pre-commit command. (_vendors dir).

@potiuk
Copy link
Member

potiuk commented Apr 26, 2023

No - it is not good. You should remove them - they were only added because the old (226 commits behind) pre-commit version had no exclusion for that folder.

@potiuk
Copy link
Member

potiuk commented Apr 26, 2023

(BTW. This is why "rebase early/rebase often" is always a good strategy.

@apilaskowski
Copy link
Contributor Author

@potiuk I think that now everything is working properly :)

@potiuk
Copy link
Member

potiuk commented Apr 26, 2023

@potiuk I think that now everything is working properly :)

And @uranusjr is around it seems for a quick check :)

@apilaskowski
Copy link
Contributor Author

@uranusjr @potiuk
Let me know if/when we are ready.
I will update the branch with a few new commits that occurred in a mean time (I don't want to use CI resources to much, or should I?).

@potiuk potiuk merged commit bc23dc5 into apache:main Apr 27, 2023
42 checks passed
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 27, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label May 8, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.0 milestone May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants