Add interceptor support to springboot integration#2500
Add interceptor support to springboot integration#2500Quinn-With-Two-Ns merged 4 commits intotemporalio:masterfrom
Conversation
| new OpenTracingWorkerInterceptor( | ||
| OpenTracingOptions.newBuilder().setTracer(tracer).build()); | ||
| options.setWorkerInterceptors(openTracingClientInterceptor); | ||
| interceptors.add(openTracingClientInterceptor); |
There was a problem hiding this comment.
@cretz @Sushisource Any thoughts on the default order here? I am putting the tracing interceptor at the end of the chain. unless we want to trace the interceptors themselves maybe?
There was a problem hiding this comment.
Interesting. In other SDKs we have them create the tracing interceptor instead of just the tracer, so it is their responsibility to choose order. But I can understand here it's easier if tracer is the bean they configure.
Hard to say whether at the end (is that innermost?) is a good default. Some users swallow or mutate errors, and the span may be better off reflecting the results of other people's interceptors.
Say I didn't want it at the end of the chain as a user, how hard would it be for me to provide a null tracer to this constructor here and put it at the beginning of the workerInterceptors list? Just curious, because both defaults have tradeoffs, so just wondering if there was a workaround.
There was a problem hiding this comment.
Yeah end would be innermost, to work around a user would provide a customizer to change the interceptors
There was a problem hiding this comment.
I can check what Quarkus did
There was a problem hiding this comment.
👍 If there's a workaround (e.g. don't inject tracer, make the interceptor in the list yourself), that's probably good enough. I do think the tracing intercepter being the outermost makes sense to have the span cover what other interceptors are doing (from a time or exception POV).
Add interceptor support to Springboot integration. With this PR any interceptor bean will be automatically picked up and registered with the worker/client. Similar to Quarkus support for interceptors in Temporal.
Note: Currently not supported in multi namespace.
closes: #1634