pulsar-tracing icon indicating copy to clipboard operation
pulsar-tracing copied to clipboard

Migrate OpenTracing to OpenTelemetry

Open tjiuming opened this issue 1 year ago • 11 comments

Migrate OpenTracing to OpenTelemetry

tjiuming avatar Feb 27 '23 14:02 tjiuming

Tests to be completed

tjiuming avatar Feb 27 '23 14:02 tjiuming

@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.

codelipenghui avatar Mar 02 '23 02:03 codelipenghui

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

tjiuming avatar Mar 02 '23 05:03 tjiuming

@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.

asafm avatar Mar 02 '23 10:03 asafm

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

tjiuming avatar Mar 02 '23 11:03 tjiuming

@tjiuming maintain in OTEL repo and then archive this repo

codelipenghui avatar Mar 03 '23 03:03 codelipenghui

@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.

codelipenghui avatar Mar 03 '23 03:03 codelipenghui

@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

tjiuming avatar Mar 03 '23 03:03 tjiuming

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 avatar Mar 06 '23 10:03 asafm

@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?

asafm avatar Mar 06 '23 11:03 asafm

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

tjiuming avatar Mar 09 '23 09:03 tjiuming