Skip to content

fix: optimize dataSourceName to resolve metrics high cardinality#19159

Open
YaoYingLong wants to merge 1 commit into
open-telemetry:mainfrom
YaoYingLong:feature/c3p0-0.9
Open

fix: optimize dataSourceName to resolve metrics high cardinality#19159
YaoYingLong wants to merge 1 commit into
open-telemetry:mainfrom
YaoYingLong:feature/c3p0-0.9

Conversation

@YaoYingLong

Copy link
Copy Markdown
Contributor

Summary

Fix high-cardinality c3p0 connection pool metric attributes when dataSourceName is not explicitly configured.

Previously, c3p0 metrics used PooledDataSource#getDataSourceName() directly as the connection pool name. In c3p0, if users do not call setDataSourceName(...), the underlying dataSourceName property remains unset, but getDataSourceName() falls back to getIdentityToken(). That identity token is generated per data source instance and includes VM/object-identity based values, so using it as pool.name / db.client.connection.pool.name can create high-cardinality metric attributes.

This change keeps explicitly configured dataSourceName values unchanged, but replaces c3p0 identity-token fallback values with generated low-cardinality names like c3p0-1.

Details

c3p0 documents dataSourceName as defaulting to either the named config name or an opaque identity token when no explicit name is set:
https://www.mchange.com/projects/c3p0/#dataSourceName

The fallback behavior is implemented in AbstractPoolBackedDataSource#getDataSourceName(), which returns getIdentityToken() when the stored dataSourceName is null:
https://github.com/swaldman/c3p0/blob/9084ab6577bb195672b8831d8fdbb9be22f76ebe/src/com/mchange/v2/c3p0/impl/AbstractPoolBackedDataSource.java#L1506-L1521

The identity token is generated by C3P0ImplUtils.allocateIdentityToken(...) using VM/object-identity based values:
https://github.com/swaldman/c3p0/blob/9084ab6577bb195672b8831d8fdbb9be22f76ebe/src/com/mchange/v2/c3p0/impl/C3P0ImplUtils.java#L1407-L1452

Changes

  • Preserve user-provided c3p0 dataSourceName values for connection pool metrics.
  • Detect the c3p0 identity-token fallback and replace it with a generated default pool name.
  • Keep generated names stable for the registered data source lifetime.
  • Remove generated name state when metrics are unregistered.
  • Add test coverage for both explicit dataSourceName and unset dataSourceName cases.

Copilot AI review requested due to automatic review settings July 9, 2026 10:58
@YaoYingLong
YaoYingLong requested a review from a team as a code owner July 9, 2026 10:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a high-cardinality metrics issue in the c3p0-0.9 connection pool instrumentation. c3p0's PooledDataSource#getDataSourceName() falls back to a per-instance identityToken when a user does not explicitly call setDataSourceName(...). Using that token as pool.name / db.client.connection.pool.name produces high-cardinality metric attributes. The change detects this fallback and substitutes a stable, low-cardinality generated name (c3p0-N), while preserving explicitly configured names.

Changes:

  • In ConnectionPoolMetrics, resolve the pool name via a new getPoolName(...) helper that keeps explicit dataSourceName values but replaces the identity-token fallback with a generated c3p0-N name cached per data source.
  • Track generated names in a generatedPoolNames map keyed by the identity-based IdentityDataSourceKey, and clean it up in unregisterMetrics.
  • Update the shared abstract test to cover both the explicit dataSourceName case and the generated-default-name case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
instrumentation/c3p0-0.9/library/src/main/java/io/opentelemetry/instrumentation/c3p0/v0_9/ConnectionPoolMetrics.java Adds getPoolName to detect the identity-token fallback and assign a stable generated name, with cleanup on unregister.
instrumentation/c3p0-0.9/testing/src/main/java/io/opentelemetry/instrumentation/c3p0/AbstractC3p0InstrumentationTest.java Sets an explicit pool name in the existing test and adds a new test verifying the generated c3p0-N default name.

I reviewed the pool-name resolution logic (null-check-before-equals avoids NPE; the fallback is reliably detected since getDataSourceName() returns exactly getIdentityToken() when unset), verified the two ConcurrentHashMaps use the same identity-based key and are both cleaned up on unregister, confirmed there is no nested same-map compute/computeIfAbsent hazard, and checked that the generated-name approach is consistent with sibling instrumentations (e.g. apache-dbcp's dbcp2-<id> fallback). The tests cover both code paths. I did not find substantive issues.

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

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.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 13, 2026

Copy link
Copy Markdown

Pull request dashboard status

Status last refreshed: 2026-07-22 03:19:10 UTC.

  • Waiting on: Author
  • Next step: Address or respond to 1 review feedback item:
    • Inline threads: 1
    • For each item, link to the commit that addresses it, explain why no change is needed, or ask a follow-up question.

This automated status or its linked feedback items may be incorrect. If something looks wrong, report it with the result you expected.

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