openzipkin / brave

Java distributed tracing implementation compatible with Zipkin backend services.
Apache License 2.0
2.35k stars 714 forks source link

Support rocketmq #1412

Open JoeKerouac opened 5 months ago

JoeKerouac commented 5 months ago

support RocketMQ see #1356

JoeKerouac commented 5 months ago

@codefromthecrypt

codefromthecrypt commented 5 months ago

ok I re-pushed into a single commit as we squash on merge anyway, and already spent the work to rebase this on master.

NEXT STEP:

The docker image for 5.1.4 is not multi-platform, so we need a multi-platform image or people like me cannot run docker tests (on apple silicon aka arm64). @JoeKerouac can you check on what is holding up https://github.com/apache/rocketmq-docker/issues/98?

codefromthecrypt commented 5 months ago

so this is the current error until there's a multi-platform image. Our other images we make on own, because they are shared with a zipkin server transport. As there's no rocketmq server transport, this one is special case and we need to address the docker image in some way, at least upstream would be good to find out why not.

[INFO] Running brave.rocketmq.client.ITRocketMQTracingTest
08:31:48,591 WARN  [main] containers.GenericContainer (GenericContainer.java:488) - The architecture 'amd64' for image 'apache/rocketmq:5.1.4' (ID sha256:f14b643f5d86369b50362c0039938cf0346f451b559d95f4f2c530083f9e579f) does not match the Docker server architecture 'arm64'. This will cause the container to execute much more slowly due to emulation and may lead to timeout failures.
[ERROR] Tests run: 6, Failures: 1, Errors: 5, Skipped: 0, Time elapsed: 34.80 s <<< FAILURE! -- in brave.rocketmq.client.ITRocketMQTracingTest
[ERROR] brave.rocketmq.client.ITRocketMQTracingTest.tracingMessageListenerConcurrently -- Time elapsed: 6.291 s <<< ERROR!
codefromthecrypt commented 5 months ago

@anuraaga @reta @shakuzen @jeqo I think this is a good example of us needing to add the new apple silicon runners and use them in the docker tests. If anyone feels up to task of github action slinging here and otherwise, please do! (can do an org search for testcontainers, but should be mostly zipkin, -dependencies, -reporter, here, brave-cassandra and the contrib storage things)

codefromthecrypt commented 5 months ago

nevermind about CI catching this as there's not yet a free runner that works with arm64 and docker https://github.com/orgs/community/discussions/69211

JoeKerouac commented 5 months ago

ok I re-pushed into a single commit as we squash on merge anyway, and already spent the work to rebase this on master.

NEXT STEP:

The docker image for 5.1.4 is not multi-platform, so we need a multi-platform image or people like me cannot run docker tests (on apple silicon aka arm64). @JoeKerouac can you check on what is holding up apache/rocketmq-docker#98?

I will do my best to resolve this issue; however, it may not be straightforward. I need some time, and there might not be a definite deadline for it.

JoeKerouac commented 5 months ago

However, regarding the current test case failure, I will address it as soon as possible.

JoeKerouac commented 5 months ago

@codefromthecrypt I don't know why the RocketMQ version in my code was upgraded from 4.6.0 to 5.1.4. Since there are significant differences between 5.1.4 and 4.x, the current goal is RocketMQ 4.6.0 instead of 5.1.4.

codefromthecrypt commented 5 months ago

@JoeKerouac I don't think we have ever not used a current version as the first version of instrumentation. Can you mention why there is an issue with 5.x or why 5.x is not desired? When we don't use current versions, we get security (CVE) problems to deal with, and also usually eventually have to support latest anyway. We don't likely want to have something both for rocket 4.x and 5.x

codefromthecrypt commented 5 months ago

for example, old and new dubbo caused a lot of work for the project. Not saying this is like dubbo, but dubbo was one of the most high maintenance and also slowest tests in the project. We don't want to repeat something like we had when we had 2 dubbos

JoeKerouac commented 5 months ago

@codefromthecrypt Since that's the case, I'll go ahead and upgrade to version 5.1.4; it's already at version 5.1.4 now.

codefromthecrypt commented 5 months ago

thanks @JoeKerouac. If there's a compatability issue with 5 against 4, we can add a maven-invoker-plugin test, but if it should work, let's just stick with 5.x.

codefromthecrypt commented 5 months ago

nice to see passing tests @JoeKerouac!

JoeKerouac commented 5 months ago

Sorry, I just finished the Chinese New Year holiday. I will handle it as soon as possible.

JoeKerouac commented 5 months ago

@codefromthecrypt I have completed all the modifications suggested during the code review.

codefromthecrypt commented 3 months ago

note: status is still the same since my last comments, except the license format has now changed and will need to be updated along with past comments here.