This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch branch-1.0
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.0 by this push:
new c67f8903b6 [#8558] fix(metrics): fix missing backend connection pool
metrics (#8573)
c67f8903b6 is described below
commit c67f8903b6cc3f4d28e047193642ca3e87530e8f
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Wed Sep 17 13:54:29 2025 +0800
[#8558] fix(metrics): fix missing backend connection pool metrics (#8573)
### What changes were proposed in this pull request?
fix missing backend connection pool metrics
### Why are the changes needed?
Fix: #8558
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
CI pass
Co-authored-by: mchades <[email protected]>
Co-authored-by: fanng <[email protected]>
---
.../org/apache/gravitino/metrics/MetricNames.java | 9 +-
.../apache/gravitino/metrics/MetricsSystem.java | 11 ++-
.../gravitino/metrics/source/MetricsSource.java | 1 +
.../source/RelationDatasourceMetricsSource.java | 13 +--
.../session/SqlSessionFactoryHelper.java | 109 +++++++++++----------
.../metrics/TestExtractMetricNameAndLabel.java | 27 +++--
.../gravitino/metrics/TestMetricsSystem.java | 25 ++++-
7 files changed, 112 insertions(+), 83 deletions(-)
diff --git a/core/src/main/java/org/apache/gravitino/metrics/MetricNames.java
b/core/src/main/java/org/apache/gravitino/metrics/MetricNames.java
index 3a26c20b4b..d6d09ec13b 100644
--- a/core/src/main/java/org/apache/gravitino/metrics/MetricNames.java
+++ b/core/src/main/java/org/apache/gravitino/metrics/MetricNames.java
@@ -22,12 +22,9 @@ package org.apache.gravitino.metrics;
public class MetricNames {
public static final String HTTP_PROCESS_DURATION =
"http-request-duration-seconds";
public static final String SERVER_IDLE_THREAD_NUM =
"http-server.idle-thread.num";
- public static final String
ENTITY_STORE_RELATION_DATASOURCE_ACTIVE_CONNECTIONS =
- "entity-store.relation-datasource.active-connections";
- public static final String ENTITY_STORE_RELATION_DATASOURCE_IDLE_CONNECTIONS
=
- "entity-store.relation-datasource.idle-connections";
- public static final String ENTITY_STORE_RELATION_DATASOURCE_MAX_CONNECTIONS =
- "entity-store.relation-datasource.max-connections";
+ public static final String DATASOURCE_ACTIVE_CONNECTIONS =
"datasource.active-connections";
+ public static final String DATASOURCE_IDLE_CONNECTIONS =
"datasource.idle-connections";
+ public static final String DATASOURCE_MAX_CONNECTIONS =
"datasource.max-connections";
private MetricNames() {}
}
diff --git a/core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java
b/core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java
index 6e30d55b2a..bcd86ad530 100644
--- a/core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java
+++ b/core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java
@@ -24,6 +24,7 @@ import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.Reporter;
import com.codahale.metrics.jmx.JmxReporter;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import io.prometheus.client.CollectorRegistry;
import io.prometheus.client.dropwizard.DropwizardExports;
@@ -77,7 +78,15 @@ public class MetricsSystem implements Closeable {
public synchronized void register(MetricsSource metricsSource) {
LOG.info("Register {} to metrics system {}",
metricsSource.getMetricsSourceName(), name);
if (metricSources.containsKey(metricsSource.getMetricsSourceName())) {
- unregister(metricSources.get(metricsSource.getMetricsSourceName()));
+ MetricsSource originalMetricsSource =
metricSources.get(metricsSource.getMetricsSourceName());
+ // In case different MetricsSource use the same name
+ Preconditions.checkState(
+
metricsSource.getClass().getName().equals(originalMetricsSource.getClass().getName()),
+ "Metrics source name %s is already registered, and it's class name
is %s, but the new metrics source class name is %s",
+ metricsSource.getMetricsSourceName(),
+ originalMetricsSource.getClass().getName(),
+ metricsSource.getClass().getName());
+ unregister(originalMetricsSource);
}
this.metricSources.put(metricsSource.getMetricsSourceName(),
metricsSource);
metricRegistry.register(
diff --git
a/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
b/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
index 68d0b32949..049e8194cd 100644
--- a/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
+++ b/core/src/main/java/org/apache/gravitino/metrics/source/MetricsSource.java
@@ -40,6 +40,7 @@ public abstract class MetricsSource {
// metrics source name
public static final String ICEBERG_REST_SERVER_METRIC_NAME =
"iceberg-rest-server";
public static final String GRAVITINO_SERVER_METRIC_NAME = "gravitino-server";
+ public static final String GRAVITINO_RELATIONAL_STORE_METRIC_NAME =
"gravitino-relational-store";
public static final String JVM_METRIC_NAME = "jvm";
private final MetricRegistry metricRegistry;
private final String metricsSourceName;
diff --git
a/core/src/main/java/org/apache/gravitino/metrics/source/RelationDatasourceMetricsSource.java
b/core/src/main/java/org/apache/gravitino/metrics/source/RelationDatasourceMetricsSource.java
index 31582776e2..ee98d27153 100644
---
a/core/src/main/java/org/apache/gravitino/metrics/source/RelationDatasourceMetricsSource.java
+++
b/core/src/main/java/org/apache/gravitino/metrics/source/RelationDatasourceMetricsSource.java
@@ -26,15 +26,10 @@ import org.apache.gravitino.metrics.MetricNames;
public class RelationDatasourceMetricsSource extends MetricsSource {
public RelationDatasourceMetricsSource(BasicDataSource dataSource) {
- super(MetricsSource.GRAVITINO_SERVER_METRIC_NAME);
+ super(MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME);
registerGauge(
- MetricNames.ENTITY_STORE_RELATION_DATASOURCE_ACTIVE_CONNECTIONS,
- (Gauge<Integer>) dataSource::getNumActive);
- registerGauge(
- MetricNames.ENTITY_STORE_RELATION_DATASOURCE_IDLE_CONNECTIONS,
- (Gauge<Integer>) dataSource::getNumIdle);
- registerGauge(
- MetricNames.ENTITY_STORE_RELATION_DATASOURCE_MAX_CONNECTIONS,
- (Gauge<Integer>) dataSource::getMaxTotal);
+ MetricNames.DATASOURCE_ACTIVE_CONNECTIONS, (Gauge<Integer>)
dataSource::getNumActive);
+ registerGauge(MetricNames.DATASOURCE_IDLE_CONNECTIONS, (Gauge<Integer>)
dataSource::getNumIdle);
+ registerGauge(MetricNames.DATASOURCE_MAX_CONNECTIONS, (Gauge<Integer>)
dataSource::getMaxTotal);
}
}
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/session/SqlSessionFactoryHelper.java
b/core/src/main/java/org/apache/gravitino/storage/relational/session/SqlSessionFactoryHelper.java
index e31cf1683a..bc406af3b5 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/session/SqlSessionFactoryHelper.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/session/SqlSessionFactoryHelper.java
@@ -62,61 +62,68 @@ public class SqlSessionFactoryHelper {
*/
@SuppressWarnings("deprecation")
public void init(Config config) {
- // Initialize the data source
- BasicDataSource dataSource = new BasicDataSource();
- String jdbcUrl = config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL);
- String driverClass =
config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER);
- JdbcUrlUtils.validateJdbcConfig(driverClass, jdbcUrl,
config.getAllConfig());
-
- JDBCBackendType jdbcType = JDBCBackendType.fromURI(jdbcUrl);
- dataSource.setUrl(jdbcUrl);
- dataSource.setDriverClassName(driverClass);
-
dataSource.setUsername(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER));
-
dataSource.setPassword(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD));
- // Close the auto commit, so that we can control the transaction manual
commit
- dataSource.setDefaultAutoCommit(false);
- dataSource.setMaxWaitMillis(
- config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_WAIT_MILLISECONDS));
-
dataSource.setMaxTotal(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS));
- dataSource.setMaxIdle(5);
- dataSource.setMinIdle(0);
- dataSource.setLogAbandoned(true);
- dataSource.setRemoveAbandonedOnBorrow(true);
- dataSource.setRemoveAbandonedTimeout(60);
- dataSource.setTimeBetweenEvictionRunsMillis(Duration.ofMillis(10 * 60 *
1000L).toMillis());
- dataSource.setTestOnBorrow(true);
- dataSource.setTestWhileIdle(true);
- dataSource.setMinEvictableIdleTimeMillis(1000);
-
dataSource.setNumTestsPerEvictionRun(BaseObjectPoolConfig.DEFAULT_NUM_TESTS_PER_EVICTION_RUN);
- dataSource.setTestOnReturn(BaseObjectPoolConfig.DEFAULT_TEST_ON_RETURN);
- dataSource.setSoftMinEvictableIdleTimeMillis(
- BaseObjectPoolConfig.DEFAULT_SOFT_MIN_EVICTABLE_IDLE_TIME.toMillis());
- dataSource.setLifo(BaseObjectPoolConfig.DEFAULT_LIFO);
- MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();
- // Add null check to avoid NPE when metrics system is not initialized in
test environments
- if (metricsSystem != null) {
- // Register connection pool metrics when metrics system is available
- metricsSystem.register(new RelationDatasourceMetricsSource(dataSource));
+ // Create the SqlSessionFactory object, it is a singleton object
+ if (sqlSessionFactory != null) {
+ return;
}
- // Create the transaction factory and env
- TransactionFactory transactionFactory = new JdbcTransactionFactory();
- Environment environment = new Environment("development",
transactionFactory, dataSource);
- // Initialize the configuration
- Configuration configuration = new Configuration(environment);
- configuration.setDatabaseId(jdbcType.name().toLowerCase());
- ServiceLoader<MapperPackageProvider> loader =
ServiceLoader.load(MapperPackageProvider.class);
- for (MapperPackageProvider provider : loader) {
- provider.getMapperClasses().forEach(configuration::addMapper);
- }
+ synchronized (SqlSessionFactoryHelper.class) {
+ if (sqlSessionFactory != null) {
+ return;
+ }
- // Create the SqlSessionFactory object, it is a singleton object
- if (sqlSessionFactory == null) {
- synchronized (SqlSessionFactoryHelper.class) {
- if (sqlSessionFactory == null) {
- sqlSessionFactory = new
SqlSessionFactoryBuilder().build(configuration);
- }
+ // Initialize the data source
+ BasicDataSource dataSource = new BasicDataSource();
+ String jdbcUrl = config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL);
+ String driverClass =
config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER);
+ JdbcUrlUtils.validateJdbcConfig(driverClass, jdbcUrl,
config.getAllConfig());
+
+ JDBCBackendType jdbcType = JDBCBackendType.fromURI(jdbcUrl);
+ dataSource.setUrl(jdbcUrl);
+ dataSource.setDriverClassName(driverClass);
+
dataSource.setUsername(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER));
+
dataSource.setPassword(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD));
+ // Close the auto commit, so that we can control the transaction manual
commit
+ dataSource.setDefaultAutoCommit(false);
+ dataSource.setMaxWaitMillis(
+
config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_WAIT_MILLISECONDS));
+
dataSource.setMaxTotal(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS));
+ dataSource.setMaxIdle(5);
+ dataSource.setMinIdle(0);
+ dataSource.setLogAbandoned(true);
+ dataSource.setRemoveAbandonedOnBorrow(true);
+ dataSource.setRemoveAbandonedTimeout(60);
+ dataSource.setTimeBetweenEvictionRunsMillis(Duration.ofMillis(10 * 60 *
1000L).toMillis());
+ dataSource.setTestOnBorrow(true);
+ dataSource.setTestWhileIdle(true);
+ dataSource.setMinEvictableIdleTimeMillis(1000);
+
dataSource.setNumTestsPerEvictionRun(BaseObjectPoolConfig.DEFAULT_NUM_TESTS_PER_EVICTION_RUN);
+ dataSource.setTestOnReturn(BaseObjectPoolConfig.DEFAULT_TEST_ON_RETURN);
+ dataSource.setSoftMinEvictableIdleTimeMillis(
+
BaseObjectPoolConfig.DEFAULT_SOFT_MIN_EVICTABLE_IDLE_TIME.toMillis());
+ dataSource.setLifo(BaseObjectPoolConfig.DEFAULT_LIFO);
+
+ // Create the transaction factory and env
+ TransactionFactory transactionFactory = new JdbcTransactionFactory();
+ Environment environment = new Environment("development",
transactionFactory, dataSource);
+
+ // Initialize the configuration
+ Configuration configuration = new Configuration(environment);
+ configuration.setDatabaseId(jdbcType.name().toLowerCase());
+ ServiceLoader<MapperPackageProvider> loader =
ServiceLoader.load(MapperPackageProvider.class);
+ for (MapperPackageProvider provider : loader) {
+ provider.getMapperClasses().forEach(configuration::addMapper);
}
+
+ MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();
+ // Add null check to avoid NPE when metrics system is not initialized in
test environments
+ if (metricsSystem != null) {
+ // Register connection pool metrics when metrics system is available
+ metricsSystem.register(new
RelationDatasourceMetricsSource(dataSource));
+ }
+
+ // Create the SqlSessionFactory object, it is a singleton object
+ sqlSessionFactory = new SqlSessionFactoryBuilder().build(configuration);
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/metrics/TestExtractMetricNameAndLabel.java
b/core/src/test/java/org/apache/gravitino/metrics/TestExtractMetricNameAndLabel.java
index 1d6e8b74e2..4f3a1feba2 100644
---
a/core/src/test/java/org/apache/gravitino/metrics/TestExtractMetricNameAndLabel.java
+++
b/core/src/test/java/org/apache/gravitino/metrics/TestExtractMetricNameAndLabel.java
@@ -81,33 +81,30 @@ public class TestExtractMetricNameAndLabel {
ImmutableMap.of("operation", "update-table"));
checkResult(
- MetricsSource.GRAVITINO_SERVER_METRIC_NAME
+ MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME
+ "."
- + MetricNames.ENTITY_STORE_RELATION_DATASOURCE_ACTIVE_CONNECTIONS,
-
Collector.sanitizeMetricName(MetricsSource.GRAVITINO_SERVER_METRIC_NAME)
+ + MetricNames.DATASOURCE_ACTIVE_CONNECTIONS,
+
Collector.sanitizeMetricName(MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME)
+ "_"
- + Collector.sanitizeMetricName(
-
MetricNames.ENTITY_STORE_RELATION_DATASOURCE_ACTIVE_CONNECTIONS),
+ +
Collector.sanitizeMetricName(MetricNames.DATASOURCE_ACTIVE_CONNECTIONS),
ImmutableMap.of());
checkResult(
- MetricsSource.GRAVITINO_SERVER_METRIC_NAME
+ MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME
+ "."
- + MetricNames.ENTITY_STORE_RELATION_DATASOURCE_IDLE_CONNECTIONS,
-
Collector.sanitizeMetricName(MetricsSource.GRAVITINO_SERVER_METRIC_NAME)
+ + MetricNames.DATASOURCE_IDLE_CONNECTIONS,
+
Collector.sanitizeMetricName(MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME)
+ "_"
- + Collector.sanitizeMetricName(
- MetricNames.ENTITY_STORE_RELATION_DATASOURCE_IDLE_CONNECTIONS),
+ +
Collector.sanitizeMetricName(MetricNames.DATASOURCE_IDLE_CONNECTIONS),
ImmutableMap.of());
checkResult(
- MetricsSource.GRAVITINO_SERVER_METRIC_NAME
+ MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME
+ "."
- + MetricNames.ENTITY_STORE_RELATION_DATASOURCE_MAX_CONNECTIONS,
-
Collector.sanitizeMetricName(MetricsSource.GRAVITINO_SERVER_METRIC_NAME)
+ + MetricNames.DATASOURCE_MAX_CONNECTIONS,
+
Collector.sanitizeMetricName(MetricsSource.GRAVITINO_RELATIONAL_STORE_METRIC_NAME)
+ "_"
- + Collector.sanitizeMetricName(
- MetricNames.ENTITY_STORE_RELATION_DATASOURCE_MAX_CONNECTIONS),
+ +
Collector.sanitizeMetricName(MetricNames.DATASOURCE_MAX_CONNECTIONS),
ImmutableMap.of());
}
}
diff --git
a/core/src/test/java/org/apache/gravitino/metrics/TestMetricsSystem.java
b/core/src/test/java/org/apache/gravitino/metrics/TestMetricsSystem.java
index 588e939f8d..35ff879c9e 100644
--- a/core/src/test/java/org/apache/gravitino/metrics/TestMetricsSystem.java
+++ b/core/src/test/java/org/apache/gravitino/metrics/TestMetricsSystem.java
@@ -19,6 +19,7 @@
package org.apache.gravitino.metrics;
+import org.apache.gravitino.metrics.source.MetricsSource;
import org.apache.gravitino.metrics.source.TestMetricsSource;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -26,8 +27,30 @@ import org.junit.jupiter.api.Test;
public class TestMetricsSystem {
private MetricsSystem metricsSystem = new MetricsSystem();
+ private static class MockMetricsSource extends MetricsSource {
+ public MockMetricsSource(String metricsSourceName) {
+ super(metricsSourceName);
+ }
+ }
+
+ private static class Mock2MetricsSource extends MetricsSource {
+ public Mock2MetricsSource(String metricsSourceName) {
+ super(metricsSourceName);
+ }
+ }
+
+ @Test
+ void testRegisterMetricsWithSameMetricsSourceName() {
+ MockMetricsSource metricsSource = new MockMetricsSource("a");
+ Mock2MetricsSource metricsSource2 = new Mock2MetricsSource("a");
+ metricsSystem.register(metricsSource);
+ Assertions.assertThrowsExactly(
+ IllegalStateException.class, () ->
metricsSystem.register(metricsSource2));
+ Assertions.assertDoesNotThrow(() -> metricsSystem.register(metricsSource));
+ }
+
@Test
- void testRegisterMetricsSource() {
+ void testRegisterMetricsSourceInRaceCondition() {
TestMetricsSource metricsSource = new TestMetricsSource();
metricsSystem.register(metricsSource);
metricsSource.incCounter("a.b");
