chore(messenger): use Messenger instrumentation #173
Conversation
src/Instrumentation/Symfony/Messenger/EventSubscriber/EndSpanEventSubscriber.php
Outdated
Show resolved
Hide resolved
src/Instrumentation/Symfony/Messenger/EventSubscriber/EndSpanEventSubscriber.php
Outdated
Show resolved
Hide resolved
| WorkerRunningEvent::class => ['endSpan'], | ||
| WorkerStoppedEvent::class => ['endSpan'], |
There was a problem hiding this comment.
We should not end a span on running/stopped worker events but on WorkerMessageReceived and WorkerMessageHandled events.
There was a problem hiding this comment.
ended on WorkerMessageHandledEvent. But I don't understand why it should also be ended on WorkerMessageReceived
There was a problem hiding this comment.
I am sorry I mean, start a span on WorkerMessageReceived event and end span on WorkerMessageHandled event.
src/Instrumentation/Symfony/Messenger/EventSubscriber/EndSpanEventSubscriber.php
Outdated
Show resolved
Hide resolved
| @@ -8,6 +8,9 @@ | |||
| use Symfony\Component\Messenger\Middleware\MiddlewareInterface; | |||
| use Symfony\Component\Messenger\Middleware\StackInterface; | |||
|
|
|||
| /** | |||
| * @deprecated: span should be created from event subscriber to avoid issue with orphan span | |||
| */ | |||
| class TraceableMessengerMiddleware implements MiddlewareInterface | |||
There was a problem hiding this comment.
I think there is misunderstanding here, we still need the middleware but maybe there is some adjustment to do.
You have to thing about the fact that a message can handled synchronously and it won't trigger worker events.
According to Symfony's documentation:
The middleware are executed when the message is dispatched but also again when a message is received via the worker (for messages that were sent to a transport to be handled asynchronously). Keep this in mind if you create your own middleware.
We need to find a way to either trace from the middleware when a synchronous message is dispatched or attach the trace context with a stamp when the message dispatched to the transport and trace it when the message is received and handled.
Here is the rolling of events:
- a message is dispatched
- the message goes through middlewares -> span start if sync
- the message is handled if synchronous -> span end if sync
- the message is sent to transport if asynchronous -> attach tracing context without span creation
- the message is received by the worker -> span start
- the message goes through middlewares -> no span creation
- the message is successfully handled by the worker -> span end
- the messagge has failed to be handled by the worker -> span end
By reviewing this, I think we could allow span creation within the middleware in an asynchronous context too as it will detail the every phases of the message, the dispatch phase and the handling phase as with synchronous message the dispatch/handling phase happen at the same time.
Still, we need to do some experiments, in each case to track down spans and make sure they are relevant, well linked and ended correctly.
Thanks again for your preliminary work on this, it's not an easy part and it will take some time to get the right implementation! This PR might also close this issue #168. |
…enger-instrumentation
jvocampings
changed the title
Ah I was talking about Instrumentation and not Propagation. |
…n on error and on message handled
|
@gaelreyrol I made some modifications related to what you asked. I'll be on holidays for 2 weeks. After that there are big changes at my work then I don't know if I'll be able to reactive. I'll do my best. |
It can't be done from the event and the
Thank you very much, don't worry at all, have a good holiday! I might work on your PR directly if I have enough time. |
| * Be aware if you start a span before this subscriber, it could leads to orphan span issue. | ||
| * Be sure your span is properly ended. | ||
| */ | ||
| class InstrumentationEventSubscriber implements EventSubscriberInterface |
There was a problem hiding this comment.
Since this subscriber is about worker events, I think we should name it WorkerMessageEventSubscriber.
|
|
||
| public function startSpan(WorkerMessageReceivedEvent $event): void | ||
| { | ||
| if (InstrumentationTypeEnum::Auto !== $this->instrumentationType) { |
There was a problem hiding this comment.
I'd like to adapt this to handle manual instrumentation, for example if the message has the #[Traceable] attribute, it should be instrumented.
WIP.
Related to issue #172
Encapsulate consumption of message in a Span. Based on Messenger events.
Missing instrumentation when a message is dispatched. Not sure how to implement it.
edit: I assume we could reuse TraceableMessengerTransport to decorate the method
send.