feat(hook): Deliver events to integrations - #607
Conversation
a09dcab to
b6aef89
Compare
b6aef89 to
a09dcab
Compare
|
🛑 Automatic stack rebase aborted — diff mismatch The rebase of Please rebase manually and verify the changes are correct. |
| failures = append(failures, fmt.Errorf("hook %s: %w", child.Name(), err)) | ||
| } | ||
| } | ||
| return errors.Join(failures...) |
There was a problem hiding this comment.
errors.Join creates an error tree (Unwrap() []error), but platform/errs.NewClassifierProcessor only walks linear Unwrap() error chains. Plain transient errors returned by child hooks can therefore bypass configured classifiers and default to non-retryable, sending the event directly to the DLQ. Let's think through how to make the error classifier properly unwrap joined error chains and classify them for retryability.
| // succeeded. | ||
| func (h Hook) Handle(ctx context.Context, event *basehook.HookEvent) error { | ||
| var failures []error | ||
| for _, child := range h.children { |
There was a problem hiding this comment.
Should these independent hooks run concurrently rather than serially? This would avoid one slow hook from blocking others from starting. Just need to be careful to avoid any leaked goroutines here.
## Summary **What**: - Define the shared event format every domain publishes for fire-and-forget lifecycle notifications, carrying origin, type, timing, a staleness marker, and open per-event detail. - Derive an event's identity from the transition it describes, so replaying a transition produces the same identity and duplicate deliveries collapse. **Why**: - Enable integrations to react to pipeline milestones without adding call sites that can stall or fail a merge. - Establish one schema and one identity rule before any producer ships, so a consumer spanning several domains needs only one reader. ## Test Plan - [x] Add unit tests. ## Revert Plan - Revert this PR. No topic is registered and no message of this shape exists on any queue. ## Issues - [CODEM-416](https://linear.app/uber/issue/CODEM-416/hooks-integration-downstream-notificaiton) ## Stack 1. @ uber#606 1. uber#607 1. uber#608
| // A hook is wired once per host rather than resolved per queue, because what an | ||
| // integration does — post a comment, write a row — is a property of the | ||
| // deployment, not of the queue the event came from. There is therefore no Config | ||
| // and no Factory here: the host constructs its hook directly and hands it to the |
There was a problem hiding this comment.
not really, different queues can point to dfferent providers so each queue should be able define and pick what hooks it wants to wire up
| ) | ||
|
|
||
| // Hook performs a side effect in response to a lifecycle event. | ||
| type Hook interface { |
There was a problem hiding this comment.
my general question here is why is it an extension vs a plain consumer who gets hooks events from wire and implements consumer controller? why it needs to be an interface?
| // Handle implements hook.Hook. It runs every child and returns the joined | ||
| // failures, each attributed to the child that raised it, or nil when all | ||
| // succeeded. | ||
| func (h Hook) Handle(ctx context.Context, event *basehook.HookEvent) error { |
There was a problem hiding this comment.
why do we need composite all all? if i understand correctly, dispatcher can dispatch different hooks, is there a need to use composite which dispatching or even using that from callsite for publishing...
| // on. Dropping an observability row costs a gap in a read model; dropping a | ||
| // merge-failure comment costs a support ticket, and nothing else in the system | ||
| // will notice it is missing. | ||
| type DLQController struct { |
There was a problem hiding this comment.
do we need to provide DLQ Hook extension as well? so we do think in most cases, there is nothing to really reconcile, but say for example, I want to close PR when merge is done and for whatever reason that fails..should we have ability to handle those from retries and DLQ? just a thought, we don't need to support as such
3 participants
Summary
What:
Why:
Test Plan
Revert Plan
Issues
Stack