[Notifications] Improving webhook notifications #6965
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors webhook notifications to use an in-memory Watermill pub/sub queue, adds configurable filters for event types and path prefixes, and updates the HTTP client to support timeouts.
- Introduce
Queuebacked by Watermill GoChannel and retry/poison-queue middleware - Apply event-type and path-prefix filters before publishing to the queue
- Update HTTP client to accept structured
webhookMessageand enforce request timeouts
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| weed/notification/webhook/webhook_queue_test.go | Added comprehensive tests for config validation, serialization, queue init/send, retry, and filters |
| weed/notification/webhook/webhook_queue.go | Implemented Queue using Watermill, middleware setup, handler, and dead-letter logging |
| weed/notification/webhook/types.go | Defined config defaults/validation, webhookMessage, and event type detection |
| weed/notification/webhook/http_test.go | Updated HTTP client tests to use newWebhookMessage and assert event_type metadata |
| weed/notification/webhook/http.go | Changed sendMessage signature, added JSON payload, timeout handling, and removed raw proto import |
| weed/notification/webhook/filter.go & filter_test.go | Added filter logic and tests for event types and path prefixes |
Comments suppressed due to low confidence (3)
weed/notification/webhook/http.go:52
- Add a Content-Type header for JSON payloads before sending the request, e.g.,
req.Header.Set("Content-Type", "application/json").
if h.token != "" {
weed/notification/webhook/webhook_queue.go:1
- [nitpick] Add GoDoc comments to exported types and methods (e.g.,
Queue,SendMessage,Initialize,handleWebhook) to improve code navigation and documentation.
package webhook
weed/notification/webhook/webhook_queue.go:60
- Guard against uninitialized
w.queueChannelorw.clientto prevent panics ifInitializewas not called.
return w.queueChannel.Publish(pubSubTopicName, wMsg)
| defer cancel() | ||
| req = req.WithContext(ctx) | ||
| } | ||
|
|
||
| resp, err := util_http.Do(req) | ||
| if err != nil { |
There was a problem hiding this comment.
Check resp.StatusCode for non-2xx responses and return an error, and ensure resp.Body.Close() is called to avoid leaking connections.
| if err != nil { | |
| if err != nil { | |
| if resp != nil { | |
| resp.Body.Close() | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
need to consume the response body. otherwise, the http connection reusing would not work.
There was a problem hiding this comment.
@chrislusf thanks, closed it in case of error as suggested.
There was a problem hiding this comment.
close it is not enough. need to read all remaining body content.
| for { | ||
| select { | ||
| case msg := <-ch: | ||
| glog.Errorf("received dead letter message: %v", msg) |
There was a problem hiding this comment.
[nitpick] Log more context for dead-lettered messages, such as msg.Metadata or string(msg.Payload), rather than the raw message object.
| glog.Errorf("received dead letter message: %v", msg) | |
| glog.Errorf("received dead letter message: metadata=%v, payload=%s", msg.Metadata, string(msg.Payload)) |
Copilot uses AI. Check for mistakes.
What problem are we solving?
How are we solving the problem?
How is the PR tested?
Checks