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

Support rocketmq #1412

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

Conversation

JoeKerouac
Copy link

support RocketMQ
see #1356

@JoeKerouac
Copy link
Author

@codefromthecrypt

@codefromthecrypt codefromthecrypt force-pushed the support_rocketmq branch 2 times, most recently from 01266d6 to 6c23605 Compare February 1, 2024 01:08
Signed-off-by: Adrian Cole <[email protected]>
@codefromthecrypt
Copy link
Member

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?

@codefromthecrypt
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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

@JoeKerouac
Copy link
Author

@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
Copy link
Member

@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
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

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.

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.

getting closer! After these changes will need another pass, and then I will start asking for and suggesting non IT unit tests, using examples from kafka or jms where we have them

@codefromthecrypt
Copy link
Member

nice to see passing tests @JoeKerouac!

@JoeKerouac
Copy link
Author

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

@JoeKerouac
Copy link
Author

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

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.

Thanks for the progress! Besides notes mentioned, please audit the other messaging libraries and look for IT tests you may have missed. For example, custom sampling and continuing a trace. You can look at least at kafka-clients to see what is left out.

Also, has there been movement on a multi-platform image? Once you get the current docker setup simpler (ideally no custom shell script), we can look into making our own image, as we can't release this until it can run on arm64.

}

function createTopic() {
sh /home/rocketmq/rocketmq-5.1.4/bin/mqadmin updateTopic -n 127.0.0.1:9876 -b 127.0.0.1:10911 -t $1
Copy link
Member

Choose a reason for hiding this comment

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

is there no way to do this in java?

sh /home/rocketmq/rocketmq-5.1.4/bin/mqbroker -n 127.0.0.1:9876 -c /broker.conf > ~/broker.log 2>&1 &
check "boot success" ~/broker.log

createTopic testSend
Copy link
Member

Choose a reason for hiding this comment

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

the better way is to use less shell scripting and set these up in java, even if java uses a shell command to create the topic. Then, there is less maintenance.

Here's an example https://github.com/openzipkin/zipkin-reporter-java/blob/master/amqp-client/src/test/java/zipkin2/reporter/amqp/RabbitMQContainer.java#L44-L55

createTopic tracingMessageListenerOrderly
createTopic testAll

echo "--JoeKerouac--"
Copy link
Member

Choose a reason for hiding this comment

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

this is cute but we don't want to echo individuals in the logs ;)


echo "--JoeKerouac--"

while true
Copy link
Member

Choose a reason for hiding this comment

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

seems odd that the container doesn't itself stay alive. Maybe when topic setup is moved to java/testcontainers side, you don't need this anymore?

@@ -0,0 +1,3 @@
brokerName=JoeKerouac-Test
Copy link
Member

Choose a reason for hiding this comment

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

use java to setup these things and leave the broker to default or just "test" or something because it is in a container so there should be no conflict.


// TODO: maybe like HttpTags.PATH if we support extended tags on first version
public static final String ROCKETMQ_TAGS = "rocketmq.tags";
public static final String ROCKETMQ_TOPIC = "rocketmq.topic";
Copy link
Member

Choose a reason for hiding this comment

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

delete this file as we will only start with rocketmq.topic

import java.util.Map;

public class RocketMQTracing {

Copy link
Member

Choose a reason for hiding this comment

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

note this won't be public, and this is like jms, rabbit, etc.

Suggested change
static final String ROCKETMQ_TOPIC = "rocketmq.topic";


import java.util.Map;

public class RocketMQTracing {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class RocketMQTracing {
/**
* Use this class to decorate your RocketMQ consumer / producer and enable Tracing.
*
* @since 6.1
*/
public final class RocketMQTracing {

private static final int defaultDelayLevelWhenNextConsume = 0;

public static RocketMQTracing create(Tracing tracing) {
return new RocketMQTracing(MessagingTracing.create(tracing), RocketMQTags.ROCKETMQ_SERVICE);
Copy link
Member

Choose a reason for hiding this comment

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

don't pass around this constant.. you'll see why, but it is simpler and less parameters

String remoteServiceName) {
return new RocketMQTracing(messagingTracing, remoteServiceName);
}

Copy link
Member

Choose a reason for hiding this comment

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

Main thing is to be similar to other code, look at jms and you'll see this.. adopt that and adapt it for rabbit. This makes it simpler for configuration tools who don't need to look for different conventions depending on the library.

  public static JmsTracing create(Tracing tracing) {
    return newBuilder(tracing).build();
  }

  public static JmsTracing create(MessagingTracing messagingTracing) {
    return newBuilder(messagingTracing).build();
  }

  public static Builder newBuilder(Tracing tracing) {
    return newBuilder(MessagingTracing.create(tracing));
  }

  public static Builder newBuilder(MessagingTracing messagingTracing) {
    return new Builder(messagingTracing);
  }

  public static final class Builder {
    final MessagingTracing messagingTracing;
    String remoteServiceName = "jms";

    Builder(MessagingTracing messagingTracing) {

@codefromthecrypt
Copy link
Member

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.

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.

2 participants