Add Pulsar collector#3788
Conversation
|
@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 |
|
@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. |
|
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? I'll review more on this PR as I can. |
codefromthecrypt
left a comment
codefromthecrypt
left a comment
There was a problem hiding this comment.
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 |
|
ok also make sure you do raise another PR for the docker image here, as that needs to merge before this and the one for reporter (to get the sequence correct). meanwhile happy new year! |
Since there is no collector-pulsar module at present, compiling docker will result in an error, so when this PR is merged, I will add `zipkin-collector-pulsar` to the github actions in #3788.
|
ok I re-pushed the 3.4.3 tag and marked zipkin-pulsar public. you can use it here and in the reporter when ready https://github.com/orgs/openzipkin/packages/container/package/zipkin-pulsar |
|
All have been updated, please take a look again when you have time, thx. |
|
good job. next release of zipkin should be a minor bump as there's a new feature to announce. We should ready the reporter's sender for this prior to that release so that it is clear you can use this transport. |
3 participants
resolves #2297