Skip to content

fix: optimize dataSourceName to resolve metrics high cardinality #19159

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.

Sign up for GitHub

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

Open
YaoYingLong wants to merge 1 commit into
base: main
Choose a base branch
from
+87 −9
Open

fix: optimize dataSourceName to resolve metrics high cardinality #19159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions ...brary/src/main/java/io/opentelemetry/instrumentation/c3p0/v0_9/ConnectionPoolMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.sql.SQLException;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.logging.Logger;
import javax.annotation.Nullable;

Expand All @@ -24,12 +25,16 @@ final class ConnectionPoolMetrics {
private static final Logger logger = Logger.getLogger(ConnectionPoolMetrics.class.getName());

private static final String INSTRUMENTATION_NAME = "io.opentelemetry.c3p0-0.9";
private static final String DEFAULT_POOL_NAME = "c3p0";
private static final AtomicInteger idGenerator = new AtomicInteger(1);

// a weak map does not make sense here because each Meter holds a reference to the dataSource
// PooledDataSource implements equals() & hashCode() in IdentityTokenResolvable,
// that's why we wrap it with IdentityDataSourceKey that uses identity comparison instead
private static final Map<IdentityDataSourceKey, BatchCallback> dataSourceMetrics =
new ConcurrentHashMap<>();
private static final Map<IdentityDataSourceKey, String> generatedPoolNames =
new ConcurrentHashMap<>();

static void registerMetrics(OpenTelemetry openTelemetry, PooledDataSource dataSource) {
dataSourceMetrics.compute(
Expand All @@ -48,8 +53,7 @@ private static BatchCallback createMeters(
PooledDataSource dataSource = key.dataSource;

DbConnectionPoolMetrics metrics =
DbConnectionPoolMetrics.create(
openTelemetry, INSTRUMENTATION_NAME, dataSource.getDataSourceName());
DbConnectionPoolMetrics.create(openTelemetry, INSTRUMENTATION_NAME, getPoolName(key));

ObservableLongMeasurement connections = metrics.connections();
ObservableLongMeasurement pendingRequestsForConnection = metrics.pendingRequestsForConnection();
Expand All @@ -75,8 +79,20 @@ private static BatchCallback createMeters(
pendingRequestsForConnection);
}

private static String getPoolName(IdentityDataSourceKey key) {
PooledDataSource dataSource = key.dataSource;
String dataSourceName = dataSource.getDataSourceName();
if (dataSourceName == null || dataSourceName.equals(dataSource.getIdentityToken())) {
return generatedPoolNames.computeIfAbsent(
key, unused -> DEFAULT_POOL_NAME + "-" + idGenerator.getAndIncrement());

@trask trask Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pending the discussion in #19160 (comment), I think we should avoid process-local -N suffixes here. If no stable name exists, use a stable fallback and let duplicates merge so metric identities remain consistent across restarts.

}
return dataSourceName;
}

static void unregisterMetrics(PooledDataSource dataSource) {
BatchCallback callback = dataSourceMetrics.remove(new IdentityDataSourceKey(dataSource));
IdentityDataSourceKey key = new IdentityDataSourceKey(dataSource);
BatchCallback callback = dataSourceMetrics.remove(key);
generatedPoolNames.remove(key);
removeMetersFromRegistry(callback);
}

Expand Down
74 changes: 68 additions & 6 deletions .../src/main/java/io/opentelemetry/instrumentation/c3p0/AbstractC3p0InstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@

package io.opentelemetry.instrumentation.c3p0;

import static io.opentelemetry.api.common.AttributeKey.stringKey;
import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableDatabaseSemconv;
import static org.assertj.core.api.Assertions.assertThat;

import com.mchange.v2.c3p0.ComboPooledDataSource;
import com.mchange.v2.c3p0.PooledDataSource;
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
import io.opentelemetry.instrumentation.testing.junit.db.DbConnectionPoolMetricsAssertions;
import io.opentelemetry.instrumentation.testing.junit.db.MockDriver;
Expand All @@ -21,6 +23,9 @@

public abstract class AbstractC3p0InstrumentationTest {
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.c3p0-0.9";
private static final String DEFAULT_POOL_NAME = "c3p0";
private static final AttributeKey<String> POOL_NAME_KEY =
stringKey(emitStableDatabaseSemconv() ? "db.client.connection.pool.name" : "pool.name");

protected abstract InstrumentationExtension testing();

Expand All @@ -39,14 +44,18 @@ void shouldReportMetrics() throws Exception {
ComboPooledDataSource c3p0DataSource = new ComboPooledDataSource();
c3p0DataSource.setDriverClass(MockDriver.class.getName());
c3p0DataSource.setJdbcUrl("jdbc:mock:testDatabase");
c3p0DataSource.setDataSourceName("testPool");

// when
try (Connection connection = c3p0DataSource.getConnection()) {
configure(c3p0DataSource);
}

// then
assertDataSourceMetrics(c3p0DataSource);
String dataSourceName = c3p0DataSource.getDataSourceName();

assertThat(dataSourceName).isNotEmpty();
assertDataSourceMetrics(dataSourceName);

// when
shutdown(c3p0DataSource);
Expand All @@ -71,13 +80,66 @@ void shouldReportMetrics() throws Exception {
AbstractIterableAssert::isEmpty);
}

private void assertDataSourceMetrics(PooledDataSource dataSource) {
String dataSourceName = dataSource.getDataSourceName();
@Test
void shouldUseGeneratedDefaultPoolName() throws Exception {
// given
ComboPooledDataSource c3p0DataSource = new ComboPooledDataSource();
c3p0DataSource.setDriverClass(MockDriver.class.getName());
c3p0DataSource.setJdbcUrl("jdbc:mock:testDatabase");

assertThat(dataSourceName).isNotEmpty();
// when
try (Connection connection = c3p0DataSource.getConnection()) {
configure(c3p0DataSource);
configure(c3p0DataSource);
}

// then
String identityToken = c3p0DataSource.getIdentityToken();
assertThat(c3p0DataSource.getDataSourceName()).isEqualTo(identityToken);
assertGeneratedDefaultPoolName();

// when
shutdown(c3p0DataSource);
c3p0DataSource.close();

testing().clearData();

// then
testing()
.waitAndAssertMetrics(
INSTRUMENTATION_NAME,
emitStableDatabaseSemconv()
? "db.client.connection.count"
: "db.client.connections.usage",
AbstractIterableAssert::isEmpty);
testing()
.waitAndAssertMetrics(
INSTRUMENTATION_NAME,
emitStableDatabaseSemconv()
? "db.client.connection.pending_requests"
: "db.client.connections.pending_requests",
AbstractIterableAssert::isEmpty);
}

private void assertGeneratedDefaultPoolName() {
testing()
.waitAndAssertMetrics(
INSTRUMENTATION_NAME,
emitStableDatabaseSemconv()
? "db.client.connection.count"
: "db.client.connections.usage",
metrics ->
metrics.anySatisfy(
metric ->
assertThat(metric.getLongSumData().getPoints())
.allSatisfy(
point ->
assertThat(point.getAttributes().get(POOL_NAME_KEY))
.matches(DEFAULT_POOL_NAME + "-\\d+"))));
}

DbConnectionPoolMetricsAssertions.create(
testing(), INSTRUMENTATION_NAME, dataSource.getDataSourceName())
private void assertDataSourceMetrics(String poolName) {
DbConnectionPoolMetricsAssertions.create(testing(), INSTRUMENTATION_NAME, poolName)
.disableMinIdleConnections()
.disableMaxIdleConnections()
.disableMaxConnections()
Expand Down
Loading