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

x-pack/metricbeat/module/activemq: Add support for PendingQueueSize in topic metricset #42821

Closed
wants to merge 8 commits into from

Conversation

Linu-Elias
Copy link
Contributor

@Linu-Elias Linu-Elias commented Feb 21, 2025

Proposed commit message

Added jmx.mapping to support PendingQueueSize in topic metricset.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

image

Logs

@Linu-Elias Linu-Elias requested a review from a team as a code owner February 21, 2025 09:11
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 21, 2025
Copy link
Contributor

mergify bot commented Feb 21, 2025

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @Linu-Elias? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@Linu-Elias Linu-Elias changed the title x-pack/metricbeat/module/activemq: Fix added support for PendingQueueSize in Topic datastream x-pack/metricbeat/module/activemq: Fix added to support PendingQueueSize in Topic datastream Feb 24, 2025
@@ -266,6 +266,7 @@ otherwise no tag is added. {issue}42208[42208] {pull}42403[42403]
- Continue collecting metrics even if the Cisco Meraki `getDeviceLicenses` operation fails. {pull}42397[42397]
- Fixed errors in the `elasticsearch.index` metricset when index settings are missing. {issue}42424[42424] {pull}42426[42426]
- Fixed panic caused by uninitialized meraki device wifi0 and wifi1 struct pointers in the device WiFi data fetching. {issue}42745[42745] {pull}42746[42746]
- Fixed the missing support for `PendingQueueSize` in the ActiveMQ topic metricset. {pull}42821[42821]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fixed the missing support for `PendingQueueSize` in the ActiveMQ topic metricset. {pull}42821[42821]
- Add support for `PendingQueueSize` in the ActiveMQ topic metricset. {pull}42821[42821]

This should be under enhancement and not bugfix.

@ishleenk17
Copy link
Contributor

Couple of points here @Linu-Elias

  1. Add this field to fields.yml
  2. Update the sample json
  3. Create a ticket in Integrations for the same change.

@ishleenk17
Copy link
Contributor

ishleenk17 commented Feb 25, 2025

@Linu-Elias : As discussed PendingQueueSize is actually not a metric for a "testTopic" but a "Subscriber".
Example below: For "Topic" topic, there are 2 subscribers and each of them have their own PendingQueueSize.

[
{
"request": {
"mbean": "org.apache.activemq:brokerName=,clientId=,consumerId=,destinationName=,destinationType=Topic,endpoint=*,type=Broker",
"type": "read"
},
"value": {
"org.apache.activemq:brokerName=localhost,clientId=987,consumerId=Durable(987_testSub1),destinationName=testTopic,destinationType=Topic,endpoint=Consumer,type=Broker": {
"Retroactive": false,
"CursorMemoryUsage": 9901,
"Exclusive": false,
"SlowConsumer": true,
"DispatchAsync": false,
"DestinationTemporary": false,
"ConnectionId": "NOTSET",
"MaximumPendingMessageLimit": 0,
"SubscriptionId": -1,
"Network": false,
"DestinationName": "testTopic",
"ClientId": "987",
"DestinationQueue": false,
"SessionId": 0,
"EnqueueCounter": 2,
"Selector": null,
"DestinationTopic": true,
"UserName": null,
"CursorFull": false,
"DispatchedQueueSize": 0,
"Priority": 0,
"Connection": null,
"PendingQueueSize": 2,
"MessageCountAwaitingAcknowledge": 0,
"SubscriptionName": "testSub1",
"PrefetchSize": 0,
"CursorPercentUsage": 0,
"Active": false,
"NoLocal": false,
"Durable": false,
"DequeueCounter": 0,
"ConsumedCount": 0,
"DispatchedCounter": 0
},
"org.apache.activemq:brokerName=localhost,clientId=234,consumerId=Durable(234_testSub2),destinationName=testTopic,destinationType=Topic,endpoint=Consumer,type=Broker": {
"Retroactive": false,
"CursorMemoryUsage": 9901,
"Exclusive": false,
"SlowConsumer": true,
"DispatchAsync": false,
"DestinationTemporary": false,
"ConnectionId": "NOTSET",
"MaximumPendingMessageLimit": 0,
"SubscriptionId": -1,
"Network": false,
"DestinationName": "testTopic",
"ClientId": "234",
"DestinationQueue": false,
"SessionId": 0,
"EnqueueCounter": 1,
"Selector": null,
"DestinationTopic": true,
"UserName": null,
"CursorFull": false,
"DispatchedQueueSize": 0,
"Priority": 0,
"Connection": null,
"PendingQueueSize": 1,
"MessageCountAwaitingAcknowledge": 0,
"SubscriptionName": "testSub2",
"PrefetchSize": 0,
"CursorPercentUsage": 0,
"Active": false,
"NoLocal": false,
"Durable": false,
"DequeueCounter": 0,
"ConsumedCount": 0,
"DispatchedCounter": 0
}
},
"timestamp": 1740463291,
"status": 200
}
]

Solution:

We should add a new metricset "Subscriber" with couple of important metrics from the above response. Eg: DestinamtionName, subscriberName, PendingQueSize etc... So that the user can do an aggregation of the PendingQueuSize/topic. Only then its useful.

cc: @lalit-satapathy @daniela-elastic

@@ -483,6 +483,8 @@ otherwise no tag is added. {issue}42208[42208] {pull}42403[42403]
- Add new metricset wmi for the windows module. {pull}42017[42017]
- Update beat module with apm-server tail sampling monitoring metrics fields {pull}42569[42569]
- Log every 401 response from Kubernetes API Server {pull}42714[42714]
- Added `pending.queue.size` in the ActiveMQ topic metricset. {pull}42821[42821]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Added `pending.queue.size` in the ActiveMQ topic metricset. {pull}42821[42821]
- Add `pending.queue.size` in the ActiveMQ's `topic` metricset. {pull}42821[42821]

@shmsr
Copy link
Member

shmsr commented Feb 25, 2025

Changes look good. But I see this field might also be very important for "queue" metricset as well. If we could do that here or even in a separate PR or create an enhancement issue for the same in the backlog? Maybe let's just file an ER for now and let's not do the change in this PR.

@shmsr shmsr added the Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team label Feb 25, 2025
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 25, 2025
@shmsr shmsr changed the title x-pack/metricbeat/module/activemq: Fix added to support PendingQueueSize in Topic datastream x-pack/metricbeat/module/activemq: Add support for PendingQueueSize in topic metricset Feb 25, 2025
@ishleenk17
Copy link
Contributor

@Linu-Elias : As discussed PendingQueueSize is actually not a metric for a "Topic" but a "Subscriber". Example below: For "Topic" topic, there are 2 subscribers and each of them have their own PendingQueueSize.

[
{
"request": {
"mbean": "org.apache.activemq:brokerName=,clientId=,consumerId=,destinationName=,destinationType=Topic,endpoint=*,type=Broker",
"type": "read"
},
"value": {
"org.apache.activemq:brokerName=localhost,clientId=987,consumerId=Durable(987_testSub1),destinationName=testTopic,destinationType=Topic,endpoint=Consumer,type=Broker": {
"Retroactive": false,
"CursorMemoryUsage": 9901,
"Exclusive": false,
"SlowConsumer": true,
"DispatchAsync": false,
"DestinationTemporary": false,
"ConnectionId": "NOTSET",
"MaximumPendingMessageLimit": 0,
"SubscriptionId": -1,
"Network": false,
"DestinationName": "testTopic",
"ClientId": "987",
"DestinationQueue": false,
"SessionId": 0,
"EnqueueCounter": 2,
"Selector": null,
"DestinationTopic": true,
"UserName": null,
"CursorFull": false,
"DispatchedQueueSize": 0,
"Priority": 0,
"Connection": null,
"PendingQueueSize": 2,
"MessageCountAwaitingAcknowledge": 0,
"SubscriptionName": "testSub1",
"PrefetchSize": 0,
"CursorPercentUsage": 0,
"Active": false,
"NoLocal": false,
"Durable": false,
"DequeueCounter": 0,
"ConsumedCount": 0,
"DispatchedCounter": 0
},
"org.apache.activemq:brokerName=localhost,clientId=234,consumerId=Durable(234_testSub2),destinationName=testTopic,destinationType=Topic,endpoint=Consumer,type=Broker": {
"Retroactive": false,
"CursorMemoryUsage": 9901,
"Exclusive": false,
"SlowConsumer": true,
"DispatchAsync": false,
"DestinationTemporary": false,
"ConnectionId": "NOTSET",
"MaximumPendingMessageLimit": 0,
"SubscriptionId": -1,
"Network": false,
"DestinationName": "testTopic",
"ClientId": "234",
"DestinationQueue": false,
"SessionId": 0,
"EnqueueCounter": 1,
"Selector": null,
"DestinationTopic": true,
"UserName": null,
"CursorFull": false,
"DispatchedQueueSize": 0,
"Priority": 0,
"Connection": null,
"PendingQueueSize": 1,
"MessageCountAwaitingAcknowledge": 0,
"SubscriptionName": "testSub2",
"PrefetchSize": 0,
"CursorPercentUsage": 0,
"Active": false,
"NoLocal": false,
"Durable": false,
"DequeueCounter": 0,
"ConsumedCount": 0,
"DispatchedCounter": 0
}
},
"timestamp": 1740463291,
"status": 200
}
]

Solution:

We should add a new metricset "Subscriber" with couple of important metrics from the above response. Eg: DestinamtionName, subscriberName, PendingQueSize etc... So that the user can do an aggregation of the PendingQueuSize/topic. Only then its useful.

cc: @lalit-satapathy @daniela-elastic

@Linu-Elias : Will be creating a separate issue describing the problem statement here. We can discuss it there on how to take this further.

@shmsr
Copy link
Member

shmsr commented Feb 25, 2025

@ishleenk17 As far I can see, for topics, "durable" subscribers in "topic" have that field. But in the issue linked, the requirement is:

"Desired topic size is the sum of PendingQueueSize of all subscriptions in that topic."

But yes, I think the field means number of messages pending for delivery to durable subscribers. So, I think we should not create a separate metricset for subscribers? We should get the field and do the aggregation (i.e., sum).

Also to get that data, we are using "destinationType=Topic". So I am not sure if it's a good idea to create a metricset for "Subscriber".

@ishleenk17
Copy link
Contributor

ishleenk17 commented Feb 25, 2025

So I am not sure if it's a good idea to create a metricset for "Subscriber".

The subscriber dataset (name can be discussed) is not only for this metric, but in general there are other important metrics (dispatchedqueuesize, pendingqueuesize, dispacthedcounter, enque_counter etc) that we should have for subscriber.

Each mbean comes as a separate event. So if we go eith current implementation we will have separate event for topic mbean and a separate event for subscriber mbean. The fields that we would get currently are activemq.topic.pendingqueuesize.... Which could be misleading as its not a metric of a topic.
We can ofcourse get all the pendingquesizes and just do addition of those per topic and give the final field of topicsize to user.
But we don't want to cater to just this request.
So, IMO, it's better to provide the subscriber level metrics to the user and leave it upto the user on how they want to use the metrics.

@Linu-Elias
Copy link
Contributor Author

Yes, I agree with @ishleenk17, as one topic can have multiple subscribers, and each subscriber has their own set of metrics. For reference, I have attached a screenshot of how the subscriber view page looks in my local setup. We can actually correlate this image with the JSON response for subscribers that @ishleenk17 shared in the earlier comment.

image

@shmsr
Copy link
Member

shmsr commented Feb 25, 2025

Okay, sure! Thanks!

@daniela-elastic
Copy link

@ishleenk17 are there any open questions for me?

@ishleenk17
Copy link
Contributor

@ishleenk17 are there any open questions for me?

No @daniela-elastic . We have created a public issue for this.
We can discuss about prioritization of it in the issue.

@ishleenk17 ishleenk17 closed this Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Subscriber metricset ActiveMQ
4 participants