You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wuwen5 (via GitHub)" <gi...@apache.org> on 2026/09/22 03:48:39 UTC
[PR] Fix/hikaricp metric split [skywalking-java]
wuwen5 opened a new pull request, #833:
URL: https://github.com/apache/skywalking-java/pull/833
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 🔌 Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist 👇 ====
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
==== 🔌 Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
This PR separates HikariCP datasource metrics by category to avoid mixing connection-count metrics with time-based configuration metrics in the same metric family.
Previously, these metrics had different units and scales, which could distort line-chart visualization and make connection-related trends difficult to interpret. Separating the metrics ensures consistent units and improves the accuracy and readability of datasource metric visualization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/hikaricp metric split [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #833:
URL: https://github.com/apache/skywalking-java/pull/833#issuecomment-5770944330
Could you be clear how the old metrics look like? and how about your new proposals?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/hikaricp metric split [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #833:
URL: https://github.com/apache/skywalking-java/pull/833#issuecomment-5777076539
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Stop reporting datasource timeout configuration as metrics [skywalking-java]
Posted by "wuwen5 (via GitHub)" <gi...@apache.org>.
wuwen5 commented on PR #833:
URL: https://github.com/apache/skywalking-java/pull/833#issuecomment-5791832118
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Stop reporting datasource timeout configuration as metrics [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #833:
URL: https://github.com/apache/skywalking-java/pull/833
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Stop reporting datasource timeout configuration as metrics [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #833:
URL: https://github.com/apache/skywalking-java/pull/833#issuecomment-5790080683
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org