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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
549c3cd
Log non actionable errors on WARN level
hanzei Jan 2, 2023
51c1a61
Merge branch 'master' into 627_log-error
mattermost-build Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/plugin/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ func (p *Plugin) withRecovery(next http.Handler) http.Handler {
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",
Comment on lines 158 to +161
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 •
edited
Loading

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.

"url", r.URL.String(),
"error", x,
"stack", string(debug.Stack()))
Expand Down Expand Up @@ -192,7 +192,7 @@ func (p *Plugin) checkAuth(handler http.HandlerFunc, responseType ResponseType)
case ResponseTypePlain:
http.Error(w, "Not authorized", http.StatusUnauthorized)
default:
p.API.LogError("Unknown ResponseType detected")
p.API.LogDebug("Unknown ResponseType detected")
}
return
}
Expand Down
2 changes: 1 addition & 1 deletion server/plugin/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestWithRecovery(t *testing.T) {

p := NewPlugin()
api := &plugintest.API{}
api.On("LogError",
api.On("LogWarn",
"Recovered from a panic",
"url", "http://random",
"error", "bad handler",
Expand Down
2 changes: 0 additions & 2 deletions server/plugin/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ func (p *Plugin) sendMessageToCluster(id string, v interface{}) {
}

func (p *Plugin) HandleClusterEvent(ev model.PluginClusterEvent) {
p.API.LogError("received cluster event", "id", ev.Id)

switch ev.Id {
case webHookPingEventID:
var event github.PingEvent
Expand Down
4 changes: 2 additions & 2 deletions server/plugin/mm_34646_token_refresh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,14 @@ func (p *Plugin) forceResetAllMM34646() error {

info, errResp := p.getGitHubUserInfo(tryInfo.UserID)
if errResp != nil {
p.API.LogError("failed to retrieve GitHubUserInfo", "key", key, "user_id", tryInfo.UserID,
p.API.LogWarn("failed to retrieve GitHubUserInfo", "key", key, "user_id", tryInfo.UserID,
"error", errResp.Error())
continue
}

_, err = p.forceResetUserTokenMM34646(ctx, config, info)
if err != nil {
p.API.LogError("failed to reset GitHub user token", "key", key, "user_id", tryInfo.UserID,
p.API.LogWarn("failed to reset GitHub user token", "key", key, "user_id", tryInfo.UserID,
"error", err.Error())
continue
}
Expand Down
12 changes: 6 additions & 6 deletions server/plugin/permalinks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (p *Plugin) makeReplacements(msg string, replacements []replacement, ghClie
r := replacements[i]
// quick bailout if the commit hash is not proper.
if _, err := hex.DecodeString(r.permalinkInfo.commit); err != nil {
p.API.LogError("Bad git commit hash in permalink", "error", err.Error(), "hash", r.permalinkInfo.commit)
p.API.LogWarn("Bad git commit hash in permalink", "error", err.Error(), "hash", r.permalinkInfo.commit)
continue
}

Expand All @@ -106,7 +106,7 @@ func (p *Plugin) makeReplacements(msg string, replacements []replacement, ghClie
if config.EnableCodePreview != "privateAndPublic" {
repo, _, err := ghClient.Repositories.Get(ctx, r.permalinkInfo.user, r.permalinkInfo.repo)
if err != nil {
p.API.LogError("Error while fetching repository information",
p.API.LogWarn("Error while fetching repository information",
"error", err.Error(),
"repo", r.permalinkInfo.repo,
"user", r.permalinkInfo.user)
Expand All @@ -126,7 +126,7 @@ func (p *Plugin) makeReplacements(msg string, replacements []replacement, ghClie
fileContent, _, _, err := ghClient.Repositories.GetContents(ctx,
r.permalinkInfo.user, r.permalinkInfo.repo, r.permalinkInfo.path, &opts)
if err != nil {
p.API.LogError("Error while fetching file contents", "error", err.Error(), "path", r.permalinkInfo.path)
p.API.LogWarn("Error while fetching file contents", "error", err.Error(), "path", r.permalinkInfo.path)
continue
}
// this is not a file, ignore.
Expand All @@ -136,7 +136,7 @@ func (p *Plugin) makeReplacements(msg string, replacements []replacement, ghClie
}
decoded, err := fileContent.GetContent()
if err != nil {
p.API.LogError("Error while decoding file contents", "error", err.Error(), "path", r.permalinkInfo.path)
p.API.LogWarn("Error while decoding file contents", "error", err.Error(), "path", r.permalinkInfo.path)
continue
}

Expand All @@ -153,10 +153,10 @@ func (p *Plugin) makeReplacements(msg string, replacements []replacement, ghClie
}
lines, err := filterLines(decoded, start, end)
if err != nil {
p.API.LogError("Error while filtering lines", "error", err.Error(), "path", r.permalinkInfo.path)
p.API.LogWarn("Error while filtering lines", "error", err.Error(), "path", r.permalinkInfo.path)
}
if lines == "" {
p.API.LogError("Line numbers out of range. Skipping.", "file", r.permalinkInfo.path, "start", start, "end", end)
p.API.LogWarn("Line numbers out of range. Skipping.", "file", r.permalinkInfo.path, "start", start, "end", end)
continue
}
final := getCodeMarkdown(r.permalinkInfo.user, r.permalinkInfo.repo, r.permalinkInfo.path, r.word, lines, isTruncated)
Expand Down
6 changes: 3 additions & 3 deletions server/plugin/permalinks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ func TestGetReplacements(t *testing.T) {
func TestMakeReplacements(t *testing.T) {
p := NewPlugin()
mockPluginAPI := &plugintest.API{}
mockPluginAPI.On("LogError", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
mockPluginAPI.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
mockPluginAPI.On("LogWarn", mock.Anything, mock.Anything)
p.SetAPI(mockPluginAPI)

Expand Down Expand Up @@ -412,8 +412,8 @@ func TestMakeReplacements(t *testing.T) {
})
}

mockPluginAPI.AssertCalled(t, "LogError", "Bad git commit hash in permalink", "error", "encoding/hex: invalid byte: U+0068 'h'", "hash", "badhash")
mockPluginAPI.AssertCalled(t, "LogError", "Error while fetching file contents", "error", "unmarshalling failed for both file and directory content: unexpected end of JSON input and unexpected end of JSON input", "path", "path/file.go")
mockPluginAPI.AssertCalled(t, "LogWarn", "Bad git commit hash in permalink", "error", "encoding/hex: invalid byte: U+0068 'h'", "hash", "badhash")
mockPluginAPI.AssertCalled(t, "LogWarn", "Error while fetching file contents", "error", "unmarshalling failed for both file and directory content: unexpected end of JSON input and unexpected end of JSON input", "path", "path/file.go")
}

const (
Expand Down
10 changes: 5 additions & 5 deletions server/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func (p *Plugin) githubConnectToken(token oauth2.Token) *github.Client {

client, err := GetGitHubClient(token, config)
if err != nil {
p.API.LogError("Failed to create GitHub client", "error", err.Error())
p.API.LogWarn("Failed to create GitHub client", "error", err.Error())
return nil
}

Expand Down Expand Up @@ -290,7 +290,7 @@ func (p *Plugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*mode

shouldProcessMessage, err := client.Post.ShouldProcessMessage(post)
if err != nil {
p.API.LogError("Error while checking if the message should be processed", "error", err.Error())
p.API.LogWarn("Error while checking if the message should be processed", "error", err.Error())
return nil, ""
}

Expand All @@ -302,7 +302,7 @@ func (p *Plugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*mode
info, appErr := p.getGitHubUserInfo(post.UserId)
if appErr != nil {
if appErr.ID != apiErrorIDNotConnected {
p.API.LogError("Error in getting user info", "error", appErr.Message)
p.API.LogWarn("Error in getting user info", "error", appErr.Message)
}
return nil, ""
}
Expand Down Expand Up @@ -614,7 +614,7 @@ func (p *Plugin) GetToDo(ctx context.Context, username string, githubClient *git
}

if n.GetRepository() == nil {
p.API.LogError("Unable to get repository for notification in todo list. Skipping.")
p.API.LogWarn("Unable to get repository for notification in todo list. Skipping.")
continue
}

Expand Down Expand Up @@ -731,7 +731,7 @@ func (p *Plugin) HasUnreads(info *GitHubUserInfo) bool {
}

if n.GetRepository() == nil {
p.API.LogError("Unable to get repository for notification in todo list. Skipping.")
p.API.LogWarn("Unable to get repository for notification in todo list. Skipping.")
continue
}

Expand Down