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

Add override_logging argument to Worker (#634) #636

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Add override_logging argument to Worker (#634) #636

merged 3 commits into from
Jul 18, 2024

Conversation

lpogo
Copy link
Contributor

@lpogo lpogo commented Jul 17, 2024

  • Add override_logging argument to Worker
  • Add worker_override_logging to App
  • Update documentation
  • Update tests

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

* Add override_logging argument to Worker

* Add worker_override_logging to App

* Update documentation

* Update tests
@lpogo
Copy link
Contributor Author

lpogo commented Jul 17, 2024

@patkivikram, @wbarnha could you please approve it to run workflow? Thanks!

@lpogo
Copy link
Contributor Author

lpogo commented Jul 17, 2024

PR blocked by #635 .
Required fixes are implemented there.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.06%. Comparing base (e5007cc) to head (d31f796).

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@wbarnha wbarnha left a 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 wbarnha merged commit 1afa214 into faust-streaming:master Jul 18, 2024
19 of 23 checks passed
@lpogo lpogo deleted the allow_worker_override_logging branch July 18, 2024 04:01
@davidvanwyk
Copy link

@wbarnha - this has seemingly broken logging for me. Was working fine on 0.11.1 and now after this change I get no logging :/

@wbarnha
Copy link
Member

wbarnha commented Aug 15, 2024

I feared this would be the case. Looks like I missed this during testing. Does setting worker_override_logging=False in your settings fix your issue? I was contemplating setting that as the default. Could you show me your logging configuration?

@davidvanwyk
Copy link

@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:

python -m my_app worker -l info

I get the exact reverse behavior of the OP (what they were seeing in their linked issue I am now seeing after this change)

@wbarnha
Copy link
Member

wbarnha commented Aug 15, 2024

Well, that's a shame... I'll try to find a happy medium.

@lpogo
Copy link
Contributor Author

lpogo commented Aug 19, 2024

@davidvanwyk could you confirm, that if you set override_logging=True in your my_app application, then logging is working fine again?
If yes, I think we should use True instead of False as default value. mode.Worker is using True as default, so it may be my bad I set it to False.
@wbarnha what do you think?

@davidvanwyk
Copy link

@lpogo - unfortunately not. I still get no logs other than the initial stdout with the Faust version etc.

@davidvanwyk
Copy link

Just to confirm - the setting you enabled on the application instance is: worker_override_logging. I think override_logging was what it was named in the context of CLI option?

@lpogo
Copy link
Contributor Author

lpogo commented Aug 19, 2024

Just to confirm - the setting you enabled on the application instance is: worker_override_logging. I think override_logging was what it was named in the context of CLI option?

Yes, you are right.

@davidvanwyk
Copy link

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.

@wbarnha
Copy link
Member

wbarnha commented Aug 19, 2024

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.

@lpogo
Copy link
Contributor Author

lpogo commented Aug 20, 2024

Lets do it this way. This change mustn't impact current users.
@wbarnha I found that it doesn't matter if worker_override_logging is set to True - faust.Worker is always receiving False. No idea why it is happening now..
Anyway for me logging issue started after #368 . Please take a look there.

@wbarnha
Copy link
Member

wbarnha commented Aug 20, 2024

Lets do it this way. This change mustn't impact current users. @wbarnha I found that it doesn't matter if worker_override_logging is set to True - faust.Worker is always receiving False. No idea why it is happening now.. Anyway for me logging issue started after #368 . Please take a look there.

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.

@lesmanacflt
Copy link

This one hit me as well. Seems like now logging doesn't work. Rolling back to previous version.

wbarnha added a commit that referenced this pull request Aug 23, 2024
@wbarnha
Copy link
Member

wbarnha commented Aug 23, 2024

Reverted changes in faust-streaming==0.11.3, thanks for your patience!

@wbarnha
Copy link
Member

wbarnha commented Aug 23, 2024

Lets do it this way. This change mustn't impact current users. @wbarnha I found that it doesn't matter if worker_override_logging is set to True - faust.Worker is always receiving False. No idea why it is happening now.. Anyway for me logging issue started after #368 . Please take a look there.

I acknowledge the issue you're facing, we'll need to solve this some other way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants