[jdbc] Capture custom object types in prepared statement parameter instrumentation#19093
[jdbc] Capture custom object types in prepared statement parameter instrumentation#19093CodingFabian wants to merge 3 commits into
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes the JDBC prepared-statement parameter capture (opt-in via CAPTURE_QUERY_PARAMETERS) so that values set via setObject() with custom types are no longer silently dropped. It replaces the hardcoded instanceof whitelist (Boolean, Number, String, Date, Time, Timestamp, URL, RowId) with a simple null check that calls toString() on any non-null value, addressing the ORM-managed ID type case from #7413.
Changes:
- Removed the type whitelist and unused imports in all three parameter-capture advice blocks, capturing any non-null value via
toString(). - Added
testCustomObjectPreparedStatementParameterand a serializableIdTypehelper to verify custom objects are captured.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| instrumentation/jdbc/javaagent/.../PreparedStatementInstrumentation.java | Replaces instanceof whitelist with null check + toString() in three advice blocks; drops now-unused imports |
| instrumentation/jdbc/testing/.../AbstractPreparedStatementParametersTest.java | Adds a parameterized test and a Serializable IdType to confirm custom object capture |
CodingFabian
force-pushed
the
jdbc-capture-custom-object-parameters
branch
from
fc0bc9f to
838d0b3
Compare
…strumentation Previously, setObject() calls with custom types (e.g. ORM-managed ID classes) were silently dropped because the instrumentation only captured a hardcoded whitelist of known JDBC types. Any class not in the list produced no parameter attribute at all, indistinguishable from a parameter that was never set.
CodingFabian
force-pushed
the
jdbc-capture-custom-object-parameters
branch
from
838d0b3 to
2903ab5
Compare
|
Copilot tells me the test errors are unrelated. If anybody can point me at something missing in my PR, I will fix it. |
At first glance I'd say copilot is mistaken. Have a look at https://scans.gradle.com/s/j6e4p4kwengm6 I'd say the issue is that some jdbc drivers used in the test are not happy with what the test is doing. |
|
@laurit thank you. I can now spot the issue as well. I will see how I can write the test in a way that it is accepted. |
laurit
left a comment
laurit
left a comment
There was a problem hiding this comment.
Does your real application also use Serializable id type like this? What does it end up inserting into the db, a serialized java object?
While I'm not completely opposed to this it does feel somewhat arbitrary. The sqllite driver used in the test seems to use the toString() value while the hsqldb seems to serialize the object. So by using the toString we don't really capture the value that was sent to the db. It is certain that not all objets passed to setObject are going to have meaningful toString methods. We need to decide whether that is a problem for us. Invoking toString isn't completely risk free as theoretically it could mutate instance state, but I think the chances of that happening are low. A slightly bigger risk it probably that it will throw a NPE or some other exception. @open-telemetry/java-instrumentation-approvers any suggestions?
FWIW, |
|
There is another line even more relevant for pg: https://github.com/pgjdbc/pgjdbc/blob/394800a38aebf54f9f293f6198e1fc5c8b19f10f/pgjdbc/src/main/java/org/postgresql/jdbc/PgPreparedStatement.java#L991 |
Summary
The JDBC prepared statement parameter capture feature (opt-in via
CAPTURE_QUERY_PARAMETERS) previously only recorded parameters whose runtime type matched a hardcoded whitelist:Any value passed via
setObject()with a custom type — for example, an ORM-managed ID class — was silently dropped. The resulting span contained no attribute for that parameter position, which is indistinguishable from a parameter that was never set. This defeats the purpose of opting into parameter capture.This PR replaces the instanceof whitelist with a simple null check and calls
toString()on any non-null value, matching the same contract the JDBC driver itself must satisfy.Risk assessment
Low. Three considerations were weighed:
The feature is opt-in. Users who enable parameter capture have explicitly accepted that parameter values appear in telemetry. There is no new exposure for users who have not opted in.
The JDBC driver already requires serializability. Any object passed to
setObject()must be something the driver can handle — a type the driver cannot use would have thrown before reaching the instrumentation. If the driver accepts it, callingtoString()on it is safe.Worst case for unknown types is noise, not harm. For raw
byte[]or other types without a meaningfultoString(), the captured value will be something like[B@7b23ec81— useless but harmless, and still more informative than a missing attribute (it at least reveals the parameter's Java type). A completely missing parameter attribute is actively misleading when debugging a query.The one theoretical risk — a custom
toString()that materialises a large blob — exists, but requires an unusual implementation choice by the application and is bounded by the same constraints as any other span attribute.Test
Added
testCustomObjectPreparedStatementParametertoAbstractPreparedStatementParametersTest, which passes a customIdType(implementingSerializablefor H2 compatibility) viasetObject()and asserts that itstoString()value"id"is captured as the parameter attribute.Fixes the case reported in #7413 for ORM frameworks that pass custom ID types through
setObject().