[Tunnelbroker] add missing error logs for notifs
ClosedPublic
Actions

Authored by kamil on Nov 19 2024, 5:20 AM.

Details

Reviewers

will
bartek

Commits

rCOMM92cb5ba01540: [Tunnelbroker] add missing error logs for notifs

Summary

While investigating ENG-9925 I discovered there are a couple more errors that worth logging

Depends on D13969

Test Plan

Not really sure what is the best way to test it - maybe @will have some ideas.

On APNs:

Notif works
Update the code to send notif with random priority -> fails with BadPriority

On web:

Notif works
Triggering error is complicated because we use lib, so just reading the code as we use the same approach as for others

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Nov 19 2024, 5:20 AM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptNov 19 2024, 5:20 AM

kamil retitled this revision from [Tunnelbroker] add missing error logs for alarms to [Tunnelbroker] add missing error logs for notifs.Nov 19 2024, 5:20 AM

remove import

kamil edited the summary of this revision. (Show Details)Nov 19 2024, 5:25 AM

kamil added inline comments.

services/tunnelbroker/src/websockets/session.rs
667–673 ↗	(On Diff #45887)	there is no better place to do it, for Web Push we use lib instead of our custom implementation

Harbormaster completed remote builds in B32717: Diff 45887.Nov 19 2024, 5:40 AM

Harbormaster completed remote builds in B32716: Diff 45886.Nov 19 2024, 5:48 AM

kamil published this revision for review.Nov 19 2024, 6:46 AM

Testing that adding WEB_PUSH_ERROR and APNS_ERROR error types to an error log will trigger an alarm + email notif has already been tested. I don't think this needs further testing.

The best way of testing this would be to ensure that failing to sending APNs error or Web Push notif logs the error. Cloudwatch alarms will take care of the rest

Doing this locally and ensuring you can find the error logs should be sufficient imo

In D13970#389656, @will wrote:

Testing that adding WEB_PUSH_ERROR and APNS_ERROR error types to an error log will trigger an alarm + email notif has already been tested. I don't think this needs further testing.

The best way of testing this would be to ensure that failing to sending APNs error or Web Push notif logs the error. Cloudwatch alarms will take care of the rest

Doing this locally and ensuring you can find the error logs should be sufficient imo

Thanks - updated test plan

will accepted this revision.Nov 20 2024, 5:49 AM

This revision is now accepted and ready to land.Nov 20 2024, 5:49 AM

rebase before landing

Harbormaster completed remote builds in B32813: Diff 46018.Mon, Nov 25, 5:08 AM

Closed by commit rCOMM92cb5ba01540: [Tunnelbroker] add missing error logs for notifs (authored by kamil). · Explain WhyMon, Nov 25, 5:17 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM92cb5ba01540: [Tunnelbroker] add missing error logs for notifs.

kamil mentioned this in D14039: [terraform] deploy tunnelbroker 0.18.2 to staging.Mon, Nov 25, 6:55 AM

kamil mentioned this in rCOMM34ae7bc14a49: [terraform] deploy tunnelbroker 0.18.2 to staging.Tue, Nov 26, 1:42 AM

kamil mentioned this in D14069: [terraform] deploy tunnelbroker 0.18.2 to production.Mon, Dec 2, 2:56 AM

kamil mentioned this in rCOMM98f0d29d70bc: [terraform] deploy tunnelbroker 0.18.2 to production.Mon, Dec 2, 4:05 AM