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-9437] Add policyEntry advancedNetworkStatisticsEnabled flag #1156

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

mattrpav
Copy link
Contributor

@mattrpav mattrpav commented Feb 23, 2024

PolicyEntry option advancedNetworkStatisticsEnabled (default: false)

[AdvancedNetworkStatistics]

  • networkEnqueueCount
  • networkDequeueCount
  • Update unit tests to validate policyEntry and runtimeConfigurationPlugin
  • Update unit tests to validate statistics are working as expected
  • Add null guard for null context/connection scenarios
  • Add queue unit test
  • Improve unit test to confirm non-network use case counting is correct

This change introduces the configuration guard for advancedNetworkStatistics as well as two simple metrics to provide fine-grained counting for network enqueue and dequeue counts that are enabled behind this config flag.

Usage notes:

  • ActiveMQ.Advisory.> topics should never have advancedNetworkStatisticsEnabled set to 'true'
  • High-performing non-persistent topic workflows may not want this enabled due to potential for latency penalties.

@mattrpav mattrpav self-assigned this Feb 23, 2024
@mattrpav mattrpav changed the title [AMQ-9426] Add destination AdvancedStatisticsEnabled flag and network… [AMQ-9426] Add destination advancedStatisticsEnabled flag and network… Feb 23, 2024
@mattrpav mattrpav changed the title [AMQ-9426] Add destination advancedStatisticsEnabled flag and network… [AMQ-9437] Add destination advancedStatisticsEnabled flag and network… Sep 13, 2024
@jbonofre
Copy link
Member

This change is breaking activemq-broker tests:

Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.activemq.broker.Connection.isNetworkConnection()" because the return value of "org.apache.activemq.broker.ConnectionContext.getConnection()" is null
        at org.apache.activemq.broker.region.Queue.messageSent(Queue.java:1978)
        at org.apache.activemq.broker.region.Queue.doPendingCursorAdditions(Queue.java:843)
        at org.apache.activemq.broker.region.Queue$CursorAddSync.afterCommit(Queue.java:861)
        at org.apache.activemq.transaction.Transaction.fireAfterCommit(Transaction.java:140)
        at org.apache.activemq.transaction.Transaction.doPostCommit(Transaction.java:211)

I'm fixing.

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.

The problem I have with this is creating a single flag for all of these extra metrics, and also the name is just very generic with no specifics. If I'm a user and i see "advancedStatistics" I have no idea what that means. Also there's nothing that really makes this advanced per se, it's just more stats.

Grouping together a bunch of random statistics with a single on/off doesn't make sense to me because all of these extra statistics will have different levels of performance hits and users might only want some of them. Someone might want extra networking stats but doesn't care about the other stuff you mentioned.

So I think that we probably need multiple flags to make it more fine grained. I view it similarly to the advisory configuration on a policy where you can control which advisories are sent. I think different stats that are unrelated should be grouped together under a different flag to turn them on or off. Obviously this is bridge related so the flag could be called some like advancedBridgeStats or advancedNetworkStats or whatever.

@mattrpav
Copy link
Contributor Author

mattrpav commented Sep 16, 2024

This change is breaking activemq-broker tests:

Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.activemq.broker.Connection.isNetworkConnection()" because the return value of "org.apache.activemq.broker.ConnectionContext.getConnection()" is null
        at org.apache.activemq.broker.region.Queue.messageSent(Queue.java:1978)
        at org.apache.activemq.broker.region.Queue.doPendingCursorAdditions(Queue.java:843)
        at org.apache.activemq.broker.region.Queue$CursorAddSync.afterCommit(Queue.java:861)
        at org.apache.activemq.transaction.Transaction.fireAfterCommit(Transaction.java:140)
        at org.apache.activemq.transaction.Transaction.doPostCommit(Transaction.java:211)

I'm fixing.

Null guard added in latest commit 1e94832

@mattrpav
Copy link
Contributor Author

mattrpav commented Sep 16, 2024

The problem I have with this is creating a single flag for all of these extra metrics, and also the name is just very generic with no specifics. If I'm a user and i see "advancedStatistics" I have no idea what that means. Also there's nothing that really makes this advanced per se, it's just more stats.

Parameter renamed in latest commit 1e948432

- Rename to advancedNetworkStatistics
- Add unit test to cover queue and topic use case
- Improve unit tests to confirm network stats do not incorrectly increment on non-network enqueue/dequeue
@mattrpav mattrpav changed the title [AMQ-9437] Add destination advancedStatisticsEnabled flag and network… [AMQ-9437] Add policyEntry advancedNetworkStatisticsEnabled flag Sep 16, 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.

LGTM, the new property name is more descriptive and a bit better

@mattrpav mattrpav merged commit f34a660 into apache:main Sep 20, 2024
1 check passed
@mattrpav mattrpav deleted the AMQ-9426 branch September 20, 2024 13:48
Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

New property name is much better and the feature is disabled by default, so it's all good for me.

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