-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Conversation
You should update the |
@potiuk Do you mean to update Should I set version to 2.6.0? I think that now it is ok (I updated |
Ah yeah. I copied wrong file. But you figured it out it seems :) |
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. Pending working out the hardest part - i.e. naming :D
I updated the name. |
If pre-commts failed, they will need to be fixed. You are extremely behind the main so you MUST rebase first |
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: 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. |
@potiuk Now pre-commit worked without any unsolvable problems (which I pushed here). |
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. |
Can you confirm that what pre-commit did is correct? |
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. |
(BTW. This is why "rebase early/rebase often" is always a good strategy. |
This reverts commit 8cec7c2.
@potiuk I think that now everything is working properly :) |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
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.