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

[enhance][pulsar] add apache-pulsar client support #5926

Merged
merged 51 commits into from
Mar 7, 2023

Conversation

tjiuming
Copy link
Contributor

Fix: #2107

Motivation:
Support apache pulsar client from version 2.8.0 to lastest.

@tjiuming tjiuming requested a review from a team as a code owner April 23, 2022 22:56
@tjiuming
Copy link
Contributor Author

Tests to be completed.

@tjiuming
Copy link
Contributor Author

server.log

Here is the server log.

@laurit
Copy link
Contributor

laurit commented Apr 25, 2022

@tjiuming

[otel.javaagent 2022-04-24 17:46:23:622 +0800] [main] WARN muzzleMatcher - Instrumentation skipped, mismatched references were found: apache-pulsar [class io.opentelemetry.javaagent.instrumentation.pulsar.PulsarInstrumentationModule] on jdk.internal.loader.ClassLoaders$AppClassLoader@73d16e93
[otel.javaagent 2022-04-24 17:46:23:628 +0800] [main] WARN muzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pulsar.ConsumerImplInstrumentation:51 Missing class io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer
[otel.javaagent 2022-04-24 17:46:23:628 +0800] [main] WARN muzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pulsar.ConsumerImplInstrumentation:0 Missing class io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation
[otel.javaagent 2022-04-24 17:46:23:628 +0800] [main] WARN muzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pulsar.ConsumerImplInstrumentation:44 Missing class net.bytebuddy.matcher.ElementMatchers
[otel.javaagent 2022-04-24 17:46:23:628 +0800] [main] WARN muzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pulsar.ConsumerImplInstrumentation:51 Missing class net.bytebuddy.matcher.ElementMatcher
[otel.javaagent 2022-04-24 17:46:23:628 +0800] [main] WARN muzzleMatcher - -- io.opentelemetry.javaagent.instrumentation.pulsar.ConsumerImplInstrumentation:44 Missing class net.bytebuddy.matcher.ElementMatcher$Junction

muzzle is our safety checker that prevents runtime failures caused by transformation using types, methods etc that are not present. Your main problem is

private static final Tracer TRACER = PulsarTelemetry.tracer();

in ConsumerImplInstrumentation. ConsumerImplInstrumentation depends on bytebuddy classes that are only available inside agent code. In your advice code you should not use the instrumentation class.
It would be best if you'd study some of our existing messaging instrumentations and instead of using Tracer directly used our instrumenter api.

@mateuszrzeszutek
Copy link
Member

Hey @tjiuming ,

Please take a look at our contributing docs about adding a new instrumentation, writing an InstrumentationModule and using the Instrumenter API. You should be able to find answers to some of your questions there.
You can use the messaging package in from the Instrumenter API to implement telemetry for Pulsar - by using it you'll implement the OpenTelemetry messaging semantic conventions (well, for the most part) in your instrumentation. As Lauri has mentioned, you take a look at existing messaging instrumentations (e.g. spring-kafka, jms, rabbitmq, etc) and see how they're implemented.

@tjiuming
Copy link
Contributor Author

tjiuming commented Apr 25, 2022

@laurit @mateuszrzeszutek Many thanks for your reply. With your help, I’m already fixed these problems.
I‘m not familiar with byte-buddy api due to I was focus on skywalking agent plugin core mostly. My pulsar advices doesn't work because of incorrect byte-buddy annotation usages, thanks for your inspire.
Instrumentation api is more complex than Tracer api a lot for me, maybe I'll change it in the future, not currently.
Thanks for your help again!

@tjiuming
Copy link
Contributor Author

image

@tjiuming
Copy link
Contributor Author

@laurit Many thanks for your advice

@tjiuming
Copy link
Contributor Author

Tests to be completed.

@tjiuming
Copy link
Contributor Author

@trask @laurit Tests completed, PTAL

@tjiuming tjiuming requested a review from laurit July 12, 2022 16:47
@mateuszrzeszutek
Copy link
Member

Hey @tjiuming ,

Can you refactor your instrumentation so that is uses the Instrumenter API instead of plain Tracer? Currently all instrumentations in this repository make use of the Instrumenter API, as it's way easier for us to maintain them this way.

Also, please make sure that you implement appropriate OTel semantic convention -- in case of Pulsar I believe it's messaging semconv. If you use the Instrumenter API, there are several utilities that make implementing OTel semantic conventions way easier.

If you need to add custom attributes that are not present in the OTel messaging conventions then the usual pattern is to add a configuration flag, disabled by default, that controls whether they will be added to the produced telemetry or not. For example: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/hystrix-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hystrix/HystrixSingletons.java#L24-L27

@trask
Copy link
Member

trask commented Oct 23, 2022

hi @tjiuming! any objection to closing this (just trying to clean up the backlog a little), and you can re-open when you start working on it again? I think if I close it you won't be able to re-open it, but if you close it, I think you will be able to re-open yourself. And of course you can always ping on the closed PR and one of us will re-open it if that fails.

@tjiuming
Copy link
Contributor Author

@trask @mateuszrzeszutek @laurit I‘ve improved and fixed the tests, and all the CI checks passed, cloud you please help review?

@tjiuming tjiuming requested review from trask and removed request for laurit December 28, 2022 09:24
@KevinLiLu
Copy link

@tjiuming @trask @mateuszrzeszutek Any update on this? We are currently using OTel interceptors but this auto-instrumentation approach would be really awesome.

@tjiuming
Copy link
Contributor Author

tjiuming commented Feb 1, 2023

@KevinLiLu This PR is waiting for review

@trask
Copy link
Member

trask commented Feb 24, 2023

hi @tjiuming sorry for the long delay, we will try to get to this in the next week

@tjiuming
Copy link
Contributor Author

hi @tjiuming sorry for the long delay, we will try to get to this in the next week

Never mind, thanks!

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx @tjiuming!

@tjiuming
Copy link
Contributor Author

@trask I've fixed them, PTAL

@trask
Copy link
Member

trask commented Feb 27, 2023

@tjiuming can you check the test failures when you have a chance? thx!

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +32 to +34
// return hasSuperType(named("org.apache.pulsar.client.api.MessageListener"));
// can't enhance MessageListener here like above due to jvm can't enhance lambda.
return named("org.apache.pulsar.client.impl.conf.ConsumerConfigurationData");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you try MessageListener and it didn't work? there is some magic that should enhance lambdas: #4182

hasSuperType is a more expensive matcher compared to named however, so the current implementation could still be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, too. The current implementation works fine, so it needn't to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for your review!

@tjiuming
Copy link
Contributor Author

tjiuming commented Mar 6, 2023

@trask Could this PR be merged?

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.

Add Instrumentation Library for Apache Pulsar
6 participants