streamnative / pulsar-tracing

Tracing instrumentation for Apache Pulsar clients.
Apache License 2.0
20 stars 10 forks source link

Migrate OpenTracing to OpenTelemetry #19

Open tjiuming opened 1 year ago

tjiuming commented 1 year ago

Migrate OpenTracing to OpenTelemetry

tjiuming commented 1 year ago

Tests to be completed

codelipenghui commented 1 year ago

@tjiuming We(Asaf and I) discussed this one in yesterday's regular sync meeting. We'd better contribute the Pulsar instrumentation to https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation. And the OTel has defined the clear spec for messaging systems https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/messaging.md

Since this repo is initially based on OpenTracing, we should not continue this project instead of contributing to OTel. It will improve the visibility to users, and we will also get experienced advice from the OTel community.

@asafm @tjiuming I will create a product board for this one, since it's not a short-term task for now. So that we can involve the product team to understand the value of this project.

tjiuming commented 1 year ago

We(Asaf and I) discussed this one in yesterday's regular sync meeting. We'd better contribute the Pulsar instrumentation to https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation

For OpenTelemetry-java-instrumentation, I had already created a PR for it: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/5926 . And it was reviewed by the Otel community, and I think it could be merged in a few days.

Since this repo is initially based on OpenTracing, we should not continue this project instead of contributing to OTel. It will improve the visibility to users, and we will also get experienced advice from the OTel community.

Although openTelemetry-java-instrumentation is more powerful than this PR, but I still think we need to provide users with the ability to OTEL tracing, even if they don't use opentelemetry-java-instrumentation. opentelemetry-java-instrumentation is unsuitable for all application scenarios, such as high-performance computing.

@codelipenghui @asafm

asafm commented 1 year ago

@tjiuming I'm not familiar enough with the Java Agent of OTel. From what I gather, when you add support for any open source library, you have two options to do it:

  1. As a JavaAgent extension, which works by using byte code manipulation to inject that code into classes that were loaded. This means any JavaAgent user will get Tracing from Pulsar clients for free, without doing anything explicit.
  2. As a standalone library, which hooks into the open source library extensions to bridge OTel into it. In our case it's the interceptors.

The PR, you have linked, adds support for the JavaAgent version. In this PR, it seems that you are creating a standalone library.

What @codelipenghui meant is that you will add the standalone library also to opentelemetry-java-instrumentation repo.

Examples I saw: Kafka Streams for example is only JavaAgent: https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/kafka/kafka-streams-0.11/javaagent Kafka client for example is only library https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/kafka/kafka-clients/kafka-clients-2.6

This way you will also gain all the great reviews from OpenTelemetry folks which knows this project its best practices in and out.

Also, in the PR you linked you wrote integration tests using Testcontainers, and here there were none. That's the advantage you get when you write in OTel repo, they guide to write in certain way, which includes integration tests.

tjiuming commented 1 year ago

I got it. You mean that we can move this repo to https://github.com/open-telemetry/opentelemetry-java-instrumentation as a standalone library. Users could import this library to their applications to get the OTEL tracing ability without installing the java agent.

like: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/README.md

It makes sense. We can maintain the library in OTEL repo

@asafm @codelipenghui

codelipenghui commented 1 year ago

@tjiuming maintain in OTEL repo and then archive this repo

codelipenghui commented 1 year ago

@tjiuming @asafm I have contributed the tracing instrumentation to SkyWalking which follow the byte code manipulation. Hmm, IMO, it's not so good. Some method has changed in the Pulsar client, then we need to have new instrumentation in SkyWalking. The Pulsar client interceptor is better for compatibility, but users need to add the dependency.

tjiuming commented 1 year ago

@codelipenghui I think library is not as easy to use as agent. In fact, many users have no understanding of the middleware he is using, and the agent will be more foolish. Also, for users who are using agents, it doesn't seem reasonable to introduce an additional lib. They have different use scenarios. For business development, they should not feel these things

In a company, it is very difficult to promote the upgrading of basic components

asafm commented 1 year ago

I got it. You mean that we can move this repo to https://github.com/open-telemetry/opentelemetry-java-instrumentation as a standalone library. Users could import this library to their applications to get the OTEL tracing ability without installing the java agent.

like: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/README.md

It makes sense. We can maintain the library in OTEL repo

@asafm @codelipenghui

Exactly, yes.

asafm commented 1 year ago

@tjiuming @asafm I have contributed the tracing instrumentation to SkyWalking which follow the byte code manipulation. Hmm, IMO, it's not so good. Some method has changed in the Pulsar client, then we need to have new instrumentation in SkyWalking. The Pulsar client interceptor is better for compatibility, but users need to add the dependency.

@tjiuming Correct me if I'm wrong, but you need to add the specific dependency for Pulsar even in the case of using the Java Agent right? It's not that the agent packages ALL libraries it supports in agent mode, right?

@codelipenghui I don't like byte code manipulation as well. I'm not familiar enough with how the Java Agent motivation. I guess some users wants metrics with 0 effort. Just plug online and that's it. Java Agent allows.

Regarding byte code manipulation. It allows hands-free - just add agent and enjoy. In interceptor mode, you have to add that explicitly. Unless @tjiuming you add the interceptor via reflection?

tjiuming commented 1 year ago

Wait until https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/8007 merged to avoid conflicts