[ISSUE #583]: 新增 扩展点用于实现链路追踪 - #735
Conversation
…ocessor - 新增 RocketMQMessageHandler 用户处理消息前预处理,可用于处理链路追踪 - 新增 RocketMQMessagePostProcessor 类,用于处理消息发送前预处理消息,用来传递链路ID
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review: Approved ✅
PR: #735 — [ISSUE #583] Add extension points for distributed tracing
Type: Enhancement (21 files, +588/-29)
Assessment
Adds RocketMQMessageHandler for pre-processing before message handling and RocketMQMessagePostProcessor for pre-processing before message sending. Enables distributed tracing integration. Includes tests and supports both v4 and v5 client modules.
Verdict
✅ Well-structured extension point design with proper chain pattern. Good test coverage.
🤖 Automated review by oss-sentinel-ai
|
This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts: git fetch origin
git checkout issue-583
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adds two extension points for tracing/observability in rocketmq-spring:
- Consumer side:
RocketMQMessageHandler+RocketMQMessageHandlerChain— chain-of-responsibility pattern for pre-processing messages before the user's listener - Producer side:
RocketMQMessagePostProcessor— intercepts messages before sending to inject trace context
The changes are applied consistently across both rocketmq-spring-boot (v1) and rocketmq-v5-client-spring-boot (v5) modules.
Findings
- [Warning]
RocketMQMessageHandler.doHandler()usesMessageExt(v1 module) andMessageView(v5 module) as parameter types. The namingdoHandleris slightly unconventional — considerhandle()oronMessage()to align with common Java naming patterns (e.g., Spring'sHandlerInterceptor). - [Warning] In the v5 module,
RocketMQMessageHandler.doHandler()returnsConsumeResult, which means a handler in the chain can short-circuit consumption. This is powerful but could be surprising — the default no-op handler returnsConsumeResult.SUCCESS, so a misbehaving handler could silently swallow messages. Consider documenting this contract clearly. - [Info] The
RocketMQMessageHandlerChainnaming is a bit confusing since it's actually thechainparameter passed to the handler, not a collection of handlers. Consider naming itRocketMQMessageHandlerCallbackor similar to clarify it's the continuation. - [Info] Tests are included for both the handler and post-processor extension points — good coverage.
Suggestions
- Consider adding Javadoc to
RocketMQMessageHandlerandRocketMQMessagePostProcessorexplaining the intended use case (tracing/observability) and the contract (e.g., must callchain.doHandler()to continue). - The default no-op handler
(message, chain) -> chain.doHandler(message)is good — ensures backward compatibility.
Verdict
The feature addresses a real need (tracing integration) and the implementation is consistent across both modules. The concerns above are minor naming/documentation issues that don't block merging but would improve API clarity.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adds message handler and post-processor extension points for tracing support in both v3 and v5 Spring Boot integrations. Well-structured with proper chain pattern implementation and good test coverage.
Findings
-
[Warning]
rocketmq-spring-boot/src/main/java/org/apache/rocketmq/spring/support/DefaultRocketMQListenerContainer.java— The handler chain is applied insideregisterMessageListenerwhich is called during container initialization. If handlers are registered as Spring beans after container init, they may not be picked up. Consider documenting the bean registration order requirement or using lazy initialization. -
[Warning]
rocketmq-spring-boot/src/main/java/org/apache/rocketmq/spring/support/RocketMQMessageHandlerChain.java:67— The chain iteration uses index-based access (handlers.get(i)) which is O(n) for LinkedList. If handler chains are expected to be long, consider using an Iterator or converting to ArrayList in the constructor. -
[Info] Both v3 and v5 implementations are duplicated with near-identical code. Consider extracting a shared base class or utility to reduce maintenance burden.
-
[Info] Good test coverage for both handler and post-processor chains. The tests verify both single and multiple handler scenarios.
Suggestions
- Add
@Orderannotation support for handlers to control execution sequence when multiple handlers are registered - Consider adding a
CompositeRocketMQMessageHandlerutility class for common use cases (e.g., tracing + logging) - Document the extension points in the project README or wiki
Cross-repo Note
This change aligns with the tracing capabilities being added to the Node.js client (apache/rocketmq-clients#1368). Good to see consistent extension point design across language SDKs.
Automated review by github-manager-bot
#583
What is the purpose of the change