Skip to content

Remove JMX registration requirement for Apache DBCP 2.0 metrics#19160

Open
YaoYingLong wants to merge 14 commits into
open-telemetry:mainfrom
YaoYingLong:feature/apache-dbcp-2.0
Open

Remove JMX registration requirement for Apache DBCP 2.0 metrics#19160
YaoYingLong wants to merge 14 commits into
open-telemetry:mainfrom
YaoYingLong:feature/apache-dbcp-2.0

Conversation

@YaoYingLong

@YaoYingLong YaoYingLong commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Make Apache DBCP 2.0 connection pool metrics independent of JMX registration by registering metrics after startPoolMaintenance() initializes the pool. Metric callbacks remain active through MBean deregistration and are removed when BasicDataSource.close() completes, including exceptional exits.

Resolve pool names from the registered JMX ObjectName, then the JDBC URL and connection properties, with apache-dbcp2 as the fallback. This does not change any public API or configuration property.

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

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 removes the hard dependency on JMX/MBeanRegistration for Apache DBCP 2.0 connection‑pool metrics auto‑instrumentation. Previously, metrics were only registered when a BasicDataSource was registered to an MBeanServer (via preRegister), which meant instrumentation silently did nothing outside JMX‑enabled setups (e.g. Spring Boot). The instrumentation now hooks the BasicDataSource#createDataSource() lifecycle so metrics are collected regardless of JMX, with a generated dbcp2-N fallback name when no name is otherwise available.

Changes:

  • Switch the javaagent advice from preRegister/postDeregister to createDataSource (registration) and close/postDeregister (cleanup), resolving the data source name from getJmxName() → registered JMX ObjectName → generated default.
  • Add a same‑package helper OpenTelemetryBasicDataSourceUtil (registered via isHelperClass) to access BasicDataSource#getRegisteredJmxName(), and expose DataSourceMetrics.getDefaultName() for the dbcp2-N fallback.
  • Refactor the shared abstract test to support JMX‑less registration, add javaagent tests for default/registered/preferred naming, and update README/metadata docs.

Reviewed changes

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

Show a summary per file
File Description
.../v2_0/BasicDataSourceInstrumentation.java Re‑targets advice to createDataSource/close/postDeregister and adds the new name‑resolution order.
.../org/apache/commons/dbcp2/OpenTelemetryBasicDataSourceUtil.java New same‑package accessor for getRegisteredJmxName().
.../v2_0/ApacheDbcpInstrumentationModule.java Registers the new helper class via isHelperClass.
.../v2_0/DataSourceMetrics.java Made public and adds getDefaultName() counter for fallback names.
.../v2_0/ApacheDbcpInstrumentationTest.java (javaagent) Adds tests for default/registered/preferred JMX naming.
.../AbstractApacheDbcpInstrumentationTest.java (testing) Refactors shared setup/assertions for JMX‑less flow.
javaagent/README.md, metadata.yaml Documents that JMX registration is no longer required.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 9, 2026

Copy link
Copy Markdown

Pull request dashboard status

Status last refreshed: 2026-07-22 01:49:46 UTC.

  • Waiting on: Reviewers
  • Next step: Review the latest changes.

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

@trask

trask commented Jul 10, 2026

Copy link
Copy Markdown
Member

can you update PR description to include motivation for why this PR? e.g.

Decouple Apache DBCP 2.0 connection-pool metrics from JMX registration. The javaagent currently detects the datasource lifecycle only through MBeanRegistration callbacks, so an active BasicDataSource produces no metrics unless it is registered with an MBeanServer.

Register metrics after successful pool initialization by instrumenting startPoolMaintenance(), and unregister them when the datasource is closed. Keep postDeregister() cleanup for JMX-managed pools. This also supports datasource restart without requiring JMX.

Preserve meaningful pool names by preferring the configured JMX name, then the registered JMX ObjectName, and falling back to a generated dbcp2-N name. Add coverage for non-JMX pools and each naming path, and update the instrumentation documentation to reflect that JMX is no longer required.

@YaoYingLong

Copy link
Copy Markdown
Contributor Author

@trask The PR description has been updated, with additional explanations added to clarify the rationale behind this PR submission.

@trask
trask requested a review from Copilot July 16, 2026 15:53
@trask trask added this to the v2.30.0 milestone Jul 16, 2026
@trask

trask commented Jul 16, 2026

Copy link
Copy Markdown
Member

is this a breaking change? if so, let's put it behind the v3preview flag and try to get it into v2.30.0

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Comment thread instrumentation/apache-dbcp-2.0/metadata.yaml Outdated
Comment thread instrumentation/apache-dbcp-2.0/javaagent/README.md Outdated
@YaoYingLong

Copy link
Copy Markdown
Contributor Author

is this a breaking change? if so, let's put it behind the v3preview flag and try to get it into v2.30.0

@trask From my perspective, this is not a breaking change. It merely modifies the metric attribute name, switching the original high-cardinality value to a low-cardinality one.

If you determine this counts as a breaking change, do we need to refactor the code for backward compatibility using a method like AgentCommonConfig.get().isV3Preview()? Please let me know if such adaptation is required, and I will implement the corresponding revisions.

@trask

trask commented Jul 19, 2026

Copy link
Copy Markdown
Member

is this a breaking change?

I think no:

Existing users on the normal successful JMX path retain the name derived from their registered ObjectName. Non-JMX users previously received no Apache DBCP metrics, so the JDBC-derived or fallback name belongs to newly added telemetry and has no existing name to preserve.

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

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

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

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

@trask trask removed this from the v2.30.0 milestone Jul 20, 2026
@trask
trask requested a review from Copilot July 20, 2026 16:09

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

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

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