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

issue-4687 : Add B3 trace header propagation support #513

Conversation

be-a-bee
Copy link
Contributor

@be-a-bee be-a-bee commented Feb 6, 2024

add b3 propagators module and use it
add test to assert that x-b3-traceid is recognized by opa and traceid is logged in decision logs

@be-a-bee
Copy link
Contributor Author

be-a-bee commented Feb 6, 2024

@ashutosh-narkar , can you please review and let me know your comments.

@ashutosh-narkar
Copy link
Member

Hey @be-a-bee as mentioned on the #512 review, we should try to limit the changes to adding the B3 propagators and avoid vendor updates. Is this not possible for some reason?

Signed-off-by: Praneeth Talishetti <47842333+be-a-bee@users.noreply.github.com>
@be-a-bee be-a-bee force-pushed the issue-4687-support-b3-propagation branch from d6275c2 to ada7036 Compare February 7, 2024 07:15
@be-a-bee
Copy link
Contributor Author

be-a-bee commented Feb 7, 2024

@ashutosh-narkar , b3 module 1.22 required other vendor updates. I set the b3/propagators version to 1.21 and avoided other vendor updates.

please check now and let me know if anything.

Comment on lines +123 to +129
ctx = metadata.AppendToOutgoingContext(ctx, "x-b3-parentspanid", "2a2b3c4d5e6f7a8b")
ctx = metadata.AppendToOutgoingContext(ctx, "x-b3-traceid", exampleTraceID)
ctx = metadata.AppendToOutgoingContext(ctx, "x-b3-spanid", "3f6a0b6d9d5f4b45")
ctx = metadata.AppendToOutgoingContext(ctx, "x-b3-sampled", "1")
ctx = metadata.AppendToOutgoingContext(ctx, "X-B3-Flags", "1")
ctx = metadata.AppendToOutgoingContext(ctx, "X-B3-Baggage-User", "alice")
ctx = metadata.AppendToOutgoingContext(ctx, "X-B3-Baggage-Transaction", "12345")
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a reference from where these headers come from? It would helpful to add it as a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Thanks @be-a-bee!

@ashutosh-narkar ashutosh-narkar merged commit 0d06ee7 into open-policy-agent:main Feb 8, 2024
8 checks passed
@be-a-bee
Copy link
Contributor Author

be-a-bee commented Feb 9, 2024

Thank you @ashutosh-narkar :)

@@ -134,11 +135,11 @@ func New(m *plugins.Manager, cfg *Config) plugins.Plugin {
if m.TracerProvider() != nil {
grpcTracingOption := []otelgrpc.Option{
otelgrpc.WithTracerProvider(m.TracerProvider()),
otelgrpc.WithPropagators(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{})),
otelgrpc.WithPropagators(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}, b3.New(b3.WithInjectEncoding(b3.B3MultipleHeader)))),

Choose a reason for hiding this comment

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

@be-a-bee Why are we only considering B3MultipleHeader? b3 standard also defines B3SingleHeader

Ideally, b3 propagator should be instantiated something like this

b3.New(b3.WithInjectEncoding(b3.B3MultipleHeader | b3.B3SingleHeader))

Copy link
Member

Choose a reason for hiding this comment

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

@be-a-bee can you please address this and push an update if needed? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sarcastic-sher , I was mainly focusing on b3 trace header extraction and b3multiple header injection . Including single header injection is also important.

@ashutosh-narkar , will create a new PR for b3 single header injection as well.

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