Nexus interceptors #1218
Nexus interceptors #1218
Conversation
temporalio/worker/_interceptor.py
Outdated
|
|
||
|
|
||
| class _NexusOperationHandlerForInterceptor( | ||
| nexusrpc.handler.MiddlewareSafeOperationHandler[Any, Any] |
There was a problem hiding this comment.
Now that I think about it, I wonder if there is any value in MiddlewareSafeOperationHandler accepting generics itself. Won't it basically always be [Any, Any]?. I think we should consider removing those over in the Nexus SDK.
There was a problem hiding this comment.
100% agreed here. Went ahead and made the change
| cast, | ||
| ) | ||
|
|
||
| import nexusrpc.handler |
There was a problem hiding this comment.
This does both import nexusrpc.handler and from nexusrpc.handler import X. Should decide which approach. I personally prefer import nexusrpc.handler and qualify at use site.
temporalio/contrib/opentelemetry.py
Outdated
| updated_context_carrier, add_to_outbound.headers | ||
| ) | ||
|
|
||
| def _completed_span_nexus( |
There was a problem hiding this comment.
I wonder if you can just add a add_to_outbound_string_headers: Optional[_InputWithStringHeaders] = None to the existing method instead of making a new one. Same for adding a input_string_headers: Optional[_InputWithStringHeaders] = None to _start_as_current_span instead of a new method (maybe, didn't check here). Doesn't matter too much, but does keep the most of the logic centralized knowing the only difference is input type.
There was a problem hiding this comment.
Appreciate the suggestion here as this is what felt the worst to me. I originally was hoping to make it add_to_outbound: Optional[_InputWithHeaders | _InputWithStringHeaders] but it didn't seem like I could do a runtime check against which one it was. After your suggestion though, I realized that the defaulted kwarg is the same amount of extra runtime checks as the union would be and still allow keeping the logic centralized. LMK what you think of the new setup.
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM (not approving to let @tconley1428 look and since it's draft)
What was changed
NexusOperationInboundInterceptorNexusOperationInboundInterceptortonexusrpc.handler.NexusOperationMiddlewareChecklist
Pairs with nexus-rpc/sdk-python#33
How was this tested: