rabbitmq / amqp091-go

An AMQP 0-9-1 Go client maintained by the RabbitMQ team. Originally by @streadway: `streadway/amqp`
Other
1.54k stars 138 forks source link

Feature Proposal: W3C Context propagation integration via OpenTelemetry #43

Open ikavgo opened 2 years ago

ikavgo commented 2 years ago

I notice increasing demand/expectations from user for built-in integration with Distributed tracing. One such example is Knative where Distributed tracing was integrated from the start.

Some time ago two major parties - OpenCensus and OpenTracing merged to OpenTelemetry and now Tracing API considered mature - https://github.com/open-telemetry/opentelemetry-go.

As a first step towards full Distributed tracing support I propose to integrate Tracing context propagation.

Feedback is much appreciated!

ikavgo commented 2 years ago

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/messaging.md

lhoguin commented 2 years ago

I think it's fair to start with standardising a header name or two for this purpose. But I would like to know how that would look like across all the protocols we currently support (AMQP0.9.1, AMQP1.0, MQTT, STOMP and Stream).

Zerpet commented 2 years ago

Related to #22

BryanEllington commented 2 years ago

Also see https://www.w3.org/TR/trace-context/ and https://w3c.github.io/trace-context-amqp/

ikavgo commented 1 year ago

with this addition https://github.com/rabbitmq/amqp091-go/pull/96 publishing interface becomes really nice - we don't have to use carriers/propagator concept for publishing and just pass our context.

Zerpet commented 1 year ago

with this addition #96 publishing interface becomes really nice - we don't have to use carriers/propagator concept for publishing and just pass our context.

Is the idea to extract the tracestate and traceparent information from the context? And automagically add those values as message headers?

In this case, I think there should be an opt-in/opt-out mechanism.

ikavgo commented 1 year ago

I don't think we need something special here. IIRC by default the propagator is no-op.

Zerpet commented 1 year ago

I'm not following. If the propagator is a no-op, how are we going to achieve context propagation, as per the spec?

ikavgo commented 1 year ago

by configuring OT SDK to use "proper" propagator :-) For example: https://github.com/open-telemetry/opentelemetry-go/blob/e6839571d26cfc6458050da275f79618fbfd4d9d/propagation.go#L30. It's up to our users to set configure. We going to use only API part.

Zerpet commented 1 year ago

Sure, I'm not suggesting we use the SDK and configure a specific implementation. My question is whether we are simply going to mimic KNative behaviour (link from OP). In other words, what KNative does is:

As a first step, is our intention to do the same in this client?

Are we going to introduce the headers suggested in this spec?

ikavgo commented 1 year ago

https://github.com/rabbitmq/amqp091-go/compare/main...ik-tracecontext-bump simplest way of doing context injection into message. On consume/get side the call to otel.GetTextMapPropagator().Extract(ctx, msg) will be needed.

Key names, for headers, their format is up to propagator.

Zerpet commented 1 year ago

I understand that the scope of this issue is simply to propagate the context, and any values set by the user, via message headers. We implement the TextMapPropagator in the publishing message, so our struct knows how to propagate. Is that right?

Automatic instrumentation, like go-pg/pg is out of the scope of this issue. Correct?

maxim-badarau-m10 commented 1 year ago

Hello, any updates here?

lukebakken commented 1 year ago

@maxim-badarau-m10 this issue is still open, so no, it has not been implemented. There is no reason to ask if there are updates.

If this is a critical feature, please contribute to this library or indicate that you have a RabbitMQ support agreement.