-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add override_logging argument to Worker (#634) #636
Add override_logging argument to Worker (#634) #636
Conversation
* Add override_logging argument to Worker * Add worker_override_logging to App * Update documentation * Update tests
@patkivikram, @wbarnha could you please approve it to run workflow? Thanks! |
PR blocked by #635 . |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #636 +/- ##
=======================================
Coverage 94.06% 94.06%
=======================================
Files 102 102
Lines 11088 11091 +3
Branches 1547 1548 +1
=======================================
+ Hits 10430 10433 +3
Misses 558 558
Partials 100 100 ☔ View full report in Codecov by Sentry. |
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 updating the tests as well! LGTM!
@wbarnha - this has seemingly broken logging for me. Was working fine on 0.11.1 and now after this change I get no logging :/ |
I feared this would be the case. Looks like I missed this during testing. Does setting |
@wbarnha - I haven't set up any form of custom logging. I'm using the defaults with no overrides and I invoke the worker as follows:
I get the exact reverse behavior of the OP (what they were seeing in their linked issue I am now seeing after this change) |
Well, that's a shame... I'll try to find a happy medium. |
@davidvanwyk could you confirm, that if you set |
@lpogo - unfortunately not. I still get no logs other than the initial stdout with the Faust version etc. |
Just to confirm - the setting you enabled on the application instance is: |
Yes, you are right. |
Well take a look and see if you can resolve this in the near term and @wbarnha can just roll forward, otherwise I think we need to roll this one back until it's been figured out? :) If I have some time in the coming days I can check it out as well but not 100% sure I will have the time. |
I agree, I'm a bit confused on why this feature was necessary to begin with but I merged anyway since I figured it'd be harmless. We'll roll it back. |
Does logging work on your end if you revert changes done in #368 locally in your environment? I'm not exactly sure what's creating the logging error here, so a reproducible setup would be greatly appreciated. |
This one hit me as well. Seems like now logging doesn't work. Rolling back to previous version. |
Reverted changes in |
I acknowledge the issue you're facing, we'll need to solve this some other way. |
Description
Allow user to decide if override_logging shoud be enabled or not.
It is forced by faust.Worker to be enabled right now.
It resolve Issue #634