Use project as targeting key, propagate message context through entity evaluation #3827
Conversation
internal/engine/handler.go
Outdated
| e.cancels = slices.DeleteFunc(e.cancels, func(cf *context.CancelFunc) bool { | ||
| return cf == &shutdownCancel | ||
| }) |
There was a problem hiding this comment.
Since we're appending a new cancelation function on every call to the handler at line 110, should we call this logic to drop shutdownCancel from the e.cancels slice before the return on line 171 too?
There was a problem hiding this comment.
Oh yeah, this should be a deferred function. Thanks!
There was a problem hiding this comment.
(also line 143... eek)
| map[string]interface{}{ | ||
| "project": ec.Project.ID.String(), | ||
| "project": ec.Project.ID.String(), | ||
| // TODO: is this useful, given how provider names are used? |
There was a problem hiding this comment.
IMHO we should consider removing it, but I don't have strong feelings about it right now.
There was a problem hiding this comment.
I think you're probably right.
internal/engine/handler.go
Outdated
| defer cancel() | ||
| ctx = engcontext.WithEntityContext(ctx, &engcontext.EntityContext{ | ||
| Project: engcontext.Project{ID: inf.ProjectID}, | ||
| // TODO: extract Provider name from ProviderID |
There was a problem hiding this comment.
If we decide to retain the provider as a feature flag attribute, we should consider using the provider ID instead of the name.
There was a problem hiding this comment.
Alternatively, we should use the provider class, rather than a particular provider ID. (i.e. "enable this feature for users with the foo provider") It still feels like a stretch, though.
I think I'll just remove this.
|
Aside from the issue which @puerco raised, this looks good to me. |
| go func() { | ||
| <-ctx.Done() | ||
| eh.lock.Lock() | ||
| defer eh.lock.Unlock() | ||
|
|
||
| for _, cancel := range eh.cancels { | ||
| (*cancel)() | ||
| } | ||
| }() |
There was a problem hiding this comment.
If I got this right, you're still waiting for Done() on what was originally the terminationcontext, but keep track of all downstream contexts to cancel them as well, right?
If yes, was this source of some sort of bug?
There was a problem hiding this comment.
What I really want is Context.TerminateOnAdditionalContext(ctx, additional). But that doesn't exist.
The problem is that handleEntityEvent is (for performance) extracting the event processing out of the watermill message lifecycle, but we still want to be able to cancel message processing during process shutdown.
The previous way we were doing this was to basically take the background context of the server and sub it in for the message context, but that means that we're losing all the message context in the event handler. The new pattern is to use Context.WithoutCancel to escape from the cancellation of the Watermill event handler (because it only allows one message in flight), but then store the cancels for this loop.
If we didn't have the performance problem with Watermill, we still might want to be able to early-terminate the in-flight event handling when the server shuts down, but we'd do that by passing the termination context to Watermill and have it cancel all the contexts. But when we call WithoutCancel, we end up putting that work on ourselves.
(I'm also back-intuiting some of this from #1654)
internal/engine/handler.go
Outdated
| { | ||
| e.lock.Lock() | ||
| defer e.lock.Unlock() | ||
| e.cancels = append(e.cancels, &shutdownCancel) | ||
| } |
There was a problem hiding this comment.
praise: TIL that you can introduce arbitrary lexical scopes in Go as well.
There was a problem hiding this comment.
Same, when I saw it I actually ran a small code snip to make sure I was understanding :)
There was a problem hiding this comment.
Actually, I think I screwed this up -- defer is at the function scope level. I thought I had a test covering this, but clearly I don't.
There was a problem hiding this comment.
Ha, I avoided screwing this up by accident! It turns out that the other lock interaction was inside the goroutine, so this one sort of speeds through and doesn't run into the other lock anyway. But I switched things up to reduce the scope of the lock by explicitly calling unlock on the following line.
Summary
Update entity evaluation to propagate message context and include data for flag evaluation in the context.
Update flag evaluation context to store user in the same form as project and provider, and make the project ID the primary targeting key for providers that need it (not GoFeatureFlag). Since no one is probably using the current fine-grained targeting, this should be okay.
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: