I'd like to clarify my earlier comment about "re-throttled messages." After further review, I believe we can simplify our approach. The dispatch throttling doesn't actually track specific message IDs when throttling occurs, so we don't need to worry about counting individual messages.
Looking at the consolidated dispatch throttling method in PR 24012 (https://github.com/apache/pulsar/blob/efcf7c27b3f0fa5dc0252ce46f54b28a897e08bd/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L405-L430), I think we could implement a straightforward counter for dispatch rate limiter throttling events per subscription. For better observability, I'd suggest including a property in the metric that indicates the throttling reason (broker, topic, or subscription level rate limiter). This would eliminate the need for separate metrics while still providing all the necessary information in a single, clean metric. Baodi, I appreciate your work on improving observability for dispatch rate limiting, and I'm looking forward to seeing this enhancement move forward once we address these design considerations. -Lari On Thu, 27 Feb 2025 at 18:59, Lari Hotari <lhot...@apache.org> wrote: > > -1 (binding) > > I cannot approve this proposal in its current form due to a design > issue with the metric implementation that I previously raised in the > PR discussion. > > Issue: > The current implementation tracks messages that have already been > successfully dispatched to consumers, rather than counting messages > that were actually throttled. This means `dispatch_throttled_msg_count > `/`dispatch_throttled_bytes_count` would not measure throttled > messages/bytes as intended, but instead track messages and bytes that > were already successfully sent. > > The dispatch rate limiter throttling logic was recently consolidated > to a single point in the codebase through PR #24012. This is where the > metric should be implemented, rather than at the token consumption > point. > > When addressing this, the design needs to account for re-throttled > messages, as Baodi brought up in the PR discussion. It seems that we > have two options: > 1. Count each throttle event independently (simpler, but may count the > same message multiple times) > 2. Implement stateful tracking to count unique throttled messages > (more accurate, but more complex) > > I recommend we: > 1. Address this design issue in the implementation > 2. Update the PR accordingly > 3. Resume the vote once these corrections are made > > -Lari > > On Thu, 27 Feb 2025 at 15:37, Baodi Shi <ba...@apache.org> wrote: > > > > I would like to start voting thread for [improve][pip] PIP-406: > > Introduce metrics related to dispatch_throttled_count > > > > - PR: https://github.com/apache/pulsar/pull/23945 > > > > - Discuss thread: > > https://lists.apache.org/thread/w9pq1jp3f22prsr97mxzj1yhdoo1cnc4 > > > > > > Thanks, > > Baodi Shi
