#354 | Metrics for ack, nack & unrouted publishes

[slayful]

Proposed Changes

See #354.

Types of Changes

• Bugfix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

This is currently a WIP.

[slayful]

@acogoluegnes this lacks tests, but can be considered complete if you ignore multiple (n)acks.

To support multiple (n)acks I'll create a separate PR. Would it possible to merge naive implementation w/o multiple confirms?

• delivery tag is incrementing for each message sent
• the broker can (n)ack a single message or multiple up til a specified delivery tag
• acks don't necessarily come in the same order

Possible implementation is to track ranges of acked or nacked messages and update the counters, in the following fashion:
1 ack -> [1:ack]
3 ack -> [1:ack, 3:ack]
2 ack -> [1-3ack]
4 nack -> [1-3ack, 4nack]
7 ack -> [1-3ack, 4nack, ?-7ack]
5 ack -> [1-3ack, 4nack, 5ack, ?-7ack]
6 ack -> [1-3ack, 4nack, 5-7ack]

When sufficient (higher than timeout for waiting for (n)ack) has elapsed, then possibly the structure can be simplified into just counters for nacks and acks plus the structure above for the newer ones. That would optimise for memory.

How's that sound for a solution?

[acogoluegnes]

Please add at least a test case with publisher confirms in the Metrics integration test. It can be a sunny test, with only publishing acknowledgments, publishing nack are hard to simulate. The fact multiple ack aren't handled shouldn't be a problem.

[acogoluegnes]

@slayful I'm not sure I'm following you on the multiple ack/nack support. But, yes, it can wait another PR, as long as the item in my review is addressed.
Going back to the multiple ack/nack support, note the delivery tags are channel-scoped, so you need to maintain a state for each channel. There's already such a state for handle delivery ack in AbstractMetricsCollector. Note also all this bookkeeping should be done only if the channel has enabled publisher confirm, another information to hold in the channel state.

[acogoluegnes]

@slayful Any update on this? The PR has still "WIP" in its title and a couple of comments to address, so I'm wondering.

[slayful]

Sorry, I was a little bit busier than usually. I should be able to address the feedback within this week. :)

[slayful]

I have yet to fix the test for unrouted messages, but when I do it should be ready for a second review.

[acogoluegnes]

@slayful Thanks for this contribution! @michaelklishin Any comments before merging?

[michaelklishin]

@acogoluegnes I think it's good to go.

[michaelklishin]

@acogoluegnes I think it's good to go.