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

stop logging errors for api error responses #5022

Closed
wants to merge 2 commits into from

Conversation

lieut-data
Copy link

Summary

From a cursory glance, when a real error occurs, we already log something at the application layer, so there's no reason to echo every api error response as an error log too. Errors should be actionable, but these logs were not, e.g.:

access denied to templates

or:

category ID specified in input does not exist for user

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-59325

From a cursory glance, when a real error occurs, we already log
something at the application layer, so there's no reason to echo every
api error response as an error log too. Errors should be actionable, but
these logs were not, e.g.:

> access denied to templates

or:

> category ID specified in input does not exist for user
@lieut-data lieut-data added the 2: Dev Review Requires review by a core committer label Jul 4, 2024
Copy link
Contributor

@Rajat-Dabade Rajat-Dabade left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lieut-data
Copy link
Author

@Rajat-Dabade, the test failure here seems spurious -- do you know if it's a known issue?

@marianunez
Copy link
Contributor

marianunez commented Jul 8, 2024

@lieut-data we've separated Boards as a plugin to it's own repo at: https://github.com/mattermost/mattermost-plugin-boards This will be the repo we will be supporting and releasing Boards from moving forward while Focalboard will remain as community supported.

Do you want to move this PR there so your changes can make the next plugin release?

@Rajat-Dabade
Copy link
Contributor

Rajat-Dabade commented Jul 9, 2024

@Rajat-Dabade, the test failure here seems spurious -- do you know if it's a known issue?

@lieut-data This has something to do with MySQL docker image deprecated version. Bumping up the image version might resolve the issue. Let me check.

@lieut-data
Copy link
Author

Aha, thank you @marianunez! The new repo was a blindspot for me. I'll copy the PR over there: should I keep it here as well?

@marianunez
Copy link
Contributor

should I keep it here as well?

@lieut-data Is this relevant for Focalboard as a standalone application or just for plugin logs in MM Server? It it's the latter, then I wouldn't think it's relevant to keep here.

@lieut-data
Copy link
Author

should I keep it here as well?

@lieut-data Is this relevant for Focalboard as a standalone application or just for plugin logs in MM Server? It it's the latter, then I wouldn't think it's relevant to keep here.

I guess from a log quality perspective, the changes might be relevant, but as we're not intending to keep the standalone and plugin in sync, I'll just close this out.

@lieut-data lieut-data closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants