Match specific direct JDBC driver implementations #2247
Conversation
|
It appears that the DB2 types referred to here are interfaces so will need handling similarly to the Oracle types |
|
Looked at the https://github.com/pgjdbc/pgjdbc repo. It gets tricky. They changed packages/classnames a few times and since we're doing exact name matching, we have to also match on Some that are missing: 6.4 6.5: 7.3 8.0 8.2 latest This list gets much shorter if we use |
richardstartin
force-pushed
the
rgs/jdbc-opt-in
branch
2 times, most recently
from
5fdffc5
to
fac2daa
Compare
|
@randomanderson thanks for digging into that, I was aware of a couple of the jdbc1/jdbc2 names but had left them out. I've added what should be complete coverage for Oracle, DB2, JT400, Postgres, and SQLite. I'll look in to the histories of the other drivers.
|
richardstartin
force-pushed
the
rgs/jdbc-opt-in
branch
from
fac2daa
to
4b8af4d
Compare
|
There's also no harm in having some redundancy in the list because it will short circuit matching, it's just important not to include any wrappers. |
richardstartin
force-pushed
the
rgs/jdbc-opt-in
branch
5 times, most recently
from
877c8fb
to
4adba40
Compare
...ntation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/ConnectionInstrumentation.java
Outdated
Show resolved
Hide resolved
| @@ -41,7 +43,52 @@ public ConnectionInstrumentation() { | |||
|
|
|||
| @Override | |||
| public ElementMatcher<TypeDescription> typeMatcher() { | |||
| return implementsInterface(named("java.sql.Connection")); | |||
There was a problem hiding this comment.
Should we consider a temporary config to toggle back to the original matcher?
There was a problem hiding this comment.
I don't think that's possible because the motivation for this PR is to remove checks on the critical path which make this matcher possible to use. The strategy proposed was to make a best effort attempt to find all relevant native connection/PS classes, extend the list over time, and add the flags to allow setting custom implementation classes.
There was a problem hiding this comment.
Yeah, I understand the motivation for this PR. If we postpone the follow on work for a future release, we could add the config I suggested and potentially have a safer fallback. I'm fine moving forward with your proposal though.
|
@randomanderson I want to rebase #2216 on to this, and then remove the unwrapping there. WDYT? |
richardstartin
force-pushed
the
rgs/jdbc-opt-in
branch
from
4b9d9df
to
9669524
Compare
richardstartin
force-pushed
the
rgs/jdbc-opt-in
branch
from
edfddd2
to
dfbc66c
Compare
This is here for review of the lists I have made of connection and preparedstatement implementations, in case any omissions jump out.
The motivation is twofold, both because JDBC involves lots of wrapping and we only want to create a single span:
ClassValue<Boolean>of bad unwrapping candidates, and replacing the implementations ofunwrapin some datanucleus wrappers. Targeting known non-delegating implementations makes this problem go away, so long as we can create a comprehensive enough list of implementation classes.ConnectiontoDBInfoassociation, which interferes with datanucleus, but would be safe if we never matched any datanucleus classes. The much bigger gain following from this is not doing any depth tracking, unwrapping, or unwrap safety checking (see unwrap prepared statements once on preparation #2216).The awkward odd one out from the big databases is Oracle, because of the licensing of its driver. I don't believe it is within the terms of the license to use the names of its implementation classes, so we need to check whether the classes implement the Oracle JDBC API. I haven't looked at the class names, so don't know if the prefix check is correct (this needs testing), and think it would be safer to just check if the interfaces are implemented, and perhaps add an oracle integration feature which can be disabled to speed up matching for users of other databases.