Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit356604e

Browse files
chore(coderd/notifications): avoid generating warning logs for trivial enqueue failures (#19840)
I noticed during a scaletest that many warning logs were being generated when enqueuing notifications. The error was:```failed to notify of workspace creation: notification is not enabled```I don't think we should be warning if automated notifications fail to send to users because they have them disabled.To fix, we'll stop returning these errors.
1 parent8781499 commit356604e

File tree

2 files changed

+39
-14
lines changed

2 files changed

+39
-14
lines changed

‎coderd/notifications/enqueuer.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,14 @@ func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers tem
7373
}
7474

7575
// Enqueue queues a notification message for later delivery, assumes no structured input data.
76+
// Returns the IDs of successfully enqueued messages, if any.
7677
func (s*StoreEnqueuer)Enqueue(ctx context.Context,userID,templateID uuid.UUID,labelsmap[string]string,createdBystring,targets...uuid.UUID) ([]uuid.UUID,error) {
7778
returns.EnqueueWithData(ctx,userID,templateID,labels,nil,createdBy,targets...)
7879
}
7980

8081
// Enqueue queues a notification message for later delivery.
8182
// Messages will be dequeued by a notifier later and dispatched.
83+
// Returns the IDs of successfully enqueued messages, if any.
8284
func (s*StoreEnqueuer)EnqueueWithData(ctx context.Context,userID,templateID uuid.UUID,labelsmap[string]string,datamap[string]any,createdBystring,targets...uuid.UUID) ([]uuid.UUID,error) {
8385
metadata,err:=s.store.FetchNewMessageMetadata(ctx, database.FetchNewMessageMetadataParams{
8486
UserID:userID,
@@ -141,14 +143,26 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID
141143
//
142144
// This is more efficient than fetching the user's preferences for each enqueue, and centralizes the business logic.
143145
ifstrings.Contains(err.Error(),ErrCannotEnqueueDisabledNotification.Error()) {
144-
returnnil,ErrCannotEnqueueDisabledNotification
146+
s.log.Debug(ctx,"notification not enqueued",
147+
slog.F("template_id",templateID),
148+
slog.F("user_id",userID),
149+
slog.F("method",method),
150+
slog.Error(ErrCannotEnqueueDisabledNotification),
151+
)
152+
continue
145153
}
146154

147155
// If the enqueue fails due to a dedupe hash conflict, this means that a notification has already been enqueued
148156
// today with identical properties. It's far simpler to prevent duplicate sends in this central manner, rather than
149157
// having each notification enqueue handle its own logic.
150158
ifdatabase.IsUniqueViolation(err,database.UniqueNotificationMessagesDedupeHashIndex) {
151-
returnnil,ErrDuplicate
159+
s.log.Debug(ctx,"notification not enqueued",
160+
slog.F("template_id",templateID),
161+
slog.F("user_id",userID),
162+
slog.F("method",method),
163+
slog.Error(ErrDuplicate),
164+
)
165+
continue
152166
}
153167

154168
s.log.Warn(ctx,"failed to enqueue notification",slog.F("template_id",templateID),slog.F("input",input),slog.Error(err))

‎coderd/notifications/notifications_test.go‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"golang.org/x/xerrors"
3636

3737
"cdr.dev/slog"
38+
"cdr.dev/slog/sloggers/sloghuman"
3839
"github.com/coder/quartz"
3940
"github.com/coder/serpent"
4041

@@ -1654,18 +1655,21 @@ func TestDisabledByDefaultBeforeEnqueue(t *testing.T) {
16541655

16551656
ctx:=dbauthz.AsNotifier(testutil.Context(t,testutil.WaitSuperLong))
16561657
store,_:=dbtestutil.NewDB(t)
1657-
logger:=testutil.Logger(t)
1658+
logbuf:= strings.Builder{}
1659+
logger:=testutil.Logger(t).AppendSinks(sloghuman.Sink(&logbuf)).Leveled(slog.LevelDebug)
16581660

16591661
cfg:=defaultNotificationsConfig(database.NotificationMethodSmtp)
16601662
enq,err:=notifications.NewStoreEnqueuer(cfg,store,defaultHelpers(),logger.Named("enqueuer"),quartz.NewReal())
16611663
require.NoError(t,err)
16621664
user:=createSampleUser(t,store)
16631665

16641666
// We want to try enqueuing a notification on a template that is disabled
1665-
// by default. We expect this tofail.
1667+
// by default. We expect this tobe a no-op that produces a debug log.
16661668
templateID:=notifications.TemplateWorkspaceManuallyUpdated
1667-
_,err=enq.Enqueue(ctx,user.ID,templateID,map[string]string{},"test")
1668-
require.ErrorIs(t,err,notifications.ErrCannotEnqueueDisabledNotification,"enqueuing did not fail with expected error")
1669+
notifIDs,err:=enq.Enqueue(ctx,user.ID,templateID,map[string]string{},"test")
1670+
require.NoError(t,err)
1671+
require.Contains(t,logbuf.String(),notifications.ErrCannotEnqueueDisabledNotification.Error())
1672+
require.Empty(t,notifIDs)
16691673
}
16701674

16711675
// TestDisabledBeforeEnqueue ensures that notifications cannot be enqueued once a user has disabled that notification template
@@ -1679,7 +1683,8 @@ func TestDisabledBeforeEnqueue(t *testing.T) {
16791683

16801684
ctx:=dbauthz.AsNotifier(testutil.Context(t,testutil.WaitSuperLong))
16811685
store,_:=dbtestutil.NewDB(t)
1682-
logger:=testutil.Logger(t)
1686+
logbuf:= strings.Builder{}
1687+
logger:=testutil.Logger(t).AppendSinks(sloghuman.Sink(&logbuf)).Leveled(slog.LevelDebug)
16831688

16841689
// GIVEN: an enqueuer & a sample user
16851690
cfg:=defaultNotificationsConfig(database.NotificationMethodSmtp)
@@ -1697,9 +1702,12 @@ func TestDisabledBeforeEnqueue(t *testing.T) {
16971702
require.NoError(t,err,"failed to set preferences")
16981703
require.EqualValues(t,1,n,"unexpected number of affected rows")
16991704

1700-
// THEN: enqueuing the "workspace deleted" notification should fail with an error
1701-
_,err=enq.Enqueue(ctx,user.ID,templateID,map[string]string{},"test")
1702-
require.ErrorIs(t,err,notifications.ErrCannotEnqueueDisabledNotification,"enqueueing did not fail with expected error")
1705+
// THEN: enqueuing the "workspace deleted" notification should fail be
1706+
// a no-op that produces a debug log
1707+
notifIDs,err:=enq.Enqueue(ctx,user.ID,templateID,map[string]string{},"test")
1708+
require.NoError(t,err)
1709+
require.Contains(t,logbuf.String(),notifications.ErrCannotEnqueueDisabledNotification.Error())
1710+
require.Empty(t,notifIDs)
17031711
}
17041712

17051713
// TestDisabledAfterEnqueue ensures that notifications enqueued before a notification template was disabled will not be
@@ -1909,7 +1917,8 @@ func TestNotificationDuplicates(t *testing.T) {
19091917

19101918
ctx:=dbauthz.AsNotifier(testutil.Context(t,testutil.WaitSuperLong))
19111919
store,pubsub:=dbtestutil.NewDB(t)
1912-
logger:=testutil.Logger(t)
1920+
logbuf:= strings.Builder{}
1921+
logger:=testutil.Logger(t).AppendSinks(sloghuman.Sink(&logbuf)).Leveled(slog.LevelDebug)
19131922

19141923
method:=database.NotificationMethodSmtp
19151924
cfg:=defaultNotificationsConfig(method)
@@ -1933,10 +1942,12 @@ func TestNotificationDuplicates(t *testing.T) {
19331942
map[string]string{"initiator":"danny"},"test",user.ID)
19341943
require.NoError(t,err)
19351944

1936-
// WHEN: the second is enqueued, the enqueuer will rejectthe request.
1937-
_,err=enq.Enqueue(ctx,user.ID,notifications.TemplateWorkspaceDeleted,
1945+
// WHEN: the second is enqueued, the enqueuer will rejectit as a duplicate.
1946+
ids,err:=enq.Enqueue(ctx,user.ID,notifications.TemplateWorkspaceDeleted,
19381947
map[string]string{"initiator":"danny"},"test",user.ID)
1939-
require.ErrorIs(t,err,notifications.ErrDuplicate)
1948+
require.NoError(t,err)
1949+
require.Contains(t,logbuf.String(),notifications.ErrDuplicate.Error())
1950+
require.Empty(t,ids)
19401951

19411952
// THEN: when the clock is advanced 24h, the notification will be accepted.
19421953
// NOTE: the time is used in the dedupe hash, so by advancing 24h we're creating a distinct notification from the one

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp