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

Feature Proposal: W3C Context propagation integration via OpenTelemetry #43

Open
ikavgo opened this issue Feb 2, 2022 · 14 comments · May be fixed by #272
Open

Feature Proposal: W3C Context propagation integration via OpenTelemetry #43

ikavgo opened this issue Feb 2, 2022 · 14 comments · May be fixed by #272
Assignees

Comments

@ikavgo
Copy link

ikavgo commented Feb 2, 2022

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.

  • I suggest to use headers as it is done for example here - https://github.com/knative-sandbox/eventing-rabbitmq/blob/main/cmd/ingress/main.go#L153 to carry context information.
  • I suggest to have same header names across all officially supproted RabbitMQ clients.
  • I argue that merely providing the header naming suggestion in the official docs will be a great step forward as it sets the ground for future development and solves compatibility issues immediately.

Feedback is much appreciated!

@lhoguin
Copy link

lhoguin commented Feb 2, 2022

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
Copy link
Contributor

Zerpet commented Feb 4, 2022

Related to #22

@BryanEllington
Copy link

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

@ikavgo
Copy link
Author

ikavgo commented May 2, 2023

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.

@Zerpet
Copy link
Contributor

Zerpet commented May 4, 2023

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
Copy link
Author

ikavgo commented May 4, 2023

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

@Zerpet
Copy link
Contributor

Zerpet commented May 4, 2023

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

@ikavgo
Copy link
Author

ikavgo commented May 4, 2023

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
Copy link
Contributor

Zerpet commented May 8, 2023

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:

  • Obtain a span from the context (link) (it is a no-op span if context doesn't have a Span)
  • Inject the Span attributes as message headers (see this and this)

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
Copy link
Author

ikavgo commented May 9, 2023

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
Copy link
Contributor

Zerpet commented May 9, 2023

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
Copy link

Hello, any updates here?

@lukebakken
Copy link
Contributor

@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.

AndrewWinterman added a commit to AndrewWinterman/amqp091-go that referenced this issue Jun 28, 2024
This PR extracts opentelemetry utility functions from my private project
and adds them to this project without calling them. It resolves rabbitmq#43

I'd like a broader discussion about whether these should be
automatically called by the library where possible, or if they should
simply be provided to clients to use if they so wish.

I did my best to follow OpenTelemetry semantic conventions as described
here
https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/,
but they are at times ambiguous for rabbitmq-- e.g. is the destination
for a message the Queue or the Consumer Tag the message was delivered to.

Given the channel based approaches of this library, it is impossible for
the library to know the full execution of a consumer. Unless
autoack=false, we cannot actually know when to end the span associated
with a delivery, so at least in the consumer case, it's probably best to
allow the client to manage spans for themselves.

We *can* manage spans on the producer side, and at the very least
extract span identifiers to include on published headers automatically,
and provide utilities for pulling them back out again.

My intention with putting this PR up is to move the conversation
forward. Because the PR *only* provides private methods (if I left
members public please call them out), it can be safely merged while
these questions are worked out.
@AndrewWinterman AndrewWinterman linked a pull request Jun 28, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

6 participants