Skip to content

Stop reporting datasource timeout configuration as metrics - #833

Merged
wu-sheng merged 8 commits into
apache:mainfrom
wuwen5:fix/hikaricp-metric-split
Sep 23, 2026
Merged

wu-sheng merged 8 commits into
apache:mainfrom
wuwen5:fix/hikaricp-metric-split

Conversation

@wuwen5

@wuwen5 wuwen5 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor
  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

This PR stops reporting datasource timeout configuration as metrics across the c3p0, DBCP and HikariCP plugins.

Previously, timeout-related values such as c3p0's maxIdleTime, DBCP's maxWaitMillis, and HikariCP's connectionTimeout, validationTimeout, idleTimeout and leakDetectionThreshold were exported as datasource gauges. These are static configuration values rather than runtime pool status, and mixing them into the datapool metrics — with units and scales that differ from connection counts — distorted line-chart visualization and made connection-related trends hard to interpret.

Removing them keeps the datasource metric family focused on real runtime pool state, so the remaining gauges share consistent semantics and the visualization is accurate and readable.

old
image

new
image

@wu-sheng wu-sheng added enhancement New feature or request plugin labels Sep 22, 2026
@wu-sheng wu-sheng added this to the 9.8.0 milestone Sep 22, 2026
@wu-sheng

Copy link
Copy Markdown
Member

Could you be clear how the old metrics look like? and how about your new proposals?

@wu-sheng

Copy link
Copy Markdown
Member

Thanks for the PR. The mixed-unit chart is a real problem, but I don't think a new metric is the right fix.

  1. OAP's only MAL rule for this is meter-analyzer-config/datasource.yaml, and it reads just datasource (datasource.sum(['service', 'instance', 'name', 'status'])). Nothing consumes datasource_time, so the agent would keep collecting these four values and OAP would silently drop them. They'd never be stored or queryable.
  2. connectionTimeout, validationTimeout, idleTimeout and leakDetectionThreshold are pool configuration, not runtime metrics. They're flat lines. HikariCP's own Micrometer tracker doesn't export them either.

So I'd suggest we stop collecting them instead of adding a new metric:

  • Remove the four timeout gauges from PoolingSealInterceptor. Keep minimumIdle and maximumPoolSize in datasource: they're counted in connections, and they serve as limit lines for the active/total connection counts.
  • Remove the four entries from hikaricp-scenario/config/expectedData.yaml, rather than renaming them.
  • Update the CHANGES.md line to say these config values are no longer reported.

No OAP MAL or UI changes are needed. The datasource rule sums over every status value, so it keeps working unchanged. The UI's datasource chart will then show only connection counts, which fixes the chart you showed.

For consistency, please do the same for dbcp's maxWaitMillis and c3p0's maxIdleTime, which are the same kind of time-based config in the same metric family. That means updating dbcp-2.x-scenario and both c3p0 scenarios. Either this PR or a follow-up works.

The runtime signals about connection timeouts are already covered:

  • threadsAwaitingConnection greater than 0 shows the pool is running out of connections.
  • An actual getConnection() timeout marks the HikariCP/Connection/getConnection span as an error, with the exception attached.

@wuwen5 wuwen5 changed the title Fix/hikaricp metric split Stop reporting datasource timeout configuration as metrics Sep 23, 2026
@wu-sheng

Copy link
Copy Markdown
Member

Thanks for the update, this is what I had in mind. The rest looks good: the removals are consistent across HikariCP, DBCP and c3p0, the expected data drops the entries instead of renaming them, and the meterSize values match what the plugins now emit. No OAP MAL or UI change is needed.

Two nits:

  1. CHANGES.md has trailing whitespace at the end of the first line of the new entry.
  2. The new javadoc on PoolingSealInterceptor says "Intercepts HikariCP connection acquisition", but the interceptor targets HikariDataSource#seal, which runs once when the pool config is sealed on the first getConnection(). Something like "Registers HikariCP datasource metrics when the pool is sealed" would describe it more accurately.

@wuwen5

wuwen5 commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the update, this is what I had in mind. The rest looks good: the removals are consistent across HikariCP, DBCP and c3p0, the expected data drops the entries instead of renaming them, and the meterSize values match what the plugins now emit. No OAP MAL or UI change is needed.

Two nits:

  1. CHANGES.md has trailing whitespace at the end of the first line of the new entry.
  2. The new javadoc on PoolingSealInterceptor says "Intercepts HikariCP connection acquisition", but the interceptor targets HikariDataSource#seal, which runs once when the pool config is sealed on the first getConnection(). Something like "Registers HikariCP datasource metrics when the pool is sealed" would describe it more accurately.

Fixed both nits. Thanks for the review.

@wu-sheng
wu-sheng merged commit 51be6a9 into apache:main Sep 23, 2026
263 of 373 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants