Add headers and hooks to taskbroker client#587
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ExternalNamespace was missing context_hooks, so on_dispatch hooks would not fire for tasks dispatched to external applications. Also fixes black formatting issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
create_activation passes headers as MutableMapping[str, Any], not dict[str, str]. Values are stringified after hooks run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests verify: - on_dispatch injects headers during create_activation - No headers injected when no hooks registered - Multiple hooks all get called - on_execute receives activation headers during child_process execution Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Implement ViewerContextHook that propagates ViewerContext through TaskBroker task headers. On dispatch, the hook reads from the contextvar and injects org_id, user_id, and actor_type into task headers. On execution, it restores the ViewerContext from headers into a viewer_context_scope. This enables implicit identity propagation through async tasks without requiring callsites to manually pass user context. Depends on getsentry/taskbroker#587 for the ContextHook protocol. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing class variable annotation for StubContextHook.last_executed and widen RecordingHook.on_dispatch parameter type to MutableMapping to match the ContextHook protocol. Co-Authored-By: Claude Opus 4.6 <noreply@example.com>
Run black on task.py and isort on test_worker.py to fix CI lint failures. Co-Authored-By: Claude Opus 4.6 <noreply@example.com>
|
Everything seems okay here, aside from maybe the AI reviewer comments. The only impact I can see is that task activations may be larger, which could result in a small decrease in throughput. I will let someone else review and approve though 🙂 |
|
Thank you! |
evanh
left a comment
evanh
left a comment
There was a problem hiding this comment.
Can you clarify what these hooks will actually do?
It would also be nice to have an understanding of the performance of the hooks vs. the actual task_func, through metrics or sentry spans.
|
Yup of course. Here's a good example: getsentry/sentry#112217 (check ViewerContextHook) The point of this is to be able to register an action that happens on both dispatch (producer side) and execute (consumer side). In this specific case this allows me to automatically and implicitly pass viewer context whenever a task is created, and then unpack it on the consumer side and pack it directly into contextvars. For performance tests, anything specific you want me to do here? Do you have specific way of testing this, or metrics you are tracking we can look at in rollout? |
| for hook in context_hooks: | ||
| stack.enter_context(hook.on_execute(headers)) |
There was a problem hiding this comment.
It would be helpful to have a timer around context rebuilding. Currently this latency would show up as task execution time, and a failure in rebuilding context counts as a task failure.
- Wrap context hook execution in metrics.timer to measure context rebuild latency separately from task execution time - Use explicit None check for context_hooks to avoid empty list problem when hooks are bound after initialization
4 participants
Relevant context: https://www.notion.so/sentry/RFC-Unified-ViewerContext-via-ContextVar-32f8b10e4b5d81988625cb5787035e02
I think if we want to implicitly pass VC across the wire, we'll need to do these.
to make this work fully implicitly i add hooks to both pack it into context as header on producer side and unpack it on the consumer side automatically.