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

[AMQ-8463] Add advancedMessageStatistics feature #1329

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

Conversation

mattrpav
Copy link
Contributor

@mattrpav mattrpav commented Oct 23, 2024

Reviewer notes:

  • New AdvancedMessageStatisticsEnabled flag per-policyEntry (default: false)
  • No new timestamp sampling added (ie no new System.currentTimeMillis() calls)
  • Values reset on resetStatistics
  • Values exposed via JMX
  • Enqueue Message BrokerInTime, ClientID, MessageID and MessageTimestamp fields
  • Dequeue Message BrokerInTime, BrokerOutTime, ClientID, MessageID and MessageTimestamp fields
  • Unit test for policyEntry runtime configuration modification
  • Unit test for functional testing including network include/exclude verification
  • Non-durable topics do not have brokerInTime or messageTimestamp in scope during dequeue, so those values will always be zero (0)
  • Convert to using generics for UnsampledStatistic
  • Add unit test for UnsampledStatisticImpl (done: 98.3% coverage)

advanced-message-statistics

@mattrpav mattrpav self-assigned this Oct 23, 2024
@mattrpav mattrpav force-pushed the AMQ-8463 branch 4 times, most recently from a25b984 to dabeb33 Compare October 25, 2024 16:46
@mattrpav mattrpav marked this pull request as ready for review October 25, 2024 17:04
@mattrpav mattrpav changed the title WIP: [AMQ-8463] Add advancedMessageStatistics feature [AMQ-8463] Add advancedMessageStatistics feature Oct 25, 2024
Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

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

One thing I noticed is the new statistic impl classes all are copy and pasted code, it seems like you could just re-use all of that if you used generics.

@cshannon
Copy link
Contributor

I know this would be a much bigger effort but if we intend to keep adding more and more metrics, from a performance and maintainability standpoint, we should really look into moving everything to Micrometer which adds more more flexibility and should be better performance. That kind of change would likely end up being a major version, ie AMQ 7.0 if we wanted to do that.

@mattrpav mattrpav force-pushed the AMQ-8463 branch 3 times, most recently from df08462 to c461626 Compare December 13, 2024 18:13
@mattrpav
Copy link
Contributor Author

mattrpav commented Dec 13, 2024

Local performance testing showed no degradation in performance

64x producers sending in async to a single topic sent 1M messages ea in ~24s on a 2023 14" Apple M3 Pro (2.6M messages/sec)

}

@Override
public long getDequeuedMessageTimestamp() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking all of these getters could be a bit cleaner and less copy/paste with using lambdas. IE.

@Override
public long getDequeuedMessageTimestamp() {
    return getMessageFlowStat(MessageFlowStats::getDequeuedMessageTimestamp, 0L);
}

private <T> T getMessageFlowStat(Function<MessageFlowStats, UnsampledStatistic<T>> f, T defVal) {
    final var stats = destination.getDestinationStatistics().getMessageFlowStats();
    return stats != null ? f.apply(stats).getValue() : defVal;
}

@@ -186,6 +177,11 @@ public void reset() {
maxUncommittedExceededCount.reset();
networkEnqueues.reset();
networkDequeues.reset();

MessageFlowStatsImpl tmpMessageFlowStats = messageFlowStats;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I usually like to make all of these final just to prevent any future mistakes

}
}

public synchronized void setAdvancedMessageStatisticsEnabled(boolean advancedMessageStatisticsEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot of thread safety issues in this entire class. By making all of this stuff protected someone could easily screw up the state outside a lock. Furthermore you are synchronizing this method but not other places like reset() or anywhere else.

You should not be allowing the ability to set messageFlowStats or to set advancedMessageStatisticsEnabled() outside this method as you could get them out of sync and right now it's easy to do because they are not private

I need to think about it more but I really don't think the current approach is great, especially if you are making member variables protected and not private.

destinationStatistics.getEnqueues().increment();
destinationStatistics.getMessages().increment();
destinationStatistics.getMessageSize().addSize(msg.getSize());

MessageFlowStats tmpMessageFlowStats = destinationStatistics.getMessageFlowStats();

if(isAdvancedMessageStatisticsEnabled() && tmpMessageFlowStats != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something where both of these should ALWAYS be in sync. you should never have the stats enabled but the object is null.

Maybe the solution is you drop the flag entirely. Maybe stats being enabled is really just a "is this object not null" and disabling is making the stats null so we don't have to check 2 objects.

@@ -83,32 +89,31 @@ public synchronized long getLastSampleTime() {
/**
* @return the enabled
*/
public boolean isEnabled() {
public synchronized boolean isEnabled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no benefit to making these methods synchronized, the variables are already volatile and reading or setting the boolean is atomic

this.set.add(statistic);
}

protected synchronized void addStatistics(Collection<StatisticImpl> statistics) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This also doesn't make a lot of sense, you don't need to synchronize and also use a CopyOnWriteArrayList.

@cshannon
Copy link
Contributor

@mattrpav - I opened up a PR to this branch to fix a few things but I noted that I think StatisticImpl has some issues right now with the changes (mostly due to the fact that so many classes inherit from it) mattrpav#1

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