search

open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.5k stars 1.32k forks source link

Add B3 and Jaeger context propagators to the OPA distributed tracing. #4687

Open ievgenii-shepeliuk opened 2 years ago

ievgenii-shepeliuk commented 2 years ago

What part of OPA would you like to see improved?

Distributed tracing support.

Currently OPA distributed tracing supports only W3C context propagation header traceparent that is standard in Open Telemetry specification.,

So OPA instance creates/reports spans that use traceid and parent span values from traceparent header. But OTEL spec and W3C standard is still not completely supported everywhere, so support of older tracing headers that existed before OpenTelemetry specification will be very useful.

For example, Traefik - a widely adopted API gateway and K8s Ingress controller doesn't support OTEL / W3C headers, but supports B3 and Jaeger ones.

Describe the ideal solution

I was thinking about introduction of fallback mechanism via what is called "composite propagator" in OTEL specification. i.e. OPA would iterate via several supported header formats until find one that can extract parent trace info from headers

  1. try to find W3C traceparent header ( current behaviour)
  2. if not found then try to extract Jaeger's uber-trace-id
  3. if not found then try to extract B3 headers

Additional Context

  1. Existing Go libraries to use https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/propagators

  2. Probable place to inject propagators in OPA code https://github.com/open-policy-agent/opa/blob/main/internal/distributedtracing/distributedtracing.go#L122

The solution should only be injecting additional propagators together with current W3C one. There should not be a need to manually implement fallback.

stale[bot] commented 2 years ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

ievgenii-shepeliuk commented 2 years ago

go away bot

stale[bot] commented 2 years ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

stale[bot] commented 1 year ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

asleire commented 11 months ago

Istio also requires B3 headers for proper tracing

be-a-bee commented 10 months ago

Istio also requires B3 headers for proper tracing

@asleire , can you please elaborate ?

be-a-bee commented 10 months ago

@ashutosh-narkar is this feature request being considered for the near future ?

asleire commented 10 months ago

Istio also requires B3 headers for proper tracing

@asleire , can you please elaborate ?

Istio supports distributed tracing, but only with B3 headers. When using http.send to a service within the service mesh, OPA will include W3C headers but not B3 headers. Istio will thus be unable to detect the current traceId, and it will generate a new traceId and send it the receiving service as B3 headers. The service will now receive both W3C headers and B3 headers, each with different trace IDs. Which traceId the service uses depends on its configuration, but either way some trace data is lost

be-a-bee commented 10 months ago

@ashutosh-narkar , in our project we are using istio and other micro services are using b3 header propagation. Because OPA doesn't understand B3 headers we are losing trace data. Istio also doesn't understand W3C so that is also causing loss of trace data.

So adding B3 header support in OPA would solve both the above mentioned problems.

I did some intial research and feel B3 propagation also could be supported by updating the usage of NewCompositeTextMapPropagator https://github.com/open-policy-agent/opa-envoy-plugin/blob/63b3a2926c6063e81173212ef295c6a415c62b81/internal/internal.go#L132C1-L132C123

Would you accept a contribution for this issue ? Do you have any guidelines other than those mentioned in this issue description ?

I would stick to supporting B3 headers because we don't need 'Jaeger' propagation format in our project. But feel free to advise if Jaeger also needs to be supported as part of this issue.

anderseknert commented 10 months ago

Sounds like this would be a rather simple addition then, and without adding dependencies? If so, I'd just go ahead and write some code for submitting a PR :)

be-a-bee commented 10 months ago

@anderseknert , I already started working on this. So I can contribute a PR if you are fine with it.

stale[bot] commented 9 months ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

tilgovi commented 9 months ago

@be-a-bee did you get anywhere with this or post any branch? I'm interested in having extauthz checks attach to the parent trace.

stale[bot] commented 8 months ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

benhass commented 7 months ago

:+1: this would be helpful!

be-a-bee commented 7 months ago

@be-a-bee did you get anywhere with this or post any branch? I'm interested in having extauthz checks attach to the parent trace.

@tilgovi , yes made progress with this and tested in local. works fine.

Currently figuring out how to add automated tests to cover this change.

jithin-zac commented 6 months ago

In the context of Datadog, a similar issue arises due to its utilization of a distinct format for trace identifiers, as outlined in its documentation.

Even though the trace context is passed to OPA using the traceparent header, a new trace ID and span ID adhering to the W3C standard are generated instead of utilizing the provided context.

As a result, there exists a disparity between the Datadog trace context supplied and the trace context accessible in the decision logs.

@be-a-bee, regarding suggestions or potential solutions, have you encountered this scenario before and do you have any insights to share? Thanks in advance!

stale[bot] commented 5 months ago

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.