opentracing-contrib / java-jdbc

OpenTracing Instrumentation for JDBC
Apache License 2.0
82 stars 56 forks source link

add jdbc url parser for adding for database information to the trace #30

Closed wuyupengwoaini closed 5 years ago

wuyupengwoaini commented 5 years ago

To fix https://github.com/opentracing-contrib/java-spring-cloud/issues/173 I think it is better to add the feature of paring the jdbc url here than to add the feature to the java-spring-cloud project.So I provided the ability to parse jdbc url Here.

But it also brings a problem that almost very databse url is different,so we need to provided more parser.Now i provided four parser (mysql ,oracle ,h2,postgresql)

wuyupengwoaini commented 5 years ago

@pavolloffay @malafeev please help me to review it

malafeev commented 5 years ago

tests failed

wuyupengwoaini commented 5 years ago

tests failed

fixed

malafeev commented 5 years ago

if we

or

how stable is instrumentation? I'm afraid that we can break user's application or spans will contain less information than now.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 105


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/main/java/io/opentracing/contrib/jdbc/ConnectionInfo.java 19 20 95.0%
src/main/java/io/opentracing/contrib/jdbc/parser/H2URLParser.java 32 33 96.97%
src/main/java/io/opentracing/contrib/jdbc/parser/OracleURLParser.java 56 57 98.25%
src/main/java/io/opentracing/contrib/jdbc/parser/URLParser.java 18 27 66.67%
<!-- Total: 201 213 94.37% -->
Files with Coverage Reduction New Missed Lines %
src/main/java/io/opentracing/contrib/jdbc/JdbcTracingUtils.java 1 91.18%
src/main/java/io/opentracing/contrib/jdbc/ConnectionInfo.java 2 91.43%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 101: 16.7%
Covered Lines: 349
Relevant Lines: 821

💛 - Coveralls
wuyupengwoaini commented 5 years ago

Now, if we don't have a parser for some database,URLParser wil return a ConnectionInfo.UNKNOWN_CONNECTION_INFO

and I provided a method to the user to register custom parser.

This instrumentation method is Borrowed from skywalking,so I think it is stable enough

wuyupengwoaini commented 5 years ago

The safe way of not breaking user's application is catching the exception of parsing url and return a ConnectionInfo.UNKNOWN_CONNECTION_INFO

what about you @malafeev

malafeev commented 5 years ago

I agree, that's the more safe way.

wuyupengwoaini commented 5 years ago

I catch all exception but I am not sure to add log or do nothing. And I remove the restrict of adding same parser to let user custome there own parser

@malafeev

malafeev commented 5 years ago

thanks @wuyupengwoaini, I think we need to add log message using java.util.logging, at least for debugging