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

Adjust logger levels to use info for side-effects #293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stephane
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #293 (15198c7) into master (cc70073) will not change coverage.
The diff coverage is 87.5%.

@@          Coverage Diff           @@
##           master    #293   +/-   ##
======================================
  Coverage    86.1%   86.1%           
======================================
  Files          11      11           
  Lines         556     556           
======================================
  Hits          479     479           
  Misses         77      77           
Files Changed Coverage Δ
django_auth_adfs/backend.py 86.4% <87.5%> (ø)

@JonasKs
Copy link
Member

JonasKs commented Aug 2, 2023

Hi, and thank you 😊 I think the only reason I didn't want to do this in the past was because of the potential dashboards and warnings people may have set up.

I don't really know if it makes sense to keep the potential of a broken dashboard in favor of fixing these to be more sensible - especially with Sentry being way more implemented than it was back when I created this.

I approve. What do you think @sondrelg ?

EDIT: oh, I thought you had lowered them from error to info. I'm more in favor of making them all info-statements than making more error-logs. It generates so much Sentry noise.

@sondrelg
Copy link
Member

sondrelg commented Aug 2, 2023

At least for my usage, it would be nice to only log errors when something needs to be explicitly handled by a user. All our projects are set up to propagate error logs to Sentry, so that defines that for us, sort of 🤷 Maybe using warning over error for most of these would be a good solution?

@JonasKs
Copy link
Member

JonasKs commented Aug 2, 2023

I agree. I'm for info+warning and remove all error logs basically.

@sondrelg
Copy link
Member

sondrelg commented Aug 2, 2023

What was your rationale here @stephane?

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.

None yet

3 participants