Skip to content

[ISSUE #583]: 新增 扩展点用于实现链路追踪 - #735

Open
k631583871 wants to merge 1 commit into
apache:masterfrom
k631583871:issue-583
Open

[ISSUE #583]: 新增 扩展点用于实现链路追踪#735
k631583871 wants to merge 1 commit into
apache:masterfrom
k631583871:issue-583

Conversation

@k631583871

Copy link
Copy Markdown

#583

What is the purpose of the change

  • 新增 RocketMQMessageHandler 用户处理消息前预处理,可用于处理链路追踪
  • 新增 RocketMQMessagePostProcessor 类,用于处理消息发送前预处理消息,用来传递链路ID

…ocessor

- 新增 RocketMQMessageHandler 用户处理消息前预处理,可用于处理链路追踪
- 新增 RocketMQMessagePostProcessor 类,用于处理消息发送前预处理消息,用来传递链路ID

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

@RockteMQ-AI

Copy link
Copy Markdown

⚠️ Merge conflict detected

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-lease

This 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 RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Summary

This PR adds two extension points for tracing/observability in rocketmq-spring:

  1. Consumer side: RocketMQMessageHandler + RocketMQMessageHandlerChain — chain-of-responsibility pattern for pre-processing messages before the user's listener
  2. 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() uses MessageExt (v1 module) and MessageView (v5 module) as parameter types. The naming doHandler is slightly unconventional — consider handle() or onMessage() to align with common Java naming patterns (e.g., Spring's HandlerInterceptor).
  • [Warning] In the v5 module, RocketMQMessageHandler.doHandler() returns ConsumeResult, which means a handler in the chain can short-circuit consumption. This is powerful but could be surprising — the default no-op handler returns ConsumeResult.SUCCESS, so a misbehaving handler could silently swallow messages. Consider documenting this contract clearly.
  • [Info] The RocketMQMessageHandlerChain naming is a bit confusing since it's actually the chain parameter passed to the handler, not a collection of handlers. Consider naming it RocketMQMessageHandlerCallback or similar to clarify it's the continuation.
  • [Info] Tests are included for both the handler and post-processor extension points — good coverage.

Suggestions

  1. Consider adding Javadoc to RocketMQMessageHandler and RocketMQMessagePostProcessor explaining the intended use case (tracing/observability) and the contract (e.g., must call chain.doHandler() to continue).
  2. 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 RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 inside registerMessageListener which 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

  1. Add @Order annotation support for handlers to control execution sequence when multiple handlers are registered
  2. Consider adding a CompositeRocketMQMessageHandler utility class for common use cases (e.g., tracing + logging)
  3. 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

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