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

Migrate OpenTracing to OpenTelemetry #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tjiuming
Copy link
Contributor

Migrate OpenTracing to OpenTelemetry

@tjiuming tjiuming requested review from codelipenghui and a team as code owners February 27, 2023 14:23
@tjiuming
Copy link
Contributor Author

Tests to be completed

@codelipenghui
Copy link
Contributor

codelipenghui commented Mar 2, 2023

@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
Copy link
Contributor Author

tjiuming commented Mar 2, 2023

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: open-telemetry/opentelemetry-java-instrumentation#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
Copy link

asafm commented Mar 2, 2023

@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
Copy link
Contributor Author

tjiuming commented Mar 2, 2023

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
Copy link
Contributor

@tjiuming maintain in OTEL repo and then archive this repo

@codelipenghui
Copy link
Contributor

@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
Copy link
Contributor Author

tjiuming commented Mar 3, 2023

@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
Copy link

asafm commented Mar 6, 2023

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
Copy link

asafm commented Mar 6, 2023

@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
Copy link
Contributor Author

tjiuming commented Mar 9, 2023

Wait until open-telemetry/opentelemetry-java-instrumentation#8007 merged to avoid conflicts

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

Successfully merging this pull request may close these issues.

4 participants