Remove deregister then register again behavior

quaff commented 4 years ago

some driver like oracle ojdbc will be destroyed forever after deregister. see https://github.com/opentracing-contrib/java-jdbc/issues/73

malafeev commented 4 years ago

I suggest not try to deregister and register again because some drivers are one-off, leave it to the application to make sure TracingDriver is registered first since they must call setInterceptorMode(true) somewhere, for example before a servlet context initialized.

@Override public void contextInitialized(ServletContextEvent event) { io.opentracing.contrib.jdbc.TracingDriver.setInterceptorMode(true); } we could add an instruction in https://github.com/opentracing-contrib/java-jdbc#interceptor.

@quaff could you update README?

quaff commented 4 years ago

@malafeev README updated.

malafeev commented 4 years ago

@quaff tests failed: https://travis-ci.org/github/opentracing-contrib/java-jdbc/builds/699225132?utm_source=github_status&utm_medium=notification

quaff commented 4 years ago

@malafeev all tests passed in local but failed with CI.

Tests run: 50, Failures: 0, Errors: 0, Skipped: 0

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  4.014 s
[INFO] Finished at: 2020-06-17T16:40:25+08:00
[INFO] ------------------------------------------------------------------------

I can't find a way to ensure TracingDriver load before h2 driver in junit tests.

malafeev commented 4 years ago

I'm thinking about adding second boolean parameter to TracingDriver.setInterceptorMode(...) which should allow/disallow deregistering drivers. hm, now I'm not sure that it will work.

malafeev commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

quaff commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

It's a good idea, I will try.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 270

9 of 12 (75.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.2%) to 47.706%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/main/java/io/opentracing/contrib/jdbc/TracingDriver.java	9	12	75.0%
<!--	Total:	9	12	75.0%	-->

Files with Coverage Reduction	New Missed Lines	%
src/main/java/io/opentracing/contrib/jdbc/TracingDriver.java	2	80.43%
<!--	Total:	2		-->

Totals
Change from base Build 260:	-0.2%
Covered Lines:	468
Relevant Lines:	981

💛 - Coveralls

quaff commented 4 years ago

@malafeev Tests passed finally, please hold on, I will move that method from JdbcTest to TracingDriver.

malafeev commented 4 years ago

@quaff great, please also update README

quaff commented 4 years ago

@malafeev please review code, especially english words and grammar.

whiskeysierra commented 4 years ago

My 2c: I'd avoid boolean parameters in public APIs and rather go with two distinct methods with speaking names. Internally, it's fine though.

felipeazv commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

quaff commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

felipeazv commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

Nice, @quaff . For the

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

Thanks @quaff , So according to your PR: https://github.com/opentracing-contrib/java-jdbc/commit/d84907e0b45339eccb7fd6e9e1436962937bd614, by setting TracingDriver.ensureRegisteredAsTheFirstDriver() along with TracingDriver.setInterceptorMode(true) will provide the fix for drivers such as Oracle, right?

I asked because on your PR, the sentence on readme file: "Please note driver like Oracle JDBC may fail since it's destroyed forever after deregistration." gives the impression that this may not work with Oracle Driver.

quaff commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

Nice, @quaff . For the

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

Thanks @quaff , So according to your PR: d84907e, by setting TracingDriver.ensureRegisteredAsTheFirstDriver() along with TracingDriver.setInterceptorMode(true) will provide the fix for drivers such as Oracle, right?

I asked because on your PR, the sentence on readme file: "Please note driver like Oracle JDBC may fail since it's destroyed forever after deregistration." gives the impression that this may not work with Oracle Driver.

If TracingDriver is registered after Oracle JDBC driver, then the interceptor mode will not works but the driver works fine, if you call TracingDriver.ensureRegisteredAsTheFirstDriver() then the driver will throw exception.

felipeazv commented 4 years ago

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

Nice, @quaff . For the

I think we need a method in TracingDriver to allow user change registered drivers order so TracingDriver will be the first one and add javadoc that it will not work for some drivers like oracle.

Hello @malafeev Does that mean that the issue with Oracle driver will remain?

If TracingDriver is registered before oracle driver, everything is OK.

Thanks @quaff , So according to your PR: d84907e, by setting TracingDriver.ensureRegisteredAsTheFirstDriver() along with TracingDriver.setInterceptorMode(true) will provide the fix for drivers such as Oracle, right? I asked because on your PR, the sentence on readme file: "Please note driver like Oracle JDBC may fail since it's destroyed forever after deregistration." gives the impression that this may not work with Oracle Driver.

If TracingDriver is registered after Oracle JDBC driver, then the interceptor mode will not works but the driver works fine, if you call TracingDriver.ensureRegisteredAsTheFirstDriver() then the driver will throw exception.

But if the interceptor mode won't work with TracingDriver being registered after Oracle Driver, then it won't be possible to enable jdbc tracing with Oracle Driver, right?

malafeev commented 4 years ago

it there is no way to load TracingDriver before Oracle then solution is don't use interceptor mode but use non-iterceptor. Non-interceptor mode should work fine.

felipeazv commented 4 years ago

it there is no way to load TracingDriver before Oracle then solution is don't use interceptor mode but use non-iterceptor. Non-interceptor mode should work fine.

As non-interceptor you mean, having TracingDriver.setInterceptorMode(false) which is set by default?

malafeev commented 4 years ago

I mean don't call TracingDriver.setInterceptorMode(...) , instead use https://github.com/opentracing-contrib/java-jdbc#non-interceptor

felipeazv commented 4 years ago

I mean don't call TracingDriver.setInterceptorMode(...) , instead use https://github.com/opentracing-contrib/java-jdbc#non-interceptor

Thanks for the info, @malafeev . I'll wait for the release.

felipeazv commented 4 years ago

I've just tested and it seems to be working just fine. Thanks! 👍

opentracing-contrib / java-jdbc

Remove deregister then register again behavior #77

Pull Request Test Coverage Report for Build 270

💛 - Coveralls