feat(client): add WorkflowClient interceptors for list and fetchHistory - #2215
feat(client): add WorkflowClient interceptors for list and fetchHistory#2215trippyogi wants to merge 4 commits into
Conversation
Wire list and fetchHistory through composeInterceptors so client interceptors can observe or modify those calls, matching describe and Nexus list. Closes temporalio#945.
…ceptors Keep Required<WorkflowClientInterceptor> completeness after adding the new client interceptor methods.
| }); | ||
| }); | ||
|
|
||
| test.serial('WorkflowClientInterceptor intercepts list and fetchHistory', async (t) => { |
There was a problem hiding this comment.
I think the list() hook will need more coverage than this. The fact that it is a generator adds some complications. Plus, we'll need to test interception in the case of list().withHistories().
Let's try this. Start a few workflow executions (~10), then do for await (const _ of client.list({ /* query is not important */, pageSize: 2 })) {. That would require >5 requests to the server to see all search result. But then, in the loop, break once you've seen 3 pages, without reaching the end of the generator.
I would expect the list interceptor to have been called only once in that sequence. And given the current test implementation (interceptor function is sync), I'm pretty sure the list interceptor function will return immediately.
Does that "return immediately" behavior really what we need for list interceptors? For example, will OTel correctly figure out the encapsulation and timeline of the list call? I'm really not sure about that. My intuition is that the list interceptor would have to be a generator itself, proxying each request to the downstream AsyncIterator obtained by calling next().
Also, do the same thing with for await (const _ of client.list({ /* query is not important */, pageSize: 2 }).intoHistories()) {. In addition to the list() call, confirm that you're seeing >6 calls to fetchHistory.
|
Thanks for the contribution. Out of curiosity, do you have an actual use case requiring those specific interceptors? One of the reasons we never prioritized that request is due to uncertainties on what granularity we'd want the |
Thanks, @mjameswh. I don’t have a production use case that depends on these interceptors. I opened this from #945’s completeness gap, without a defined list() granularity or a concrete consumer. The motivating idea was observability around those client ops, but I agree the generator / multi-page semantics need to be settled before locking the API. Happy to park or close this until that’s clearer, or split out fetchHistory if you want that independently while list gets designed. |
Summary
listandfetchHistorytoWorkflowClientInterceptor, with matching input typescomposeInterceptors(same pattern as describe / Nexus list)test-interceptors.tsCloses #945
Test plan
pnpm -F @temporalio/client... run buildRUN_INTEGRATION_TESTS=true pnpm -F @temporalio/test run test -- test-interceptorslist/fetchHistorycalls andnext()still works