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

Document Matrix pushkey error + set log level to warnings for Matrix errors #384

Merged

Conversation

christophehenry
Copy link
Contributor

The rationale is to clearly indicate what is set in the configuration compared to what was received in the request for debug purpose. Also, Matrix request errors now logs in warning level. I believe this can be disturbing having your setup not working and no error being logged in the default log level.

Copy link
Owner

@binwiederhier binwiederhier left a comment

Choose a reason for hiding this comment

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

Looks great. Left a comment.

@@ -148,7 +148,7 @@ func writeMatrixDiscoveryResponse(w http.ResponseWriter) error {

// writeMatrixError logs and writes the errMatrix to the given http.ResponseWriter as a matrixResponse
func writeMatrixError(w http.ResponseWriter, r *http.Request, v *visitor, err *errMatrix) error {
log.Debug("%s Matrix gateway error: %s", logHTTPPrefix(v, r), err.Error())
log.Warn("%s Matrix gateway error: %s", logHTTPPrefix(v, r), err.Error())
Copy link
Owner

Choose a reason for hiding this comment

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

So as a rule I log most (all?) "bad request" type errors only as DEBUG, because otherwise users would be able to spam the logs, so I'd be strongly in favor of leaving this as DEBUG. I think we could make an "if >=500" here and log 500s as WARN, but that wouldn't catch errHTTPBadRequestMatrixPushkeyBaseURLMismatch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonnable.

@binwiederhier
Copy link
Owner

Thanks

@binwiederhier binwiederhier merged commit 29a2487 into binwiederhier:main Aug 21, 2022
@binwiederhier
Copy link
Owner

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

2 participants