Attempt to properly parse out instance name from JDBC url#866
Attempt to properly parse out instance name from JDBC url#866tylerbenson merged 7 commits intomasterfrom
Conversation
Unfortunately implementations are INCREDIBLY inconsistent on the matter. Oracle implementation is still pending since it’s really complicated.
to avoid changing existing service names
tylerbenson
force-pushed
the
tyler/jdbc-instance
branch
from
536b7ad to
1a5a706
Compare
labbati
left a comment
labbati
left a comment
There was a problem hiding this comment.
@tylerbenson awesome work. I included a first round of CR and ideas. I would love to hear your thoughts about them
...rumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/DriverInstrumentation.java
Outdated
Show resolved
Hide resolved
...rumentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/DriverInstrumentation.java
Outdated
Show resolved
Hide resolved
| import java.util.regex.Pattern; | ||
|
|
||
| /** | ||
| * Structured as an enum instead of a class hierarchy to allow iterating through the parsers |
There was a problem hiding this comment.
The number of cases that we covered with this code is impressive. Very great job.
Let me add a comment here that applies to the approach in general, not to any specific parser.
From the functional standpoint I have nothing against it. I see your point (you are not required to remember adding a class/instance to a list) but I honestly think that this approach decreases readability to a certain degree. Now, I don't expect the number of DBs to explode over time, but we are already to the point where GH collapse 'large files' by default 😄.
So in the end I would probably fallback to defining an interface JDBCInfoParser or the liked and having N concrete parsers implementing it.
There was a problem hiding this comment.
I think I'd prefer to leave it as is if that's ok...
There was a problem hiding this comment.
Yeah no problem with that. Worst case we can extract at a later time if we decide it is growing too much.
...mentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/JDBCConnectionUrlParser.java
Outdated
Show resolved
Hide resolved
| final int typeLoc = jdbcUrl.indexOf(':'); | ||
|
|
||
| if (typeLoc < 1) { | ||
| // Invalid format: `jdbc:` or `jdbc::` |
There was a problem hiding this comment.
Did not know that jdbc:: was a valid format. BTW in this case is not tested and I don't think that code around can handle it, because indexOf returns the first occurence and below you are doing jdbcUrl.substring(0, typeLoc);
There was a problem hiding this comment.
I don't think jdbc: or jdbc:: is valid. I added a test to verify behavior.
|
|
||
| final String baseType = jdbcUrl.substring(0, typeLoc); | ||
|
|
||
| final DBInfo.Builder parsedProps = DEFAULT.toBuilder().type(baseType); |
There was a problem hiding this comment.
How do you see moving the part from final int typeLoc = jdbcUrl.indexOf(':'); to here into a separate method parseDatabaseType()?
There was a problem hiding this comment.
I tried to avoid extracting too many methods for various things that are either very context sensitive or only used once. Is this important to you?
There was a problem hiding this comment.
Nevermind, is just that in this part I was having hard times figuring 'what' we were doing without going into 'how' we were doing it.
.../jdbc/src/main/java/datadog/trace/instrumentation/jdbc/PreparedStatementInstrumentation.java
Outdated
Show resolved
Hide resolved
...mentation/jdbc/src/main/java/datadog/trace/instrumentation/jdbc/JDBCConnectionUrlParser.java
Outdated
Show resolved
Hide resolved
labbati
left a comment
labbati
left a comment
There was a problem hiding this comment.
Thanks for your answers. I am fine with the changes and I really appreciated the extensive testing in this PR.
2 participants
Unfortunately implementations are INCREDIBLY inconsistent on the matter so parsing is pretty complicated.
Updates #606