Support B3 headers in OTEL propagation

[treyhyde]

Problem

Tracing is instrumental in supported distributed systems such as Knative Eventing, Kafka, etc. Many systems (including Istio), by default use B3 (multi) headers. Kafka Broker appears to be configured for only W3C headers. Traces involving Knative Eventing stop when it hits the broker or only start after messages hit other systems, you don't get an end-to-end trace when using B3

Persona:
Which persona is this feature for?

• Producer
• Consumer

Exit Criteria
Kafka Broker propagates B3 headers (in addition to W3C)

Time Estimate (optional):
How many developer-days do you think this may take to resolve?

Additional context (optional)
Add any other context about the feature request here.

[treyhyde]

FYI, I added

- name: OTEL_PROPAGATORS
   value: b3

to the receiver and dispatcher deployments to no effect.

[pierDipi]

Thanks for reporting!

We should be able to easily support it by injecting a propagator in https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L48
and use it in place of:

• https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L67
• https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L166
• https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L183

/good-first-issue

[knative-prow-robot]

@pierDipi:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Thanks for reporting!

We should be able to easily support it by injecting a propagator in https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L48
and use it in place of:

• https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L67
• https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L166
• https://github.com/knative-sandbox/eventing-kafka-broker/blob/67b507aeee9f3f88864a98f2be2dc56194051e71/data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/tracing/OpenTelemetryTracer.java#L183

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[arghya88]

/assign

[slinkydeveloper]

@arghya88 No need to fix it here, we're planning to use the new module directly from vertx eclipse-vertx/vertx-tracing#22
We'll just need to configure the tracer when using the new module

[slinkydeveloper]

Now that this is merged eclipse-vertx/vertx-tracing#22, we're going to implement this as soon as we update to vert.x 4.1 #784

[slinkydeveloper]

Just an heads up here. Once we merge #784 (hopefully soon), we need to import the sdk extensions and add this trace propagator: https://github.com/open-telemetry/opentelemetry-java/blob/50408d499f85d5761d0a5ed9bf9d77d5ff01fff5/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/B3Propagator.java

[skonto]

What is the support plan for the OTEL stuff at the vertx side long term? We heavily depend on vertx for most stuff, should not we use the otel java lib (directly)? Also should not we add this to the effort for migrating Eventing to OTEL?

[slinkydeveloper]

What is the support plan for the OTEL stuff at the vertx side long term?

Vert.x is going to support the vertx-opentelemetry integration, that is the implementation of the Tracing SPI, the rest is on us 😄

We heavily depend on vertx for most stuff, should not we use the otel java lib (directly)?

Why should we? The vertx-opentelemetry project is a contribution back to them started from this project, where we can help the greater vert.x community to use opentelemetry, and we can also profit back. There's no reason, nor any need, for us to do a custom implementation of the Tracing SPI. We use today vertx-opentelemetry, it works fine and fits our use case, so I don't see any need to change it.

Also should not we add this to the effort for migrating Eventing to OTEL?

For this specific eventing-kafka-broker project, there isn't anything left to do about this. Except perhaps this issue, which is not really related to otel but in general to support more tracing functionalities.

[skonto]

Vert.x is going to support the vertx-opentelemetry integration
There's no reason, nor any need, for us to do a custom implementation of the Tracing SPI.

I am referring to the first part. We should minimize deps (if possible) and from what I see across all Knative components there are different client libs, which makes integration life and debugging (I care about this) worse. From what I see we implemented the OTEL support in Vert.x (which uses otel java lib if not mistaken anyway) that is why I am asking about it :)

[slinkydeveloper]

Honestly, I don't get what are you referring to 😄 The opentelemetry library we use is the official one from the opentelemetry team.

What we do in this project, data plane wise, has nothing to do with the rest of knative, since we use Java, so we obviously have a different stack with different libs etc. In other words, if we would use opencensus here, nothing would change anyway because the java opencensus library is different from the golang opencensus library.

We should minimize deps (if possible)

I disagree, when talking about official vertx dependencies of the vertx stack. Those are well tested and fits together with vertx-core, so everything in there is good for us to use.

[slinkydeveloper]

Links to open-telemetry/opentelemetry-java#3364

[slinkydeveloper]

@treyhyde your goal is to emit b3 headers, or you just want the broker to recognize them and then still emit tracecontext?

[slinkydeveloper]

/assign

[treyhyde]

@treyhyde your goal is to emit b3 headers, or you just want the broker to recognize them and then still emit tracecontext?

The basic goal to propagate all tracing headers so we have basic insights into how the app is working. B3 is the defacto standard ... still (ignoring ACTUAL standards) as it is the default in sooo many places due to its long tenure. If I pass B3 I expect B3 on the other side. If I pass W3C, I expect W3C on the other side. In the same deployment I could easily see different apps using different header types and I would want all of them to work. (though, in this case, we use b3 universally since that is the Istio default). We switched to the kafka-channel with the standard broker because this worked out of the box.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.