NotificationsYou must be signed in to change notification settings
Fork906
Star10k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

defelmnq merged 22 commits intomainfromnotif-inbox/inte-403

Mar 5, 2025

Merged

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

defelmnq merged 22 commits intomainfromnotif-inbox/inte-403

Mar 5, 2025

Conversation

Copy link

Contributor

defelmnq commentedMar 3, 2025•
edited
Loading

This PR isresolving 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 theEnqueue method.

add new notification method

61111c9

github-actionsbot assigneddefelmnq

Mar 3, 2025

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

Mar 3, 2025

defelmnq added15 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 fromjohnstcn,dannykopping andDanielleMaywood

March 4, 2025 11:07

defelmnq marked this pull request as ready for review

March 4, 2025 11:14

defelmnq added3 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

Copy link

Contributor

dannykopping left a comment

Choose a reason for hiding this comment

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

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

coderd/notifications/dispatch/inbox.go OutdatedShow resolvedHide resolved

coderd/notifications/enqueuer.goShow resolvedHide resolved

coderd/notifications/manager_test.go Outdated

		@@ -79,7 +81,7 @@ func TestBufferedUpdates(t *testing.T) {
		// Wait for the expected number of buffered updates to be accumulated.
		require.Eventually(t, func() bool {
		success, failure := mgr.BufferedUpdatesCount()
		return success == expectedSuccess && failure == expectedFailure
		return success == expectedSuccess2 && failure == expectedFailure2 // Each message is enqueued twice.

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

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 canlen them instead of hardcoding magic numbers like this.

Copy link

ContributorAuthor

defelmnqMar 4, 2025

Choose a reason for hiding this comment

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

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.

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

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

coderd/notifications/metrics_test.go

		@@ -77,7 +78,10 @@ func TestMetrics(t *testing.T) {

		// 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())

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

Do we need metrics for inbox?

Copy link

ContributorAuthor

defelmnqMar 4, 2025

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

coderd/notifications/notifications_test.goShow resolvedHide resolved

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

		@@ -19,7 +19,8 @@
		"name": "bobby-workspace",
		"reason": "autostart"
		},
		"data": null
		"data": null,
		"targets": null

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

I guess we're never testing with targets?

Copy link

ContributorAuthor

defelmnqMar 4, 2025

Choose a reason for hiding this comment

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

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

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

defelmnqMar 5, 2025

Choose a reason for hiding this comment

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

Ok I took a bit longer than expected , but this targets never changes - it is part ofthe 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 thepayload and another one outside of it.

coderd/notifications/dispatch/inbox_test.go OutdatedShow resolvedHide resolved

defelmnq added2 commits

March 4, 2025 15:06

fixes from review

15933fe

improve testing logic

aae0f98

defelmnq commented

Mar 4, 2025

View reviewed changes

Copy link

ContributorAuthor

defelmnq left a comment

Choose a reason for hiding this comment

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

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 OutdatedShow resolvedHide resolved

coderd/notifications/metrics_test.go

		@@ -77,7 +78,10 @@ func TestMetrics(t *testing.T) {

		// 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())

Copy link

ContributorAuthor

defelmnqMar 4, 2025

Choose a reason for hiding this comment

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

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/manager_test.go Outdated

		@@ -79,7 +81,7 @@ func TestBufferedUpdates(t *testing.T) {
		// Wait for the expected number of buffered updates to be accumulated.
		require.Eventually(t, func() bool {
		success, failure := mgr.BufferedUpdatesCount()
		return success == expectedSuccess && failure == expectedFailure
		return success == expectedSuccess2 && failure == expectedFailure2 // Each message is enqueued twice.

Copy link

ContributorAuthor

defelmnqMar 4, 2025

Choose a reason for hiding this comment

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

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.

defelmnq requested a review fromdannykopping

March 4, 2025 16:04

dannykopping reviewed

Mar 4, 2025

View reviewed changes

coderd/notifications/dispatch/inbox_test.go OutdatedShow resolvedHide resolved

coderd/notifications/enqueuer.goShow resolvedHide resolved

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

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

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.

coderd/notifications/manager_test.go Outdated

		@@ -79,7 +81,7 @@ func TestBufferedUpdates(t *testing.T) {
		// Wait for the expected number of buffered updates to be accumulated.
		require.Eventually(t, func() bool {
		success, failure := mgr.BufferedUpdatesCount()
		return success == expectedSuccess && failure == expectedFailure
		return success == expectedSuccess2 && failure == expectedFailure2 // Each message is enqueued twice.

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

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

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

		@@ -19,7 +19,8 @@
		"name": "bobby-workspace",
		"reason": "autostart"
		},
		"data": null
		"data": null,
		"targets": null

Copy link

Contributor

dannykoppingMar 4, 2025

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

defelmnq commentedMar 5, 2025

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 nottargets from payload as this value is set in two places, including one which is always null.

defelmnq requested a review fromdannykopping

March 5, 2025 14:39

change tests for user id

5a5f2d6

dannykopping reviewed

Mar 5, 2025

View reviewed changes

coderd/notifications/dispatch/inbox_test.go

		@@ -34,7 +35,7 @@ func TestInbox(t *testing.T) {
		payload: types.MessagePayload{
		NotificationName: "test",
		NotificationTemplateID: notifications.TemplateWorkspaceDeleted.String(),
		UserID: "1e965b51-9465-43d8-ac20-c5f689f9c788",
		UserID: "valid",

Copy link

Contributor

dannykoppingMar 5, 2025

Choose a reason for hiding this comment

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

How isvalid a validUserID?

dannykopping approved these changes

Mar 5, 2025

View reviewed changes

Copy link

Contributor

dannykopping left a comment

Choose a reason for hiding this comment

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

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 targetsplus 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.

defelmnq merged commit522181f intomain

Mar 5, 2025

30 checks passed

defelmnq deleted the notif-inbox/inte-403 branch

March 5, 2025 21:43

github-actionsbot locked and limited conversation to collaborators

Mar 5, 2025

Labels

None yet

2 participants

Movatterモバイル変換

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

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

Uh oh!

Conversation

defelmnq commentedMar 3, 2025• editedLoading 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 commentedMar 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!

Uh oh!

defelmnq commentedMar 3, 2025•
edited
Loading