This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 37c81ab0baf51326aed3d78a17940fc1777522e8 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 19:45:14 2026 +0100 Expose the SlowQueryReportJmx JMX surface its metadata declares SlowQueryReportJmx is registered as a Standard MBean, so its management surface is exactly SlowQueryReportJmxMBean plus the notification methods - yet that interface declared only SlowQueriesCD, while the interceptor's mbeans-descriptors.xml and the implementation javadoc advertised PoolName, PoolNames and the resetStats() operation. Operators could not discover pool names or drop accumulated statistics through JMX at all (attribute lookups failed with AttributeNotFoundException, the invoke with ReflectionException: No such operation: resetStats). Declare the three documented members in the interface. getPoolNames() was already a public instance method reading the JVM-wide statistics registry, so no static-to-instance bridge was needed; resetStats() and getPoolName() were reachable methods that simply were not part of the interface. Fixing the interface also exposed a latent implementation bug: resetStats() emptied the map through "while (it.hasNext()) it.remove();" without ever calling next(), so the ConcurrentHashMap iterator threw IllegalStateException on its first remove() whenever anything had been recorded. Nothing had ever called it, so it went unnoticed since the method was introduced. Clear the shared map instead - the map instance is held by every interceptor instance of the pool name, so only its content is dropped and monitoring continues; statistics are keyed by name, so one name's MBean clears what all live pools of that name share (pinned by a test). notifyPool is deliberately not exposed. Registration only ever happens while notifyPool is false, and every interceptor instance routes its own notifications from its own copy of the flag (the pool builds one chain per pooled connection), so a JMX write would re-route only the connection that owns the registration and could leave the MBean registered but silent. It stays a configuration property applied to every instance by setProperties. Align the descriptor's attribute names with the names the Standard MBean introspector derives from the getters, and spell its array types the way the rest of the repository's descriptors do; no code loads that descriptor at runtime (the modeler reads a package descriptor only when creating a Model MBean, which never happens for this interceptor). Tests: TestSlowQueryJmxMBeanSurface pins the MBeanInfo attribute and operation set, reads PoolName/PoolNames through the platform MBeanServer, and invokes resetStats() - including through the shared registration of two same-named pools. All four fail before the fix. Full module suite: 277 tests, no failures. --- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 33 ++- .../pool/interceptor/SlowQueryReportJmxMBean.java | 76 ++++++- .../jdbc/pool/interceptor/mbeans-descriptors.xml | 6 +- .../interceptor/TestSlowQueryJmxMBeanSurface.java | 223 +++++++++++++++++++++ webapps/docs/changelog.xml | 12 ++ 5 files changed, 337 insertions(+), 13 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java index 21030aa65f..96beec950f 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java @@ -364,8 +364,11 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** * JMX operation - return the names of all the pools - * @return - all the names of pools that we have stored data for + * @return - all the names of pools that we have stored data for, + * which is a view of the JVM-wide statistics registry rather + * than of this pool alone */ + @Override public String[] getPoolNames() { Set<String> keys = perPoolStats.keySet(); return keys.toArray(new String[0]); @@ -373,8 +376,10 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE /** * JMX operation - return the name of the pool - * @return the name of the pool, unique within the JVM + * @return the name of the pool, unique within the JVM, or + * {@code null} before this instance is bound to a pool */ + @Override public String getPoolName() { return poolName; } @@ -389,7 +394,13 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE } /** - * Sets whether notifications are sent to the pool. + * Sets whether notifications are sent to the pool. Configuration + * entry point only, applied to every interceptor instance of a pool by + * {@code setProperties} while its connection chain is built: it is not + * part of the JMX management interface, because writing it through the + * registered MBean would reach only the one instance that owns the + * registration and leave the pool's other connections routing as + * before. See {@link SlowQueryReportJmxMBean}. * @param notifyPool true if notifications should be sent to the pool */ public void setNotifyPool(boolean notifyPool) { @@ -397,15 +408,21 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE } /** - * JMX operation - remove all stats for this connection pool + * JMX operation - remove all stats for this connection pool. The + * statistics are keyed by pool name and shared by every live pool + * configured with that name, so clearing them through one pool's MBean + * clears them for all of those pools. Only the recorded content goes: + * the statistics map stays registered with the name - every live + * instance holds that very map and keeps accumulating into it. */ + @Override public void resetStats() { ConcurrentHashMap<String,QueryStats> queries = getPoolStats(poolName); if (queries!=null) { - Iterator<String> it = queries.keySet().iterator(); - while (it.hasNext()) { - it.remove(); - } + // clear the map itself rather than emptying it through an + // iterator: the map is shared with every interceptor instance + // of the name, which all hold this same instance + queries.clear(); } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmxMBean.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmxMBean.java index 1ae12ff0bd..482fe1241d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmxMBean.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmxMBean.java @@ -20,12 +20,84 @@ import javax.management.openmbean.OpenDataException; /** * JMX MBean interface for exposing slow query statistics via JMX. + * <p> + * {@link SlowQueryReportJmx} is published as a <em>Standard</em> MBean, so + * this interface is the whole management surface: a public method of the + * resource that is not declared here stays invisible to a JMX client - + * only the notification methods contributed by + * {@link javax.management.NotificationEmitter} come on top. Everything an + * operator needs, and everything the interceptor's + * {@code mbeans-descriptors.xml} advertises, therefore has to be declared + * here.</p> + * <p> + * The MBean is published once per connection pool name - under + * {@code tomcat.jdbc:type=org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx,name=<pool>} + * unless the {@code objectName} interceptor property overrides it - and + * only when the interceptor is configured with {@code notifyPool=false}. + * With the default {@code notifyPool=true} no MBean is registered and the + * statistics are only observable through the connection pool's own MBean.</p> + * <p> + * {@code notifyPool} itself is deliberately not part of this interface. It + * is a per-interceptor-instance configuration property - the pool builds + * one interceptor chain per pooled connection and configures every + * instance with it - and every instance decides the routing of its own + * notifications from its own copy when it reports a query. Writing it + * through JMX would change only the single instance that happens to own the + * registration and leave the pool's other connections routing as before, + * so the attribute would promise a pool-wide effect it could not deliver. + * It stays a configuration entry point, applied to every instance of the + * pool by + * {@link SlowQueryReportJmx#setProperties(java.util.Map)}.</p> */ public interface SlowQueryReportJmxMBean { /** - * Returns slow query statistics as JMX CompositeData. + * Returns the name of the connection pool this MBean reports for, as + * the {@code PoolName} attribute. The registration, the notifications + * and the statistics are all keyed by pool name, so every live pool + * configured with the same name shares the one MBean reporting that + * name. + * @return the name of the pool, unique within the JVM; {@code null} + * when this interceptor instance is not bound to a pool yet - + * an instance registered with the MBeanServer always is + */ + String getPoolName(); + + /** + * Returns the names of all the connection pools this JVM currently + * holds recorded query statistics for, as the {@code PoolNames} + * attribute. That is a JVM-wide view, not this pool alone: a pool using + * the plain {@code SlowQueryReport} interceptor, which never registers + * an MBean of its own, appears here as well, and pools configured with + * the same name appear once. + * @return the names of the pools that currently hold a statistics map; + * an empty array when no pool has registered statistics + */ + String[] getPoolNames(); + + /** + * Removes all query statistics recorded for the pool this MBean + * reports for, so an operator can start from a clean slate instead of + * restarting the pool. Statistics are keyed by pool name, so this + * clears the map shared by every live pool configured with that name. + * Only the recorded content is dropped: the pool stays monitored and + * keeps accumulating statistics afterwards, and a query recorded again + * starts from empty counters. + */ + void resetStats(); + + /** + * Returns slow query statistics as JMX CompositeData, as the + * {@code SlowQueriesCD} attribute. The element schema is + * {@link SlowQueryReport.QueryStats}: every duration + * ({@code maxInvocationTime}, {@code minInvocationTime}, + * {@code totalInvocationTime}, {@code prepareTime}) is in milliseconds + * and every date field ({@code maxInvocationDate}, + * {@code minInvocationDate}, {@code lastInvocation}) is epoch + * milliseconds. * @return array of composite data representing slow query statistics - * @throws OpenDataException if the composite data cannot be created + * @throws OpenDataException if the composite data cannot be created; + * a JMX client gets it wrapped in an + * {@link javax.management.MBeanException} */ CompositeData[] getSlowQueriesCD() throws OpenDataException; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml index 4db1108547..52eed076ce 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/mbeans-descriptors.xml @@ -23,9 +23,9 @@ <mbean description="Reports " domain="tomcat.jdbc" group="jdbc-pool" name="SlowQueryReportJmx" type="org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx"> - <attribute description="The name of the connection pool this Jmx bean is representing" name="poolName" type="java.lang.String" writeable="false"/> - <attribute description="List of all registered connections pools" name="poolNames" type="[java.lang.String;" writeable="false"/> - <attribute description="All the recorded query stats. " name="slowQueriesCD" type="[javax.management.openmbean.CompositeData;" writeable="false"/> + <attribute description="The name of the connection pool this Jmx bean is representing" name="PoolName" type="java.lang.String" writeable="false"/> + <attribute description="List of all registered connections pools" name="PoolNames" type="[Ljava.lang.String;" writeable="false"/> + <attribute description="All the recorded query stats. " name="SlowQueriesCD" type="[Ljavax.management.openmbean.CompositeData;" writeable="false"/> <notification description="Notification sent out by the slow query report when a query exceeds the threshold" name="slow-query"> <notification-type>Slow query</notification-type> diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java new file mode 100644 index 0000000000..40133eb0f3 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestSlowQueryJmxMBeanSurface.java @@ -0,0 +1,223 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.pool.interceptor; + +import java.lang.management.ManagementFactory; +import java.sql.Connection; +import java.sql.Statement; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; +import java.util.stream.Collectors; + +import javax.management.MBeanAttributeInfo; +import javax.management.MBeanInfo; +import javax.management.MBeanOperationInfo; +import javax.management.MBeanServer; +import javax.management.ObjectName; +import javax.management.openmbean.CompositeData; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * The management surface of the {@code SlowQueryReportJmx} interceptor + * MBean. The interceptor is registered as a <em>Standard</em> MBean, so + * its management interface is exactly what + * {@link SlowQueryReportJmxMBean} declares (plus the notification + * methods) - a public method on the resource that the interface does not + * declare is invisible to a JMX client. The pool's own JMX metadata + * ({@code mbeans-descriptors.xml}) and the implementation's javadoc + * advertise {@code PoolName}, {@code PoolNames}, {@code SlowQueriesCD} + * and the {@code resetStats()} operation, so those must be reachable + * through the MBeanServer: an operator must be able to discover pool + * names and clear recorded statistics without restarting the pool. + * <p> + * Statistics are keyed by pool <em>name</em>, so the MBean of one name + * reads and clears the statistics every live pool of that name shares. + */ +class TestSlowQueryJmxMBeanSurface extends JdbcPoolBaseTest { + + /* A pool whose interceptor registers its MBean and records every query. */ + private DataSource reportingPoolNamed(String name) { + DataSource ds = newDataSource(); // jmxEnabled=false keeps the pools' + // own ObjectNames out of the way; the interceptor registers by name + ds.getPoolProperties().setName(name); + ds.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReportJmx" + + "(notifyPool=false,threshold=-1)"); + ds.getPoolProperties().setInitialSize(0); + return ds; + } + + private static ObjectName objectNameFor(String poolName) throws Exception { + return new ObjectName("tomcat.jdbc:type=" + SlowQueryReportJmx.class.getName() + + ",name=" + poolName); + } + + private static Set<String> attributeNames(MBeanInfo info) { + return Arrays.stream(info.getAttributes()).map(MBeanAttributeInfo::getName) + .collect(Collectors.toSet()); + } + + private static Set<String> operationNames(MBeanInfo info) { + return Arrays.stream(info.getOperations()).map(MBeanOperationInfo::getName) + .collect(Collectors.toSet()); + } + + @Test + void mBeanInfoExposesEveryAdvertisedAttributeAndOperation() throws Exception { + String poolName = "JmxSurface-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = objectNameFor(poolName); + + try (Connection con = ds.getConnection()) { + Assertions.assertTrue(server.isRegistered(oname), + "the interceptor MBean must be registered once the pool is in use"); + MBeanInfo info = server.getMBeanInfo(oname); + Set<String> attributes = attributeNames(info); + Assertions.assertEquals(new HashSet<>(Arrays.asList( + "PoolName", "PoolNames", "SlowQueriesCD")), attributes, + "the attributes the descriptor and the implementation javadoc advertise must " + + "be exactly the attributes of the Standard MBean interface"); + Assertions.assertFalse(attributes.contains("NotifyPool"), + "notifyPool is per-interceptor-instance configuration, not management state: " + + "every instance routes its own notifications from its own copy, so a " + + "JMX write would re-route only the connection owning the registration"); + + Assertions.assertTrue(operationNames(info).contains("resetStats"), + "resetStats() must be an operation of the management interface, found: " + + operationNames(info)); + MBeanOperationInfo reset = Arrays.stream(info.getOperations()) + .filter(op -> "resetStats".equals(op.getName())).findFirst() + .orElse(null); + Assertions.assertNotNull(reset, "resetStats() must be invocable, found: " + + operationNames(info)); + Assertions.assertEquals("void", reset.getReturnType(), + "resetStats() is declared without a return value, as the descriptor states"); + } + } + + @Test + void poolNameAndPoolNamesAreReadableThroughTheServer() throws Exception { + String poolName = "JmxPoolName-" + System.nanoTime(); + String otherPoolName = "JmxPoolName-Other-" + System.nanoTime(); + DataSource reporting = reportingPoolNamed(poolName); + // a second pool under a different name, whose statistics exist but + // which registers no MBean of its own: PoolNames is documented as + // the names of all pools holding recorded statistics in this JVM + DataSource statsOnly = newDataSource(); + statsOnly.getPoolProperties().setName(otherPoolName); + statsOnly.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport" + + "(threshold=-1)"); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = objectNameFor(poolName); + + try (Connection con = reporting.getConnection(); + Connection other = statsOnly.getConnection(); + Statement st = other.createStatement()) { + st.execute("SELECT 1"); + + Assertions.assertEquals(poolName, server.getAttribute(oname, "PoolName"), + "the PoolName attribute must report the pool the MBean was registered for"); + + Object poolNames = server.getAttribute(oname, "PoolNames"); + Assertions.assertInstanceOf(String[].class, poolNames, + "PoolNames must be readable as a String array"); + Set<String> names = new HashSet<>(Arrays.asList((String[]) poolNames)); + Assertions.assertTrue(names.contains(poolName), + "PoolNames must contain this MBean's pool, found: " + names); + Assertions.assertTrue(names.contains(otherPoolName), + "PoolNames must contain every pool that holds recorded statistics, found: " + + names); + } + } + + @Test + void resetStatsIsInvocableAndClearsTheRecordedStatistics() throws Exception { + String poolName = "JmxResetStats-" + System.nanoTime(); + DataSource ds = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = objectNameFor(poolName); + + try (Connection con = ds.getConnection()) { + try (Statement st = con.createStatement()) { + st.execute("SELECT 1"); + } + Assertions.assertEquals(1, ((CompositeData[]) server.getAttribute(oname, + "SlowQueriesCD")).length, + "the query must have been recorded before the reset"); + + // the documented way for an operator to drop accumulated stats: + // no pool restart needed + Assertions.assertNull(server.invoke(oname, "resetStats", null, null), + "resetStats() is declared with a void return type"); + + Assertions.assertEquals(0, ((CompositeData[]) server.getAttribute(oname, + "SlowQueriesCD")).length, + "invoking resetStats() must empty the statistics exposed through the MBean"); + Assertions.assertTrue(SlowQueryReport.getPoolStats(poolName).isEmpty(), + "invoking resetStats() must clear the pool's statistics map, still holding: " + + SlowQueryReport.getPoolStats(poolName).keySet()); + + // clearing the contents must not take the statistics map away: + // the still-running pool keeps reporting into it + try (Statement st = con.createStatement()) { + st.execute("SELECT 2"); + } + Assertions.assertEquals(1, ((CompositeData[]) server.getAttribute(oname, + "SlowQueriesCD")).length, + "statistics must accumulate again after the reset"); + } + } + + @Test + void resetStatsClearsTheStatisticsEveryPoolOfTheNameShares() throws Exception { + String poolName = "JmxResetShared-" + System.nanoTime(); + DataSource owner = reportingPoolNamed(poolName); + DataSource sharer = reportingPoolNamed(poolName); + + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + ObjectName oname = objectNameFor(poolName); + + try (Connection ownerCon = owner.getConnection(); + Connection sharerCon = sharer.getConnection(); + Statement st = sharerCon.createStatement()) { + st.execute("SELECT 1"); + + Assertions.assertEquals(1, ((CompositeData[]) server.getAttribute(oname, + "SlowQueriesCD")).length, + "the query of the same-named pool must show up in the one registered MBean"); + + server.invoke(oname, "resetStats", null, null); + + Assertions.assertTrue(SlowQueryReport.getPoolStats(poolName).isEmpty(), + "statistics are keyed by pool name, so resetStats() through that name's MBean " + + "clears the map all live pools of the name share, still holding: " + + SlowQueryReport.getPoolStats(poolName).keySet()); + Assertions.assertEquals(0, ((CompositeData[]) server.getAttribute(oname, + "SlowQueriesCD")).length, + "the exposed statistics must be empty for the owning pool as well"); + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 0f3bc267af..abbe814455 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -908,6 +908,18 @@ three of the interceptors bundled with the pool, has been corrected. (remm) </fix> + <fix> + The <code>SlowQueryReportJmx</code> interceptor MBean now exposes + the management surface its metadata declares: the + <code>PoolName</code> and <code>PoolNames</code> attributes and + the <code>resetStats()</code> operation are reachable through + JMX. Previously the MBean interface declared only + <code>SlowQueriesCD</code>, so the attributes and the operation + did not exist on the registered MBean, and once declared, + <code>resetStats()</code> would have thrown an + <code>IllegalStateException</code> whenever statistics were + present. (remm) + </fix> </changelog> </subsection> <subsection name="Other"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
