akka-projection icon indicating copy to clipboard operation
akka-projection copied to clipboard

ThreadLocal aware context propagation for telemetry

Open ignasi35 opened this issue 4 years ago • 7 comments

(See some of the discussion in https://github.com/akka/akka-projection/pull/230#discussion_r444898182 but most of the discussion continued offline (private link))

Short description

Support ThreadLocal dependant SPI-implementations.

Details

Some telemetry backends rely on ThreadLocal usage which restricts what hooks to expose on the TelemetrySpi and how to use them. We need a hook to be invoked immediately before handler.process and another one to happen immediately after and invoked from the same thread where the before was invoked and always invoked (to prevent resource leaking).

ignasi35 avatar Jun 25 '20 19:06 ignasi35

Started some draft work on https://github.com/ignasi35/akka-projection/commit/f8966d29af11ba48a6f5624a0da1ab0fee2eb004

ignasi35 avatar Jun 25 '20 19:06 ignasi35

This is easily implemented in the case of SingleHandler but I think the other handlers may present some challenges:

  • GroupHandler processes multiple envelopes at once, so it's unclear which envelope should be passed on (or maybe all of them)
  • in FlowHandler I'm not sure we would be able to wrap the user code between invocations to beforeSchedule/afterSchedule guaranteeing afterSchedule is always invoked and that is is invoked from the same thread.

ignasi35 avatar Jun 25 '20 19:06 ignasi35

Spontaneously, we should propagate context via ProjectionContext or the external context. Not via ThreadLocal.

patriknw avatar Jul 02 '20 06:07 patriknw

Spontaneously, we should propagate context via ProjectionContext or the external context. Not via ThreadLocal.

I think there is confusion about what context means in this issue. This issue is not about the ProjectionContext we propagate next to the envelope across the whole stream.

Instead, this issue only focuses on a short-lived, telemetry-specific context (a Any) which can only be propagated using ThreadLocal because of implementation limitations on the underlying libraries. As in: "in order to use library ABC.jar for tracing the creation and the release of the tracing span must be invoked on the same Thread".

So the proposal involves only 2 calls in the TelemetrySpi where it could make sense to try to fulfill this implementation-specific request. These 2 calls in the SPI will be the new beforeSchedule/afterSchedule which will wrap the handler.process invocation.

cc @ygree

ignasi35 avatar Jul 06 '20 15:07 ignasi35

In some cases the before and after hooks are needed. For instance, for measuring execution time when before returns a timestamp as a context passed into after, or for setting a tracing context (thread local) and swapping it back. The latter is needed to avoid residual contexts to avoid propagating such contexts into the next task.

ygree avatar Jul 08 '20 13:07 ygree

@ignasi35 / @ygree, is this something we need to have on v1.0.0 or can be added later?

octonato avatar Jul 20 '20 09:07 octonato

@ignasi35 / @ygree, is this something we need to have on v1.0.0 or can be added later?

I think we agreed that added later was acceptable.

ignasi35 avatar Jul 20 '20 09:07 ignasi35