Skip to content

fix(druid): optimize dataSourceName to resolve metrics high cardinality#19108

Open
YaoYingLong wants to merge 2 commits into
open-telemetry:mainfrom
YaoYingLong:feature/alibaba-druid
Open

fix(druid): optimize dataSourceName to resolve metrics high cardinality#19108
YaoYingLong wants to merge 2 commits into
open-telemetry:mainfrom
YaoYingLong:feature/alibaba-druid

Conversation

@YaoYingLong

Copy link
Copy Markdown
Contributor

Problem

If DruidDataSource.setName() is not explicitly invoked to configure the data source name, DruidDataSourceStatManager uses System.identityHashCode(dataSource) as the default dataSourceName. This identifier varies across different application instances and changes every time the service restarts, leading to serious high cardinality of metric tags. It causes sharp growth of time-series storage volume and degrades index query performance.

Solution

Optimize the parsing logic of the Alibaba Druid dataSourceName metric attribute to avoid instance-specific temporary hash values, effectively mitigate high cardinality and reduce storage and query pressure.

Copilot AI review requested due to automatic review settings June 30, 2026 12:17
@YaoYingLong
YaoYingLong requested a review from a team as a code owner June 30, 2026 12:17

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 metrics high-cardinality problem in the Alibaba Druid connection-pool instrumentation. Previously, when a DruidDataSource had no explicit name, Druid falls back to System.identityHashCode(dataSource) for the JMX ObjectName id, which the instrumentation used as the pool.name metric attribute. That value changes every restart and per instance, exploding time-series cardinality. The change derives the pool name from the actual name argument (falling back to "unknown") and adds process-wide deduplication so colliding names get a stable -2, -3, ... suffix instead of an identity hash.

Changes:

  • Javaagent advice now matches the addDataSource(Object, String) overload and builds the pool name from the name argument ("unknown" when null) instead of the JMX ObjectName id.
  • Library ConnectionPoolMetrics introduces a name-reservation mechanism (activeDataSourceNames set + DataSourceMetricsRegistration) to deduplicate concurrent/duplicate pool names and release them on unregister.
  • Tests are expanded to cover duplicate names, name reuse after shutdown, reserved-suffix skipping, and the null-name "unknown" fallback.

Reviewed changes

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

Show a summary per file
File Description
.../library/src/main/.../ConnectionPoolMetrics.java Adds name reservation/dedup, refactors callback creation, wraps registration in a holder that releases the reserved name on close.
.../javaagent/src/main/.../DruidDataSourceInstrumentation.java Narrows the advice to addDataSource(Object, String) and derives the pool name from the name arg with an "unknown" fallback.
.../testing/src/main/.../AbstractDruidInstrumentationTest.java Adds shared helpers and new tests for duplicate names, reuse-after-shutdown, and reserved-suffix handling; semconv-aware metric/attribute keys.
.../javaagent/src/test/.../DruidInstrumentationTest.java Adds a test asserting the "unknown" pool name when the data source name is null.
.../library/src/test/.../DruidInstrumentationTest.java Updates configure to map a null name to "unknown" when building the ObjectName.

DruidDataSourceMBean druidDataSource = (DruidDataSourceMBean) dataSource;
String dataSourceName =
objectName.getKeyProperty("type") + "-" + objectName.getKeyProperty("id");
objectName.getKeyProperty("type") + "-" + (name == null ? "unknown" : name);

@laurit laurit Jul 1, 2026

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.

I think that we usually just omit name components that are not present instead of using placeholders like unknown.
Noticed that spec https://opentelemetry.io/docs/specs/semconv/registry/attributes/db/#db-client-connection-pool-name provides some guidance on how to create a name when it is not available. Idk whether this is a viable strategy since these properties might not be available either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In typical business scenarios, when configuring a data source using DruidDataSource, the setName or setObjectName methods of DruidDataSource are not called by default. Similarly, in Spring projects, the name property is generally not configured via application.yml. The current implementation will definitely result in a high cardinality issue, and we can simply omit these fields as you mentioned, since neither the type nor the passed name attribute in objectName may be obtainable.

I’m wondering if we should skip the unavailable attribute as long as either one of the two can be retrieved. If neither attribute can be obtained, we need a fallback strategy with a default value such as unknown, default, or something component-related like druid.

@laurit @trask Could you two please review this and share any better suggestions?

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 1, 2026

Copy link
Copy Markdown

Pull request dashboard status

Status last refreshed: 2026-07-22 03:18:59 UTC.

  • Waiting on: Author
  • Next step: Address or respond to 2 review feedback items:
    • Inline threads: 1, 2
    • 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.

});
}

private static String reserveDataSourceName(String dataSourceName) {

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.

So the idea here is that when there are multiple data sources with the same name we'll append a suffix. Since we are not doing this for other connection pool instrumentations it is questionable whether we should be doing this here. If we decide to do this then we should apply the same strategy across the connection pool instrumentations.
cc @trask any suggestions on how to handle this?

@YaoYingLong

Copy link
Copy Markdown
Contributor Author

@laurit @trask Could you two please review this proposal.
An alternative approach follows the implementation used in PR #19160, #19173 and #19159: assign a default value combined with an auto-increment ID for cases where no configuration is specified.
Feel free to share any better suggestions, and I will make corresponding revisions accordingly.
In real-world development scenarios, developers rarely configure the data source name manually. Only a small number of users will specify it when multiple data sources coexist. This field is largely unnecessary and of little practical value for business applications. It is only useful for troubleshooting when faults occur.
However, the current implementation generates a high-cardinality value by default, which is unfavorable for time-series queries.

return dataSourceName;
}
for (int count = 2; ; count++) {
String candidate = dataSourceName + "-" + count;

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.

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.

4 participants