Metrics for successful and unsuccessful outgoing publishes

[slayful]

Hey there! This is a feature request I'd be willing to create PR for.

As it stands now, when you're publishing a message and it is successful, the published counter in MetricsCollector will be incremented.
I'd like to add publishFailed counter, which will be incremented if and when publishing attempt fails.

This will allow users to easily create reports of publishing success rates and help them determine if the number messages they're not publishing are a significant enough to increase resiliency of their programs.

I'd like to find out if this is a feature that would be a welcome addition and if the way I'd like to add it would meet your expectations.

[michaelklishin]

Unfortunately it's not obvious how to know if a publishing attempt failed. Not all failures result in timely I/O exceptions.

[michaelklishin]

@acogoluegnes we can count (I/O and other) exceptions on publish attempts and negative publisher acknowledgements. This would not account for publishes that technically were written to the socket but were never acknowledged because connection went down right after. WDYT?

Having two categories is a better idea than lumping everything together, given that there's at least one other category which it or may not be possible to reliably count.

[michaelklishin]

Another question is whether returned (unroutable) messages should count for "unsuccessful". Again, I think it should be a separate category just like it is in the broker.

[acogoluegnes]

Sounds reasonable to me. If we sum up:

• publish exceptions (failures/exceptions in basicPublish)
• publisher acks (ConfirmListener#handleAck)
• publisher nacks (ConfirmListener#handleNack)
• unroutable deliveries (ReturnListener#handleReturn)

We may add new methods to MetricsCollector, but then the change would need to go into 6.0.0. We may also create a new interface, e.g. PublishingMetricsCollector that the current collector implementations would implement and release this in the 5.x branch as there would be no breaking changes (not really sure about that actually, but it's worth investigating). With the latter, the metrics collecting logic may be tricky, as we would need to juggle with new interfaces.

[michaelklishin]

I took the liberty to edit @acogoluegnes' comment to clarify some proposed metric names.

[slayful]

@michaelklishin, @acogoluegnes I've implemented the first and most basic type of failure, would you mind giving it a look?
I was thinking that after addressing the CR for ths PR, we could merge it and I could add the other types in other PRs to keep them on the smaller side, how's that sound?

[michaelklishin]

@slayful thank you, we will next week. It just so happens that both Java client maintainers on the core team are on PTO this week. Having follow-up PRs with other improvements (even if they depend on this one) is what we'd prefer.

[slayful]

Thanks @michaelklishin! :)

[slayful]

Seems like we could merge #357 and I could add the other types of failures.

[slayful]

#360 is ready for a second review. :)

[michaelklishin]

@slayful thank you!

[slayful]

Thank you for merging and helping me do this. We'd love to start monitoring our publishing rates, so please don't mind me asking, do you have an idea when will it be part of a release?

[michaelklishin]

Release plans for 6.0 haven't been discussed yet but I think a month or so is realistic. We certainly can release a milestone some time this week.

[acogoluegnes]

We can indeed release a milestone in the next few days. We still have a few things to address in 6.0.

[acogoluegnes]

Note I created a follow-up issue to handle multiple publisher confirms in metrics.

[acogoluegnes]

@slayful 6.0.0.M1 is out: https://groups.google.com/d/msg/rabbitmq-users/yGsBB8Rom2w/leIkIwv7AgAJ

[slayful]

Thanks! :)