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

Log non actionable errors on WARN level #628

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Log non actionable errors on WARN level #628

merged 2 commits into from
Jan 12, 2023

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Jan 2, 2023

Summary

Ticket Link

Fixes #627

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Jan 2, 2023
@hanzei hanzei added this to the v2.2.0 milestone Jan 2, 2023
@hanzei hanzei requested a review from mickmister January 2, 2023 22:32
@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2023

Codecov Report

Base: 15.67% // Head: 15.68% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (51c1a61) compared to base (79e2b7a).
Patch coverage: 20.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #628   +/-   ##
=======================================
  Coverage   15.67%   15.68%           
=======================================
  Files          15       15           
  Lines        5231     5229    -2     
=======================================
  Hits          820      820           
+ Misses       4368     4366    -2     
  Partials       43       43           
Impacted Files Coverage Δ
server/plugin/cluster.go 0.00% <ø> (ø)
server/plugin/mm_34646_token_refresh.go 0.00% <0.00%> (ø)
server/plugin/plugin.go 3.69% <0.00%> (ø)
server/plugin/permalinks.go 73.19% <33.33%> (ø)
server/plugin/api.go 7.75% <50.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 157 to +160
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
defer func() {
if x := recover(); x != nil {
p.API.LogError("Recovered from a panic",
p.API.LogWarn("Recovered from a panic",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this strategy in general. Do we do this in other plugins? Should we do this in all plugins?

Copy link
Contributor Author

@hanzei hanzei Jan 3, 2023

Choose a reason for hiding this comment

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

Are you referring to the recover handler? Yes, that is a pattern that all plugins should follow. Currently, only a handful do that.

@spirosoik
Copy link
Member

/update-branch

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jan 12, 2023
@hanzei hanzei merged commit b324769 into master Jan 12, 2023
@hanzei hanzei deleted the 627_log-error branch January 12, 2023 10:25
@mickmister mickmister mentioned this pull request Mar 20, 2023
@avas27JTG avas27JTG mentioned this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non actionable error are logged on ERROR level
5 participants