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.

Sign up for GitHub

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

Jump to bottom

Add Pulsar collector #3788

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

Add Pulsar collector #3788

wants to merge 3 commits into from

Conversation

CodePrometheus
Copy link

resolves #2297

  1. Add pulsar collector
  2. Add tests for all changes
  3. Add zipkin pulsar docker related files
  4. Add corresponding usage documents

@CodePrometheus
Copy link
Author

@reta @codefromthecrypt Excuse me, could you please help with the review when you have a moment, thanks.

@reta
Copy link
Contributor

reta commented Jan 25, 2025

@reta @codefromthecrypt Excuse me, could you please help with the review when you have a moment, thanks.

Thanks @CodePrometheus , did first round of review, looks solid! Few comments to look at, thank you

@CodePrometheus
Copy link
Author

@reta Thanks for your detailed review. I have made the corresponding fixes. Please feel free to take a look again when you have time.

reta
reta reviewed Jan 26, 2025
View reviewed changes
zipkin-collector/pulsar/src/main/java/zipkin2/collector/pulsar/LazyPulsarInit.java Outdated
.build();
} catch (Exception e) {
failure.set(CheckResult.failed(e));
throw new RuntimeException("Pulsar client create failed" + e.getMessage(), e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Pulsar client create failed" + e.getMessage(), e);
throw new RuntimeException("Pulsar client creation failed" + e.getMessage(), e);

reta
reta reviewed Jan 26, 2025
View reviewed changes
zipkin-collector/pulsar/src/main/java/zipkin2/collector/pulsar/LazyPulsarInit.java Outdated
// Nobody cares me.
}
failure.set(CheckResult.failed(e));
throw new RuntimeException("Pulsar unable to subscribe the topic(" + topic + "), please check the pulsar service.", e);
Copy link
Contributor

@reta reta Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Pulsar unable to subscribe the topic(" + topic + "), please check the pulsar service.", e);
throw new RuntimeException("Pulsar Client is unable to subscribe to the topic(" + topic + "), please check the service.", e);

reta
reta reviewed Jan 26, 2025
View reviewed changes
zipkin-collector/pulsar/src/main/java/zipkin2/collector/pulsar/LazyPulsarInit.java Outdated

void close() throws PulsarClientException {
PulsarClient maybe = result;
if (maybe != null) result.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we "release" the result as well?

result = null;

Copy link
Author

Choose a reason for hiding this comment

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

OK, that's good.

reta
reta reviewed Jan 26, 2025
View reviewed changes
zipkin-collector/pulsar/src/main/java/zipkin2/collector/pulsar/PulsarSpanConsumer.java
@Override public void close() {
try {
if (consumer != null) {
consumer.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I forgot to add the null assignment to the consumer:

Suggested change
consumer.close();
consumer.close();
consumer = null;

@reta
Copy link
Contributor

reta commented Jan 26, 2025 •
edited
Loading

@reta Thanks for your detailed review. I have made the corresponding fixes. Please feel free to take a look again when you have time.

Thank you @CodePrometheus , just a few very minor nits but LGTM otherwise, @codefromthecrypt do you have time to take a look? thank you.

@codefromthecrypt
Copy link
Member

wow! nice to see this coming around. To make sure this is usable, we need to have a sender for it. So, can you make a PR here following the existing conventions?

https://github.com/openzipkin/zipkin-reporter-java

I'll review more on this PR as I can.

codefromthecrypt
codefromthecrypt reviewed Jan 27, 2025
View reviewed changes
Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

one quick request is to pull the docker related parts into a new PR. This will allow us to merge that and reduce a "chicken or the egg" problem, where this PR depends on something published that it also uses. This also detangles reporter for the same reason.

@CodePrometheus
Copy link
Author

one quick request is to pull the docker related parts into a new PR. This will allow us to merge that and reduce a "chicken or the egg" problem, where this PR depends on something published that it also uses. This also detangles reporter for the same reason.

OK, let me remove the docker related changes from this PR, and I'll submit the pulsar related part in zipkin-reporter-java later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reta reta reta left review comments

@codefromthecrypt codefromthecrypt codefromthecrypt left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Pulsar transport
3 participants
@CodePrometheus @reta @codefromthecrypt