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

WIP: AMQ-9552: Add metric lastMessageTimestamp to StatisticsBroker based o… #1285

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ggkochanski
Copy link

Add metric lastMessageTimestamp to StatisticsBroker based on modification time of metric enqueues:

// NOTICE: Client-side, you may get the broker "now" Timestamp by msg.getJMSTimestamp()
// This allows for calculating inactivity.
statsMessage.setLong("lastMessageTimestamp", stats.getEnqueues().getLastSampleTime()); 

@ggkochanski
Copy link
Author

Some examples per threshold of this metric from real life implementation:

image

@ggkochanski ggkochanski force-pushed the AMQ-9552 branch 4 times, most recently from cc8ba95 to e0bf09d Compare August 23, 2024 06:45
@@ -80,6 +80,9 @@ public synchronized long getLastSampleTime() {
return this.lastSampleTime;
}

public synchronized long getLastSampleTimeOrStartTime(){
return lastSampleTime == 0 ? startTime : lastSampleTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative approach to setting the lastSampleTime to 0, we can keep the this.lastSampleTime = this.startTime; as originally implemented. But then we add an additional field in StatisticImpl class like "hasUpdated" to indicate if the lastSampleTime has been updated. In that way, it's up to the caller to decide what to make use of this "hasUpdated" field. I.E write 0, or not set at all ... etc.

The advantage of avoiding giving special meaning to 0 is:

  1. Not explicit in semantic and make the code a bit harder to follow (why 0?)
  2. What if 0 means some other things in downstream code
  3. More flexible and clear on the caller, it calls the object to see if the lastSampleTime has been updated since restart, rather than just set the value to whatever stats.getEnqueues().getLastSampleTime() returns

Copy link
Author

Choose a reason for hiding this comment

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

wow, that's clever 😊

it's much better,
we may even skip generation of
statsMessage.setLong("lastMessageTimestamp", stats.getEnqueues().getLastSampleTime())
when it's not "hasUpdated" 😋

…n lastSampleTime of metric enqueues (if hasUpdated)
@mattrpav mattrpav changed the title AMQ-9552: Add metric lastMessageTimestamp to StatisticsBroker based o… WIP: AMQ-9552: Add metric lastMessageTimestamp to StatisticsBroker based o… Aug 23, 2024
@mattrpav
Copy link
Contributor

I have started a capability to put additional statistics behind a config flag, so adding new metrics such as lastMessageTimestamp, lastMessageId, etc.. do not add computation overhead to all queues by default to all users.

The statistics broker update should wait to follow the work done for the core capability.

The field names should be consistent across all APIs and functions. There are other requests to add deeper observability fields -- firstEnqueue, lastEnqueue, firstDequeue, etc.. so 'lastMessageTimestamp' as a field name is not specific enough to meet all the requests.

ref: [AMQ-9426]

The initial work for the feature is here: #1156

@mattrpav mattrpav self-assigned this Aug 23, 2024
@mattrpav mattrpav self-requested a review August 23, 2024 17:49
if ( stats.getEnqueues().hasUpdated() ) {
// NOTICE: Client-side, you may get the broker "now" Timestamp by msg.getJMSTimestamp()
// This allows for calculating inactivity.
statsMessage.setLong("lastMessageTimestamp", stats.getEnqueues().getLastSampleTime());
Copy link
Author

Choose a reason for hiding this comment

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

@mattrpav I can see your change is merged, so I'm refreshing the pull request.

I understand the name unification motivation. Should I change name lastMessageTimestamp into lastEnqueTimestamp here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ggkochanski The field naming is a broader conversation. The full scope is not one field as you have in this PR. We currently have a number of metrics (and config params) that are not fully aligned in naming convention, so I would press for an organized approach for these new metrics.

Related JIRAs:
ref: https://issues.apache.org/jira/browse/AMQ-4358
ref: https://issues.apache.org/jira/browse/AMQ-5497

Copy link
Author

Choose a reason for hiding this comment

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

What is the meaning LastDequeueTime and FirstDequeueTime? I believe one of them cannot be determined (if the last means the newest enqueued message then it is not dequeued yet)

@mattrpav could you explain in details how you imagine this new approach to metrics based on some example preferably?

Maybe I try to use statistics in the wrong way. What I need to know to assess the condition of mq system is: enqueue count and last update timestamp, dequeue count and last update, last process time (iso "averageEnqueueTime") and the age of the oldest message in queue. What do you suggest me?

Copy link
Contributor

@mattrpav mattrpav Oct 10, 2024

Choose a reason for hiding this comment

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

@ggkochanski yeah, the challenge is settling on the terminology for the metric values. For example, does 'last' mean last-as-in-time, or last-as-in-last-message-in-the-queue (most recently added).

This feature will be enabled via PolicyEntry property, such as advancedTimeStatisticsEnabled="true". This will allow existing users to not incur the potential latency penalty and users that wish to have increased observability can simply toggle the parameter.

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.

3 participants