Skip to content

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg commented Oct 23, 2024

What do these changes do?

This PR sets the default log level of faststream RabbitBroker to DEBUG.
@GitHK will need to review this, as this can't just be that so many messages are created here.

An issue was created for him to review this.

Related issue/s

How to test

Dev-ops checklist

@sanderegg sanderegg added a:services-library issues on packages/service-libs a:dynamic-scheduler labels Oct 23, 2024
@sanderegg sanderegg added this to the MartinKippenberger milestone Oct 23, 2024
@sanderegg sanderegg self-assigned this Oct 23, 2024
@sanderegg sanderegg requested a review from pcrespov as a code owner October 23, 2024 20:06
@codecov
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.6%. Comparing base (cafbf96) to head (09506cf).
Report is 668 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6589      +/-   ##
=========================================
+ Coverage    84.5%   87.6%    +3.0%     
=========================================
  Files          10    1217    +1207     
  Lines         214   52982   +52768     
  Branches       25     958     +933     
=========================================
+ Hits          181   46452   +46271     
- Misses         23    6353    +6330     
- Partials       10     177     +167     
Flag Coverage Δ
integrationtests 64.7% <ø> (?)
unittests 85.2% <100.0%> (+0.6%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...src/servicelib/deferred_tasks/_deferred_manager.py 89.1% <100.0%> (ø)

... and 1226 files with indirect coverage changes

@sanderegg sanderegg enabled auto-merge (squash) October 23, 2024 20:10
@sonarqubecloud
Copy link

@sanderegg sanderegg changed the title 🐛Deferred tasks: set default log level of messages in Faststream broker to DEBUG AND pass our logger in so it respects our format 🐛Deferred tasks: set default log level of messages in Faststream broker to DEBUG Oct 24, 2024
@YuryHrytsuk
Copy link
Contributor

YuryHrytsuk commented Oct 24, 2024

@sanderegg why do we set to debug level by default and without a way to override it?

@sanderegg
Copy link
Member Author

@sanderegg why is it set to debug level by default and without a way to override it?

@YuryHrytsuk this is the log level of all messages going through the RabbitBroker class in faststream library. This leads to the 1000s of messages that you saw in master.
Since I do not entirely understand the reasons why it is like that I currently just change the default value to be DEBUG instead of INFO.

  1. I do not know if what is done in these deferred tasks is somehow wrong,
  2. or if this is intended to be changed,
    therefore I prefer to wait on @GitHK so he looks into this.

@YuryHrytsuk
Copy link
Contributor

@sanderegg why is it set to debug level by default and without a way to override it?

@YuryHrytsuk this is the log level of all messages going through the RabbitBroker class in faststream library. This leads to the 1000s of messages that you saw in master. Since I do not entirely understand the reasons why it is like that I currently just change the default value to be DEBUG instead of INFO.

1. I do not know if what is done in these deferred tasks is somehow wrong,

2. or if this is intended to be changed,
   therefore I prefer to wait on @GitHK so he looks into this.

Ah, you mean that by setting it to DEBUG we will always filter them out with INFO (or further) log levels? Is it correct?

P.S. I was initially confused since it looked like now we will generate even more logs because we set some "service" on DEBUG level

@sanderegg
Copy link
Member Author

@sanderegg why is it set to debug level by default and without a way to override it?

@YuryHrytsuk this is the log level of all messages going through the RabbitBroker class in faststream library. This leads to the 1000s of messages that you saw in master. Since I do not entirely understand the reasons why it is like that I currently just change the default value to be DEBUG instead of INFO.

1. I do not know if what is done in these deferred tasks is somehow wrong,

2. or if this is intended to be changed,
   therefore I prefer to wait on @GitHK so he looks into this.

Ah, you mean that by setting it to DEBUG we will always filter them out with INFO (or further) log levels? Is it correct?

@YuryHrytsuk yes. Unless the LOG_LEVEL is set to DEBUG in the configuration.

P.S. I was initially confused since it looked like now we will generate even more logs because we set some "service" on DEBUG level

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

I understand this is an emergency and you will revert it back, right?

@sanderegg
Copy link
Member Author

I understand this is an emergency and you will revert it back, right?

@pcrespov no I have no such intention. The issue created above has been assigned to @GitHK . I am happy to help him if needed, but I do not have the knowledge of deferred tasks so I cannot say now if I will do anything.

@sanderegg sanderegg disabled auto-merge October 24, 2024 07:12
@sanderegg sanderegg merged commit aaabc92 into ITISFoundation:master Oct 24, 2024
57 checks passed
@sanderegg sanderegg deleted the faststream-fix-logger branch October 24, 2024 07:12
mrnicegyu11 pushed a commit to mrnicegyu11/osparc-simcore that referenced this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:dynamic-scheduler a:services-library issues on packages/service-libs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants