A more configurable URLNotifier #726
Conversation
|
webhook/notifier.go
Outdated
| }) | ||
| n.urlNotifiers = append(n.urlNotifiers, u) | ||
| } | ||
| return n | ||
| } | ||
|
|
||
| func NewDefaultNotifierByParams(params []URLNotifierParams) QueuedNotifier { |
There was a problem hiding this comment.
what is the intent of creating different notifiers underneath?
There was a problem hiding this comment.
For more control. With this we can have different configuration per webhook. But the real reason was to avoid long constructors. It was simpler just to pass in a list of URLNotifierParams.
| @@ -49,7 +49,9 @@ message WebhookEvent { | |||
| // timestamp in seconds | |||
| int64 created_at = 7; | |||
|
|
|||
| int64 dequeued_at = 12; | |||
There was a problem hiding this comment.
why is this field needed? what is it intended to communicate to to the end user?
There was a problem hiding this comment.
This is useful for identifying issues in webhook logic. I have high latency spikes for webhook events, but I don't know how much it took Livekit from when the event was queued until it's been sent.
| APISecret string | ||
| Logger logger.Logger | ||
| QueueSize int | ||
| DropWhenFull bool |
There was a problem hiding this comment.
should this be exposed at all? what is the behavior when it's set to false?
There was a problem hiding this comment.
If set to false, events will drop only due to network failures, and not the queue being full. Assume we want to make events more reliabale, i think we should be able to configure this parameter.
|
|
* Add a new constructor that accepts 'URLNotifierParams' directly
* Add 'DropWhenFull' as parameter
* Add 'DeqeuedAt' for better monitoring at hooked backends
* Fixes tag name being off by 1
* Sets DequeuedAt
#3) * feat: allow for custom URLNotifier and add batched events protobuf definition * feat: Implement batch sender
* feat: Add include/exclude options * generated protobuf --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Makes URLNotifier more configurable