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

Get status code from Firebase error #126

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

larkox
Copy link
Contributor

@larkox larkox commented Sep 3, 2024

Summary

Log firebase error status code along the error itself

Ticket Link

NONE

@larkox larkox added the 1: Dev Review Requires review by a core commiter label Sep 3, 2024
return 0, false
}

response, ok := errorValue.FieldByName("Response").Interface().(*http.Response)
Copy link
Member

Choose a reason for hiding this comment

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

The "internal error encountered" is actually coming from this function NewFirebaseErrorOnePlatform, which tries to parse a GCP error into a firebase error. So what we need is just the ErrorCode field which is what should be considered as the actual status code from my reading.

All of this is so badly written, because the error string doesn't even contain the status code which would have solved all our problems in the start.

@@ -178,7 +180,16 @@ func (me *AndroidNotificationServer) SendNotification(msg *PushNotification) Pus
}

if err != nil {
me.logger.Errorf("Failed to send FCM push sid=%v did=%v err=%v type=%v", msg.ServerID, msg.DeviceID, err, me.AndroidPushSettings.Type)
statusCode, hasStatusCode := getStatusCode(err)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can be bit smart about hasStatusCode and dynamically add the statusCode to the error string if hasStatusCode is true. Otherwise use the original string.

Long term, we should move to structured logging which makes all of this very simple. But out of scope for now.

@larkox larkox requested a review from agnivade September 4, 2024 09:25
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Thanks!

@larkox larkox added 2: Reviews Complete All reviewers have approved the pull request and removed 1: Dev Review Requires review by a core commiter labels Sep 5, 2024
@larkox
Copy link
Contributor Author

larkox commented Sep 5, 2024

/update-branch

@larkox larkox merged commit 83b3754 into mattermost:master Sep 6, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants