Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjusting the message count logic when using includeUnacked #781

Merged
merged 2 commits into from
May 5, 2020

Conversation

tbickford
Copy link
Contributor

@tbickford tbickford commented Apr 25, 2020

When using the RabbitMQ Scaler's includeUnacked setting to true I noticed the message count appeared to be off when I added some logging of the value of the external metric (the total message count). The value (message count) appeared to be double counting the unacknowledged message count. The current logic (before this PR) is to include the messages (count) in addition to the messages_unacknowledged (count). It appears that the HTTP Management URI messages attribute already includes the unacknowledged messages in it's value. Here is an example of some json from the HTTP Management URI:

    "messages": 498,
    "messages_details": {
      "rate": 0
    },
    "messages_paged_out": 0,
    "messages_persistent": 498,
    "messages_ram": 498,
    "messages_ready": 496,
    "messages_ready_details": {
      "rate": 0
    },
    "messages_ready_ram": 496,
    "messages_unacknowledged": 2,
    "messages_unacknowledged_details": {
      "rate": 0
    },

Note - messages_ready is 496, messages_unacknowledged is 2. The messages field is 498 (the addition of the two fields) which is what the includeUnacked setting is supposed to reflect from my understanding.

For further reference see https://www.rabbitmq.com/monitoring.html#queue-metrics messages field name - Total number of messages (ready plus unacknowledged)

…unt currently included unacknowledged messages

Signed-off-by: Travis Bickford <tbickford@shutterstock.com>
Signed-off-by: Travis Bickford <tbickford@shutterstock.com>
@zroubalik
Copy link
Member

@tbickford thanks. So to be clear, based on your example it currently returns 500 (498+2) instead of 498 (496+2)?

@holyketzer ^^

@holyketzer
Copy link
Contributor

holyketzer commented Apr 27, 2020

Oh, good catch @tbickford, I'm not very experienced user of RMQ )
yes looks like messages field includes both ready and unacked messages.
Screenshot 2020-04-27 at 13 42 01

so the fix is correct

@zroubalik
Copy link
Member

@holyketzer thanks for the confirmation.

I haven't look at the code yet, so to clarify. If Unacked msgs are already included in messages field, what is the behavior when includeUnacked == false ? Do we get 496 or 498 msgs, based on the previous example?

@tbickford
Copy link
Contributor Author

@zroubalik When includeUnacked == false an alternative method is used to retrieve the message count which would return the number of ready messages (does not include the unacknowledged messages) https://github.com/streadway/amqp/blob/1c71cc93ed716f9a6f4c2ae8955c25f9176d9f19/channel.go#L826. So given the example above, the External Metric would show a value of 496 when setting includeUnacked == false.

@holyketzer
Copy link
Contributor

@zroubalik When includeUnacked == false an alternative method is used to retrieve the message count which would return the number of ready messages (does not include the unacknowledged messages) https://github.com/streadway/amqp/blob/1c71cc93ed716f9a6f4c2ae8955c25f9176d9f19/channel.go#L826. So given the example above, the External Metric would show a value of 496 when setting includeUnacked == false.

Correct for includeUnacked == false AMQP protocol is used, and it can only returns info about ready messages.

@zroubalik
Copy link
Member

@tbickford @holyketzer I thought it is like that, but wanted to have a clarification on this. Thanks!

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @tbickford !

@zroubalik zroubalik merged commit 63c35dd into kedacore:master May 5, 2020
@tomkerkhove tomkerkhove added this to the v1.5 milestone Jul 7, 2020
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023
Signed-off-by: Zach Land <zachary.land@fedex.com>
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.

5 participants