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.

Sign up for GitHub

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

Jump to bottom

Adds RocketMQ plugin #1449

Merged
merged 3 commits into from
Feb 12, 2025
Merged

Adds RocketMQ plugin #1449

merged 3 commits into from
Feb 12, 2025

Conversation

CodePrometheus
Copy link
Contributor

@CodePrometheus CodePrometheus commented Feb 7, 2025 •
edited
Loading

Resolves #1043

Benchmark                                                     Mode     Cnt      Score     Error   Units
TracingProducerBenchmarks.send_baseCase                     sample  362740      0.018 ±   0.001   us/op
TracingProducerBenchmarks.send_baseCase:gc.alloc.rate       sample      15  12041.287 ± 915.439  MB/sec
TracingProducerBenchmarks.send_baseCase:gc.alloc.rate.norm  sample      15     64.000 ±   0.001    B/op
TracingProducerBenchmarks.send_baseCase:gc.count            sample      15    314.000            counts
TracingProducerBenchmarks.send_baseCase:gc.time             sample      15    113.000                ms
TracingProducerBenchmarks.send_baseCase:p0.00               sample                ≈ 0             us/op
TracingProducerBenchmarks.send_baseCase:p0.50               sample                ≈ 0             us/op
TracingProducerBenchmarks.send_baseCase:p0.90               sample              0.042             us/op
TracingProducerBenchmarks.send_baseCase:p0.95               sample              0.042             us/op
TracingProducerBenchmarks.send_baseCase:p0.99               sample              0.042             us/op
TracingProducerBenchmarks.send_baseCase:p0.999              sample              0.084             us/op
TracingProducerBenchmarks.send_baseCase:p0.9999             sample              0.883             us/op
TracingProducerBenchmarks.send_baseCase:p1.00               sample            142.080             us/op
TracingProducerBenchmarks.send_traced                       sample  438551      9.473 ±   0.570   us/op
TracingProducerBenchmarks.send_traced:gc.alloc.rate         sample      15   1321.673 ± 117.872  MB/sec
TracingProducerBenchmarks.send_traced:gc.alloc.rate.norm    sample      15  11941.827 ±  37.758    B/op
TracingProducerBenchmarks.send_traced:gc.count              sample      15     59.000            counts
TracingProducerBenchmarks.send_traced:gc.time               sample      15     24.000                ms
TracingProducerBenchmarks.send_traced:p0.00                 sample              5.536             us/op
TracingProducerBenchmarks.send_traced:p0.50                 sample              7.832             us/op
TracingProducerBenchmarks.send_traced:p0.90                 sample              8.912             us/op
TracingProducerBenchmarks.send_traced:p0.95                 sample             10.160             us/op
TracingProducerBenchmarks.send_traced:p0.99                 sample             24.832             us/op
TracingProducerBenchmarks.send_traced:p0.999                sample             88.505             us/op
TracingProducerBenchmarks.send_traced:p0.9999               sample           5059.209             us/op
TracingProducerBenchmarks.send_traced:p1.00                 sample          25296.896             us/op

@CodePrometheus
Copy link
Contributor Author

@codefromthecrypt Could you kindly review this when you have some time? Thank you!

codefromthecrypt
codefromthecrypt approved these changes Feb 8, 2025
View reviewed changes
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 taking this over @CodePrometheus. I think it needs to be in brave-bom and also added to beans to complete. Consider this approved when comments addressed. @reta if you don't mind covering next review pass.

instrumentation/benchmarks/src/test/java/brave/rocketmq/client/RocketMQProducerBenchmarks.java
@BenchmarkMode(Mode.SampleTime)
@OutputTimeUnit(TimeUnit.MICROSECONDS)
@State(Scope.Thread)
public class RocketMQProducerBenchmarks {
Copy link
Member

Choose a reason for hiding this comment

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

please add an example of this run in the PR desc in triple backtick

instrumentation/rocketmq-client/README.md Outdated
@@ -0,0 +1,98 @@
# brave-instrumentation-rocketmq-client
Copy link
Member

Choose a reason for hiding this comment

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

look at the other README examples, I don't recall if we are pasting everything including imports, or just the pertinant parts. I think the latter as it keeps the thing more readable and also we don't compile check the markdown. When reviewing, see if you may have any other ideas to improve the README, but nothing besides this comes to mind.

.../rocketmq-client/src/main/java/brave/rocketmq/client/TracingMessageListenerConcurrently.java
*/
final class TracingMessageListenerConcurrently extends AbstractMessageListener implements MessageListenerConcurrently {
final MessageListenerConcurrently messageListenerConcurrently;
final BiFunction<ConsumeConcurrentlyStatus, ConsumeConcurrentlyStatus, Boolean> CONCURRENT_CHECK =
Copy link
Member

Choose a reason for hiding this comment

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

I assume rocketmq is minimum java 8 so this will be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, rocketmq requires at least Java8.

instrumentation/rocketmq-client/src/main/java/brave/rocketmq/client/TracingSendMessage.java Outdated Show resolved Hide resolved
instrumentation/rocketmq-client/src/main/java/brave/rocketmq/client/TracingSendMessage.java Outdated Show resolved Hide resolved
instrumentation/rocketmq-client/src/main/java/brave/rocketmq/client/RocketMQTracing.java Outdated Show resolved Hide resolved
instrumentation/rocketmq-client/src/test/java/brave/rocketmq/client/RocketMQContainer.java
static final int BROKER_PORT = 10911;

RocketMQContainer() {
super(DockerImageName.parse("apache/rocketmq:5.3.1"));
Copy link
Member

Choose a reason for hiding this comment

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

I looked and this is multi-arch now. good! https://hub.docker.com/r/apache/rocketmq/tags

@reta
Copy link
Contributor

reta commented Feb 9, 2025

Thanks for taking this over @CodePrometheus. I think it needs to be in brave-bom and also added to beans to complete. Consider this approved when comments addressed. @reta if you don't mind covering next review pass.

Thanks a lot @CodePrometheus , a few very minor comments from me, solid work!

@CodePrometheus
Copy link
Contributor Author

@codefromthecrypt @reta Thanks for your time! I made a round of fixes for the current issue.

Benchmark results on my local are as follows:

Benchmark                                                     Mode     Cnt      Score     Error   Units
TracingProducerBenchmarks.send_baseCase                     sample  362740      0.018 ±   0.001   us/op
TracingProducerBenchmarks.send_baseCase:gc.alloc.rate       sample      15  12041.287 ± 915.439  MB/sec
TracingProducerBenchmarks.send_baseCase:gc.alloc.rate.norm  sample      15     64.000 ±   0.001    B/op
TracingProducerBenchmarks.send_baseCase:gc.count            sample      15    314.000            counts
TracingProducerBenchmarks.send_baseCase:gc.time             sample      15    113.000                ms
TracingProducerBenchmarks.send_baseCase:p0.00               sample                ≈ 0             us/op
TracingProducerBenchmarks.send_baseCase:p0.50               sample                ≈ 0             us/op
TracingProducerBenchmarks.send_baseCase:p0.90               sample              0.042             us/op
TracingProducerBenchmarks.send_baseCase:p0.95               sample              0.042             us/op
TracingProducerBenchmarks.send_baseCase:p0.99               sample              0.042             us/op
TracingProducerBenchmarks.send_baseCase:p0.999              sample              0.084             us/op
TracingProducerBenchmarks.send_baseCase:p0.9999             sample              0.883             us/op
TracingProducerBenchmarks.send_baseCase:p1.00               sample            142.080             us/op
TracingProducerBenchmarks.send_traced                       sample  438551      9.473 ±   0.570   us/op
TracingProducerBenchmarks.send_traced:gc.alloc.rate         sample      15   1321.673 ± 117.872  MB/sec
TracingProducerBenchmarks.send_traced:gc.alloc.rate.norm    sample      15  11941.827 ±  37.758    B/op
TracingProducerBenchmarks.send_traced:gc.count              sample      15     59.000            counts
TracingProducerBenchmarks.send_traced:gc.time               sample      15     24.000                ms
TracingProducerBenchmarks.send_traced:p0.00                 sample              5.536             us/op
TracingProducerBenchmarks.send_traced:p0.50                 sample              7.832             us/op
TracingProducerBenchmarks.send_traced:p0.90                 sample              8.912             us/op
TracingProducerBenchmarks.send_traced:p0.95                 sample             10.160             us/op
TracingProducerBenchmarks.send_traced:p0.99                 sample             24.832             us/op
TracingProducerBenchmarks.send_traced:p0.999                sample             88.505             us/op
TracingProducerBenchmarks.send_traced:p0.9999               sample           5059.209             us/op
TracingProducerBenchmarks.send_traced:p1.00                 sample          25296.896             us/op

@codefromthecrypt codefromthecrypt merged commit ec007eb into openzipkin:master Feb 12, 2025
4 checks passed
@codefromthecrypt
Copy link
Member

nice work!

@CodePrometheus CodePrometheus deleted the add-rocketmq-plugin branch February 12, 2025 02:43
@reta reta mentioned this pull request Feb 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codefromthecrypt codefromthecrypt codefromthecrypt approved these changes

@reta reta reta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

how can I use brave to trace the appication with RocketMQ?
3 participants
@CodePrometheus @reta @codefromthecrypt