brave
brave copied to clipboard
openzipkin
Support rocketmq
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?
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!
@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)
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
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.
However, regarding the current test case failure, I will address it as soon as possible.
@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.
@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
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
@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.
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.
Sorry, I just finished the Chinese New Year holiday. I will handle it as soon as possible.
@codefromthecrypt I have completed all the modifications suggested during the code review.
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.