Support for RocketMQ #1356
Support for RocketMQ #1356
Conversation
|
@jcchavezs Pipeline failures that appear to be caused by expired license headers for old files |
|
@jcchavezs @jeqo @shakuzen license has been updated |
|
@jcchavezs @jeqo @shakuzen |
| * or implied. See the License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package brave.rocketmq.clients; |
There was a problem hiding this comment.
make this "client" to match the artifact name please
| import org.apache.rocketmq.common.message.MessageExt; | ||
|
|
||
| /** | ||
| * @author JoeKerouac |
| * @author JoeKerouac | ||
| * @date 2023-01-09 15:02 | ||
| */ | ||
| public class ConsumeMessageBraveHookImpl implements ConsumeMessageHook { |
There was a problem hiding this comment.
| public class ConsumeMessageBraveHookImpl implements ConsumeMessageHook { | |
| public class TracingConsumeMessageHook implements ConsumeMessageHook { |
There was a problem hiding this comment.
also does this need to be public? if not, make it package private
instrumentation/rocketmq/src/main/java/brave/rocketmq/clients/MessageConsumerRequest.java
Outdated
Show resolved
Hide resolved
| import java.util.Map; | ||
|
|
||
| /** | ||
| * @author JoeKerouac |
There was a problem hiding this comment.
look at our other packages and make JavaDoc and also a README similarly
instrumentation/rocketmq/src/main/java/brave/rocketmq/clients/SendMessageBraveHookImpl.java
Outdated
Show resolved
Hide resolved
| */ | ||
| class SpanUtil { | ||
|
|
||
| static <T extends MessagingRequest> Span createAndStartSpan(RocketMQTracing tracing, |
There was a problem hiding this comment.
maybe this can move into RocketMQTracing
|
|
||
| static final String EMPTY = ""; | ||
|
|
||
| static String getOrEmpty(String obj) { |
There was a problem hiding this comment.
move to RocketMQTracing
| * @author JoeKerouac | ||
| * @date 2023-01-10 15:19 | ||
| */ | ||
| public class TraceConstants { |
There was a problem hiding this comment.
look at how other packages define constants and use similar naming scheme
There was a problem hiding this comment.
I've been busy lately. I will deal with it in the next two days.
|
@codefromthecrypt Please help review again |
There was a problem hiding this comment.
Thanks.. I really want to help you land this, but we need to start smaller. Try to reduce the amount of code here, in tagging and tests to minimal. Also remove the spring option for now. If you can reduce the code to the size of other instrumentation, it will be easier for me to help you land this, then we can enhance it with less context.
instrumentation/rocketmq/pom.xml
Outdated
| <profile> | ||
| <id>netty-module</id> | ||
| <activation> | ||
| <jdk>[9,)</jdk> |
There was a problem hiding this comment.
minimum JDK of brave is 11 now, so you can make this a default property
instrumentation/rocketmq/pom.xml
Outdated
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <version>${maven-surefire-plugin.version}</version> | ||
| <configuration> | ||
| <argLine>${testArgLine}</argLine> |
There was a problem hiding this comment.
instead of adding this, add to properties what you want as maven-surefire-plugin.argLine because the below is in the parent pom now
<maven-surefire-plugin.argLine />
<maven-failsafe-plugin.argLine />
instrumentation/rocketmq/pom.xml
Outdated
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <groupId>net.orfjackal.retrolambda</groupId> |
There was a problem hiding this comment.
not needed as no longer used
instrumentation/rocketmq/pom.xml
Outdated
|
|
||
| <dependency> | ||
| <groupId>org.testcontainers</groupId> | ||
| <artifactId>testcontainers</artifactId> |
There was a problem hiding this comment.
use this and then in the tests that use docker, do something like this:
@Tag("docker")
@Testcontainers(disabledWithoutDocker = true)
@Timeout(60)
class ITRocketMQ {
@Container RocketMQContainer rocketmq = new RocketMQContainer();
| <artifactId>testcontainers</artifactId> | |
| <artifactId>junit-jupiter</artifactId> |
instrumentation/rocketmq/pom.xml
Outdated
| <main.java.version>1.6</main.java.version> | ||
| <main.signature.artifact>java17</main.signature.artifact> | ||
|
|
||
| <old-kafka-clients.version>1.0.0</old-kafka-clients.version> |
There was a problem hiding this comment.
remove anything about kafka
instrumentation/rocketmq/pom.xml
Outdated
| <module.name>brave.rocketmq.clients</module.name> | ||
|
|
||
| <main.basedir>${project.basedir}/../..</main.basedir> | ||
| <main.java.version>1.6</main.java.version> |
There was a problem hiding this comment.
this approach has changed. If rocketMQ is floor java 6, then copy style from another Java 6 thing like JMS. look carefully and setup pom the same way
|
|
||
| public static final class Builder { | ||
| final MessagingTracing messagingTracing; | ||
| String remoteServiceName = TraceConstants.ROCKETMQ_SERVICE; |
There was a problem hiding this comment.
inline this as it is easier. the rest will make more sense later.
| SpanUtil.createAndStartSpan(tracing, tracing.producerExtractor, tracing.producerSampler, | ||
| request, | ||
| msg.getProperties()); | ||
| span.name(TraceConstants.TO_PREFIX + msg.getTopic()); |
There was a problem hiding this comment.
look at the other messaging implementations, and notice that we do not add any non-default tags to the span. That's what customization is for, and it also significantly reduces the amount of code and constants to review.
Similar to KafkaTags start only with very very minimal, like pick 2 tags and delete everything else from this PR that isn't required to pass similarly
| } | ||
| } | ||
|
|
||
| @BeforeAll |
There was a problem hiding this comment.
per other note.. use @Container to manage these
There was a problem hiding this comment.
I have already resolved the above issue. Please help review
There was a problem hiding this comment.
sorry maybe I wasn't clear. I meant literally use @Container and like how it is done in https://github.com/openzipkin/brave/blob/master/instrumentation/kafka-clients/src/test/java/brave/kafka/clients/ITKafkaTracing.java
once this is done and the other comments including removing spring are, I'll review again, but not before. that way I don't need to repeat existing comments and you can have focus on what's needed to land this.
One thing is that once this is in, we'll likely have to publish our own image for rocketmq to avoid docker.io rate limiting, but I can help with that
There was a problem hiding this comment.
@codefromthecrypt I still don't quite understand your meaning. Regarding the use of @Container, I have already replaced @BeforeAll with @Container in the test cases. As for removing spring, are you referring to removing the spring-boot-rocketmq module?
There was a problem hiding this comment.
we currently do not have any code for spring boot, only minimal properties for some libraries whose primary abstraction is spring (spring-rabbit), so adding spring boot to this is more of a decision (e.g. why not micrometer/tracing, vs sleuth etc) Since all brave is low-level where possible, it is easy to make a "go" decision on the main rocketmq even if we don't put autoconfigure into this repo. Adding autoconfig here is adding more work forever to the maintainers and possibly in conflict with some other projects.
on testcontainers, it appears your last commit did what I mentioned.
So, let's remove the spring boot stuff from this PR and try to land it. If your PR is marked to allow edits by maintainers, I can help with small things after you remove the spring boot. sound ok?
There was a problem hiding this comment.
@codefromthecrypt I have removed the Spring part. and the PR has been marked to allow edits by maintainers.
|
Thanks @JoeKerouac! I have some heavy work activity in next two days, but will try to land it, if not definitely on Monday |
|
@JoeKerouac if you don't mind looking into the test failures before I go for review and style fixes, would be appreciated |
|
@codefromthecrypt Sure, I will handle it as soon as possible. |
|
I have fixed the failed test case. |
codefromthecrypt
force-pushed
the
master
branch
from
87c2c1f
to
21779e3
Compare
|
@JoeKerouac since you raised this PR from Later there is some follow-up. For example, the docker image is not multi-platform, and it needs to be multi-platform to proceed (at least amd64 and arm64, currently, it is single-platform arm64) |
|
I saved a copy of the commit here in case you lose it https://github.com/openzipkin/brave/compare/rocketmq-client |
Support for RocketMQ 4.6.0+.
see #1043