Make JDBC tracer configurable through Spring properties #177
Conversation
Add properties for "withActiveSpanOnly" and "ignoreStatement" so the behaviour of the JDBC tracer can be controlled through application.yaml
Fix indentation and licences headers on new test classes.
| @@ -11,7 +11,7 @@ | |||
| * or implied. See the License for the specific language governing permissions and limitations under | |||
| * the License. | |||
| */ | |||
| package io.opemtracing.contrib.spring.cloud.jdbc; | |||
| package io.opentracing.contrib.spring.cloud.jdbc; | |||
There was a problem hiding this comment.
👍
| package io.opentracing.contrib.spring.cloud.jdbc; | |
| package io.opentracing.contrib.spring.cloud.jdbc; |
| @SpringBootTest(classes = {MockTracingConfiguration.class}) | ||
| @RunWith(SpringJUnit4ClassRunner.class) | ||
| @TestPropertySource(properties = { | ||
| "opentracing.spring.cloud.jdbc.withActiveSpanOnly=true" |
There was a problem hiding this comment.
Could we have the same test with the default configuration?
There was a problem hiding this comment.
So I copied this test and removed setting the property to let it be the default. The test itself passed however when I ran all the tests together they failed with this error:
Failed tests: spanJoinsActiveSpan(io.opentracing.contrib.spring.cloud.jdbc.JdbcTracingTest): expected:<1> but was:<0>
spanIsCreatedWhenUsingJdbcTemplate(io.opentracing.contrib.spring.cloud.jdbc.JdbcTracingTest): expected:<1> but was:<0>
spanIsCreatedForPreparedStatement(io.opentracing.contrib.spring.cloud.jdbc.JdbcTracingTest): expected:<1> but was:<0>
Tests in error:
concurrentParents(io.opentracing.contrib.spring.cloud.jdbc.JdbcTracingTest): Condition with lambda expression in io.opentracing.contrib.spring.cloud.jdbc.JdbcTracingTest was not fulfilled within 10 seconds.
I went through a bunch of iterations to see what the exact issue is as you can see through all the commits and reverts. The only thing I can pin point is that having a third test that sets properties different than this one, the test in the JdbcTracingTest fails with said error. I realized that this exact test is in JdbcTracingTest (https://github.com/opentracing-contrib/java-spring-cloud/blob/master/instrument-starters/opentracing-spring-cloud-jdbc-starter/src/test/java/io/opemtracing/contrib/spring/cloud/jdbc/JdbcTracingTest.java#L73:L81) with the default values so I believe that test covers this case. I will leave it that because I cannot get the tests to pass with adding the defaults test.
| @@ -29,6 +30,14 @@ | |||
| @Aspect | |||
| public class JdbcAspect { | |||
|
|
|||
| private final boolean withActiveSpanOnly; | |||
There was a problem hiding this comment.
withactivespanOnly should be documented. It wasn't clear to me what it means
There was a problem hiding this comment.
Ok. I documented it https://github.com/opentracing-contrib/java-spring-cloud/pull/177/files#diff-10f92decf61b800616faae597226a6e4R12 and https://github.com/opentracing-contrib/java-spring-cloud/pull/177/files#diff-9cee7f5e8a0570301726d076a1744f86R24 however I am not completely sure that is right or explains it correctly. It's already a field for the TracingConnection object. I looked there for some documentation but didn't find much besides https://github.com/opentracing-contrib/java-jdbc/blob/master/README.md#usage
There was a problem hiding this comment.
I documented it in the Properties class instead of here in the Aspect class because that was the trend I saw in other parts of this repo
1 similar comment
There was a problem hiding this comment.
thanks @lainekendall and sorry for the delay. I will cut a new version today!
…b#177) * Make JDBC tracer configurable through Spring properties Add properties for "withActiveSpanOnly" and "ignoreStatement" so the behaviour of the JDBC tracer can be controlled through application.yaml * Formatting fixes Fix indentation and licences headers on new test classes. * Fix pom ordering * Rename package, fix tests * Add default test, document properties * Revert "Add default test, document properties" This reverts commit bdbf55a. * Add Default test * Add documentation for new properties * Revert "Add documentation for new properties" This reverts commit 5274ddb. * Revert "Add Default test" This reverts commit 1a72821. * Revert "Revert "Add documentation for new properties"" This reverts commit 47353f2. * Add identical test * Empty out test * Add empty test * Delete defaults test
* Make JDBC tracing configurable through properties (#177) * Make JDBC tracer configurable through Spring properties Add properties for "withActiveSpanOnly" and "ignoreStatement" so the behaviour of the JDBC tracer can be controlled through application.yaml * Formatting fixes Fix indentation and licences headers on new test classes. * Fix pom ordering * Rename package, fix tests * Add default test, document properties * Revert "Add default test, document properties" This reverts commit bdbf55a. * Add Default test * Add documentation for new properties * Revert "Add documentation for new properties" This reverts commit 5274ddb. * Revert "Add Default test" This reverts commit 1a72821. * Revert "Revert "Add documentation for new properties"" This reverts commit 47353f2. * Add identical test * Empty out test * Add empty test * Delete defaults test * Rebased #176 (#182) * [issue-69] Redis instrumentation * [issue-69] PR changes * [issue-69] Fix dependecies * Use redis 0.0.7 Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Use -redis-spring-data Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Assert redis command on operation name Signed-off-by: Pavol Loffay <ploffay@redhat.com>
This is a rebased version of #144 by @will-gtv