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

chore: distinguish bad device token errors #257

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Oct 19, 2023

Description

We have a lot of apns(unregistered) HTTP 500 errors. An unregistered device token is not an internal server error and should be distinguished from it. BadDeviceToken error should be used for such cases.

  1. This PR changes the BadDeviceToken error type to have a string argument that reflects a certain device token error.
  2. Distinguish apns and fcm responses from bad device tokens and other types that should throw 500 (Internal Server Error).

Resolves #256

How Has This Been Tested?

Not tested.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@geekbrother geekbrother requested a review from xav October 19, 2023 10:01
@geekbrother geekbrother self-assigned this Oct 19, 2023
@arein arein added the accepted The issue has been accepted into the project label Oct 19, 2023
Copy link
Contributor

@xav xav left a comment

Choose a reason for hiding this comment

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

Why are we returning a 400 when a dependency fails? The client has nothing to do with it, wouldn't it be better with a 503 instead?

@xav
Copy link
Contributor

xav commented Oct 19, 2023

Why are we returning a 400 when a dependency fails? The client has nothing to do with it, wouldn't it be better with a 503 instead?

That would also mean we should stop monitoring 5xx and only monitor 500 to see actual server errors.

@geekbrother
Copy link
Contributor Author

geekbrother commented Oct 19, 2023

That would also mean we should stop monitoring 5xx and only monitor 500 to see actual server errors.

As a possible solution, we can check if the error is Unregistered and throw another return code such as 4xx and throw 500 otherwise to catch all non Unregistered errors. Because we don't want to monitor all client's unregistered token errors and have an alert for them.

@xav
Copy link
Contributor

xav commented Oct 19, 2023

That would also mean we should stop monitoring 5xx and only monitor 500 to see actual server errors.

As a possible solution, we can check if the error is Unregistered and throw another return code such as 4xx and throw 500 otherwise to catch all non Unregistered errors. Because we don't want to monitor all client's unregistered token errors and have an alert for them.

That would be nice since it would let us still see the actual errors instead of hiding everything under 400s

@geekbrother
Copy link
Contributor Author

That would be nice since it would let us still see the actual errors instead of hiding everything under 400s

We have a specific error for the Unregistered in A2, I'll just extract it and throw an error only for that case. Thanks @xav!

@geekbrother geekbrother force-pushed the max/chore/remove_500_for_apns_fcm_responses branch from b83dce1 to a918363 Compare October 19, 2023 10:45
@geekbrother geekbrother changed the title chore: change apns and fcm response errors from 500 to 400 chore: distinguish bad device token errors Oct 19, 2023
@geekbrother geekbrother force-pushed the max/chore/remove_500_for_apns_fcm_responses branch from a918363 to adf0860 Compare October 19, 2023 11:05
@geekbrother geekbrother marked this pull request as ready for review October 19, 2023 11:10
@geekbrother geekbrother requested a review from xav October 19, 2023 11:13
@geekbrother geekbrother merged commit 733c152 into main Oct 19, 2023
4 checks passed
@chris13524 chris13524 deleted the max/chore/remove_500_for_apns_fcm_responses branch October 19, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The issue has been accepted into the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: apns and fcm bad token responses are considered as an internal server errors
4 participants