Skip to content

Conversation

@pdimitra
Copy link
Contributor

@pdimitra pdimitra commented Jul 29, 2021

This PR enables the forwarding and participating of the w3c trace context propagation.
The w3c trace context is going to be propagated for all HTTP requests
Additional changes occurred in this PR, are including moving the "asgi" span type from SDK to Registered.
The tracer test suite is passing all tests related to w3c trace context.

Copy link

@scorphus scorphus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! 👋

Please consider my suggestions below.

@pdimitra pdimitra requested a review from andrewslotin August 5, 2021 12:11
Copy link

@andrewslotin andrewslotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. I did not check the complete flow, let's make sure it passes the compatibility test suite first :)

@pdimitra pdimitra marked this pull request as ready for review August 11, 2021 08:41
@pdimitra pdimitra requested review from andrewslotin and hmadison and removed request for andrewslotin August 11, 2021 11:21
Copy link
Contributor

@hmadison hmadison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to drop Pika support untill we have a clear idea if we want to support AMQP header propagation. Outside of that, I'm happy to see this work get finished and I think making a dev0 release and testing internally should be our next steps.

Copy link

@andrewslotin andrewslotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM implementation-wise, but I'm not sure about the introduction of new propagator formats. Would it be possible to update the existing ones?

Copy link

@andrewslotin andrewslotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest we make W3C trace context support opt-out instead of opt-in. Otherwise LGTM

Copy link

@andrewslotin andrewslotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pdimitra pdimitra merged commit 2d4c9ca into master Sep 2, 2021
@pdimitra pdimitra deleted the w3c-trace-context branch September 2, 2021 04:05
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 this pull request may close these issues.

5 participants