Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context.Context support to StandardRouter message routing #286

Open
johndeng opened this issue Dec 11, 2024 · 4 comments
Open

Add context.Context support to StandardRouter message routing #286

johndeng opened this issue Dec 11, 2024 · 4 comments

Comments

@johndeng
Copy link

Currently using paho.StandardRouter for message routing but it lacks context.Context support.

Adding context support would allow applications to integrate distributed tracing (OpenTelemetry) much easier.

@MattBrittan
Copy link
Contributor

Could you please add an example showing how this would work? My main question is "what context will the library pass into Route(context.Context, *packets.Publish)? As contexts are frequently used for cancellation signals this has the potential to cause some confusion (should the Context be terminated when the library is disconnecting, what should a message handler do in that situation etc).

An alternative would be for you to inject a Context yourself i.e. (pseudo code):

r.RegisterHandler("foo", func(p *Publish) { doSometing(someContext, p)  })

@johndeng
Copy link
Author

Thanks for reporting this issue. I'd like to clarify how tracing typically works in this context:

The router.Route() usually serves as the entry point when messages are received. Here's a common pattern:

func (svc *Service) onPublishReceived(p paho.PublishReceived) (bool, error) {
  ctx, span := tracer.Start(context.Background(), "message_route") // root span
  defer span.End()
  m.router.Route(ctx, p.Packet.Packet())
  return true, nil
}

func (svc *Service) defaultHandler(ctx context.Context, p *paho.Publish) {
  ctx, span := tracer.Start(ctx, "default_handler")
  defer span.End()

  doWork(ctx) // continue propagating the span
}

The flow works as follows:

  1. A root span is created at message reception
  2. The context is passed through router.Route()
  3. Handlers can create child spans
  4. Business logic can continue the trace propagation

The key point is that handlers have the flexibility to opt into tracing - the context parameter enables them to participate in the trace chain from the router's entry point through to completion, but they're not required to do so. This gives developers the freedom to integrate with tracing where it makes sense for their use case.

Another important consideration is to distinguish between application-level and message dispatcher-level contexts. This distinction helps in understanding the scope and responsibility of context propagation through different layers of the system.

We could potentially enhance the router's tracing support to make this pattern even more straightforward to implement.

@MattBrittan
Copy link
Contributor

The flow works as follows:

Thanks, that makes sense.

I think my confusion re your PR was due to the changes needed due to ClientConfig.Router. This was left in for backwards compatiblity when onPublishReceived was added; we discussed removing it but ended up flagged it as "Depreciated". Because of this your change requires doing something that smells bad here as well as breaking any code that relies on the current Router.

As such, I think your change (as is) would need to be contingent upon us removing the depreciated ClientConfig.Router.

We could do this, but it will break users code, and I'm not sure how beneficial it would be (given that Router is pretty short). I wonder if an alternative would be to introduce a RouterContext that implements the functionality you need without breaking other users? This would lead to some duplication of code, but that's something I think is acceptable if it means we can avoid the breaking change (potentially StandardRouter could be flagged as depreciated and examples updated to use the new router).

@johndeng
Copy link
Author

Thanks for the explanation. I apologize for not considering the backward compatibility impact earlier.

Let me think about how to adjust the approach - perhaps introducing a new Router interface, as you suggested, would be a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants