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

Adds the pulsar reporter #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CodePrometheus
Copy link

Since openzipkin/zipkin#3788 has been merged, this PR implements pulsar-go-reporter

  1. Add tests for all changes
  2. All tests pass locally
  3. Update the corresponding documents

@codefromthecrypt
Copy link
Member

I won't have time to review this, possibly for 2 weeks due to time off. maybe ping someone else

@codefromthecrypt
Copy link
Member

@jamesmoessis mind helping do a review pass? I suspect we can cut a release tag once this is in. cc @basvanbeek in case you have any objections or additions to that.

@basvanbeek
Copy link
Member

I have no objections. I do think it might be a good idea to start adding modules to integrations like pulsar, kafka, amqp, etc.

No need for people to pull in dependencies when not using them.

@codefromthecrypt
Copy link
Member

@CodePrometheus mind starting the good dep hygiene practice with a go.mod for this one? In other projects I use a replace to the parent directory for the main library dep. Assume similar here unless someone else pipes up

@CodePrometheus
Copy link
Author

That's good, let me start adding modules, thanks to Bas for the suggestion!

@codefromthecrypt
Copy link
Member

If you feel industrious, you could also be the first to use an integration test here via testcontainers. Since this is a module there would be low risk on the dependencies. If you need an example of something similar in go I can probably dig one up

@CodePrometheus
Copy link
Author

If you feel industrious, you could also be the first to use an integration test here via testcontainers. Since this is a module there would be low risk on the dependencies. If you need an example of something similar in go I can probably dig one up

I have used testcontainers-go before and I'll try to complete this integration. If I run into the tricky issue, I'll bring it up here, thank you very much!

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.

3 participants