feat(coderd): add new dispatch logic for coder inbox #16764

defelmnq · 2025-03-03T10:08:10Z

This PR is resolving the dispatch part of Coder Inbocx.

Since the DB layer has been merged - we now want to insert notifications into Coder Inbox in parallel of the other delivery target.

To do so, we push two messages instead of one using the Enqueue method.

dannykopping

Good start, a few things we need to address but none of them huge.

coderd/notifications/dispatch/inbox.go

coderd/notifications/enqueuer.go

dannykopping · 2025-03-04T11:20:48Z

coderd/notifications/manager_test.go

 	require.Eventually(t, func() bool {
 		success, failure := mgr.BufferedUpdatesCount()
-		return success == expectedSuccess && failure == expectedFailure
+		return success == expectedSuccess*2 && failure == expectedFailure*2 // Each message is enqueued twice.


Your tests are going to become complicated once inbox can be disabled (by feature flag or otherwise).
I'd suggest creating a func which returns the configured targets and then you can len them instead of hardcoding magic numbers like this.

I tried to clean it up a bit - still not perfect but as we do not have FF or experiment logic, please tell me if that's good enough.

No I think we need to take this further; there are several other places relying on this magic number.

dannykopping · 2025-03-04T11:21:36Z

coderd/notifications/metrics_test.go


 	// Build fingerprints for the two different series we expect.
 	methodTemplateFP := fingerprintLabels(notifications.LabelMethod, string(method), notifications.LabelTemplateID, tmpl.String())
+	methodTemplateFPWithInbox := fingerprintLabels(notifications.LabelMethod, string(database.NotificationMethodInbox), notifications.LabelTemplateID, tmpl.String())


Do we need metrics for inbox?

I kept it to avoid any specific logic and as I felt like it could be interesting to have - no real other reason tbh.

coderd/notifications/notifications_test.go

dannykopping · 2025-03-04T11:27:25Z

...tifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutobuildFailed.json.golden

    },
-    "data": null
+    "data": null,
+    "targets": null


I guess we're never testing with targets?

def not as much as we could - I added one on the logic but we can improve the golden files.

Which golden file did that update? I don't see it.

Ok I took a bit longer than expected , but this targets never changes - it is part of the buildPayload which takes directly data fetched from db that does not contain the targets.

I'd like to clean it up as a follow-up PR, seems like there's some mixed fields with targets - one within the payload and another one outside of it.

coderd/notifications/dispatch/inbox_test.go

defelmnq

Thanks @dannykopping for the first review - I tried to fix all your comments. 🙏

Just still have an open comment on the FF for inbox.

coderd/notifications/notifications_test.go

defelmnq · 2025-03-04T15:46:20Z

coderd/notifications/metrics_test.go


 	// Build fingerprints for the two different series we expect.
 	methodTemplateFP := fingerprintLabels(notifications.LabelMethod, string(method), notifications.LabelTemplateID, tmpl.String())
+	methodTemplateFPWithInbox := fingerprintLabels(notifications.LabelMethod, string(database.NotificationMethodInbox), notifications.LabelTemplateID, tmpl.String())


I kept it to avoid any specific logic and as I felt like it could be interesting to have - no real other reason tbh.

defelmnq · 2025-03-04T16:03:21Z

coderd/notifications/manager_test.go

 	require.Eventually(t, func() bool {
 		success, failure := mgr.BufferedUpdatesCount()
-		return success == expectedSuccess && failure == expectedFailure
+		return success == expectedSuccess*2 && failure == expectedFailure*2 // Each message is enqueued twice.


I tried to clean it up a bit - still not perfect but as we do not have FF or experiment logic, please tell me if that's good enough.

coderd/notifications/dispatch/inbox_test.go

coderd/notifications/enqueuer.go

dannykopping · 2025-03-04T21:04:08Z

coderd/notifications/notifications_test.go

-	// THEN: we expect to see all but the final attempts failing
+	// the number of tries is equal to the number of messages times the number of attempts
+	// times 2 as the Enqueue method pushes into both the defined dispatch method and inbox
+	nbTries := msgCount * maxAttempts * 2


If we conditionalize when inbox is used in the future, this will become brittle and it won't be easily understood why; this is why magic numbers are to be avoided at ~~all~~ reasonable costs.

dannykopping · 2025-03-04T21:05:36Z

coderd/notifications/manager_test.go

 	require.Eventually(t, func() bool {
 		success, failure := mgr.BufferedUpdatesCount()
-		return success == expectedSuccess && failure == expectedFailure
+		return success == expectedSuccess*2 && failure == expectedFailure*2 // Each message is enqueued twice.


No I think we need to take this further; there are several other places relying on this magic number.

dannykopping · 2025-03-04T21:07:04Z

...tifications/testdata/rendered-templates/webhook/TemplateWorkspaceAutobuildFailed.json.golden

    },
-    "data": null
+    "data": null,
+    "targets": null


Which golden file did that update? I don't see it.

defelmnq · 2025-03-05T14:39:36Z

I fixed most of the comments, but would like to create two follow-up PR :

One prio to implement a FF to enable / disable inbox. This one would need to be implemented just after the endpoints, and will also change the tests and way we compute number of messages sent etc..
One less prio that will define if we want to remove or not targets from payload as this value is set in two places, including one which is always null.

dannykopping · 2025-03-05T14:48:08Z

coderd/notifications/dispatch/inbox_test.go

 				NotificationName:       "test",
 				NotificationTemplateID: notifications.TemplateWorkspaceDeleted.String(),
-				UserID:                 "1e965b51-9465-43d8-ac20-c5f689f9c788",
+				UserID:                 "valid",


How is valid a valid UserID?

dannykopping

Approving to unblock, and for the outstanding comments to be addressed in a follow-up.

I would like to see the magic numbers replaced with a functional call to express which delivery targets are active (this will help when we allow multiple targets plus inbox, later). Having the feature flag (default on, possible opt-out) will naturally expose the need for this.

We just need to ensure this PR does not get cherry-picked into a release, otherwise customers will have no way to turn it off.

add new notification method

61111c9

github-actions bot assigned defelmnq Mar 3, 2025

defelmnq changed the title ~~add new notification method~~ feat(coderd): add new dispatch logic for coder inbox Mar 3, 2025

defelmnq added 15 commits March 3, 2025 10:24

add missing migration file

10276a5

add coder inbox delivery target

9573813

update golden files with targets

4485b59

fix missing details in test

02113f6

fix enqueue functions

1c2f5d2

fix tests on notifications

9134ea2

fix missing index

9a58bf7

improve EnqueueWithData function to have multiple methods

7e088ac

Merge remote-tracking branch 'origin/main' into notif-inbox/inte-403

548335b

rename migration

b995d9d

work on tests

ab1e9ed

finishing up tests

61746c8

fix tests

e265ea0

cleanup testing variables

e56c155

add tests for notifications inbox

ac36085

defelmnq requested review from DanielleMaywood, dannykopping and johnstcn March 4, 2025 11:07

defelmnq marked this pull request as ready for review March 4, 2025 11:14

defelmnq added 3 commits March 4, 2025 11:15

filter coder inbox from UI

ad59e8b

add missing inbox tests

a11f505

fix tests

fe58472

dannykopping reviewed Mar 4, 2025

View reviewed changes

defelmnq added 2 commits March 4, 2025 15:06

fixes from review

15933fe

improve testing logic

aae0f98

defelmnq commented Mar 4, 2025

View reviewed changes

defelmnq requested a review from dannykopping March 4, 2025 16:04

dannykopping reviewed Mar 4, 2025

View reviewed changes

defelmnq requested a review from dannykopping March 5, 2025 14:39

change tests for user id

5a5f2d6

dannykopping reviewed Mar 5, 2025

View reviewed changes

dannykopping approved these changes Mar 5, 2025

View reviewed changes

defelmnq merged commit 522181f into main Mar 5, 2025
30 checks passed

defelmnq deleted the notif-inbox/inte-403 branch March 5, 2025 21:43

github-actions bot locked and limited conversation to collaborators Mar 5, 2025

feat(coderd): add new dispatch logic for coder inbox #16764

feat(coderd): add new dispatch logic for coder inbox #16764

Uh oh!

Conversation

defelmnq commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

defelmnq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

defelmnq commented Mar 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

defelmnq commented Mar 3, 2025 •

edited

Loading