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

reduce logging output #2353

Merged
merged 3 commits into from
Dec 14, 2021
Merged

reduce logging output #2353

merged 3 commits into from
Dec 14, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Dec 10, 2021

Reduced log output. Some errors or warnings were logged multiple times or even unnecesarily.

Now, there are probably a lot of places where logging is missing but these can be added when we touch those spots again.

@C0rby
Copy link
Contributor Author

C0rby commented Dec 10, 2021

Similarly, should both the services and the gateway log stuff or only one of both? If the latter who should log? I'd say the service itself would make sense.

@C0rby
Copy link
Contributor Author

C0rby commented Dec 13, 2021

I added error logging to the storage provider. It probably would make sense to have more logging in other places too but I propose to add those when we are working at those places.

@refs
Copy link
Member

refs commented Dec 13, 2021

I find this approach better rather than having a wall of text. Adding logging when needed (if needed) is more beneficial 👍

@lgtm-com
Copy link

lgtm-com bot commented Dec 13, 2021

This pull request introduces 25 alerts when merging 8a9f44c into e0529be - view on LGTM.com

new alerts:

  • 25 for Useless assignment to local variable

@C0rby C0rby marked this pull request as ready for review December 13, 2021 16:01
@C0rby
Copy link
Contributor Author

C0rby commented Dec 13, 2021

There are more things that can be done but I'm going to leave it at that to not make this PR any longer.

@C0rby C0rby requested a review from refs December 13, 2021 16:02
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