[issue-69] Redis instrumentation #176

ddcprg · 2018-09-20T10:32:50Z

Fixes #69

Please feed back on this approach

pavolloffay

Did you have a look at https://github.com/opentracing-contrib/java-redis-client?

@malafeev could it be consumed and used here?

pavolloffay · 2018-09-20T11:03:51Z

instrument-starters/opentracing-spring-cloud-redis-starter/pom.xml

+    </dependency>
+    <dependency>
+      <groupId>org.springframework.boot</groupId>
+      <artifactId>spring-boot-starter-data-redis</artifactId>


it has to be optional

I've made it optional, I'll push the change in a minute

RedisAutoConfiguration has to be changed to kick in only when redis classes are on classpath. Please also add test to NoDepsTest.java

thanks, I'll have look

pavolloffay · 2018-09-20T11:06:11Z

...starter/src/main/java/io/opentracing/contrib/spring/cloud/redis/connection/RedisCommand.java

+ *
+ * @author Daniel del Castillo
+ */
+final class RedisCommand {


could be this names consumed from redis lib? we could also use class names

do you mean from org.springframework.data.redis.core.RedisCommand? I couldn't find all the operations in this enum, e.g. cluster operations are not defined there. I didn't look in specific client libraries.

ddcprg · 2018-09-20T11:18:36Z

@pavolloffay I couldn't find an easy way to make use of java-redis-client in the Spring classes. I'll wait for @malafeev 's advice on this

malafeev · 2018-09-20T11:53:05Z

java-redis-client is more low level api for jedis and lettuce clients.
I don't think it can be used here because here is abstraction over any client. And that's good.

But maybe we can move this instrumentation into java-redis-client as module?
And use it here? It would be useful for spring projects without cloud.
opentracing-contrib/java-redis-client#8

ddcprg · 2018-09-20T12:09:31Z

Indeed these could be useful for a regular spring project. I'll refactor the code and open a PR against the other project

ddcprg · 2018-09-20T14:01:00Z

Raised opentracing-contrib/java-redis-client#10

malafeev · 2018-09-20T14:42:11Z

java-redis-client version 0.0.7 with spring support is released.
It will be available in maven central during 24 hours.

ddcprg · 2018-09-20T14:45:35Z

thanks @malafeev

ddcprg · 2018-10-08T11:58:53Z

hi @pavolloffay any thoughts on this PR?

pavolloffay · 2018-10-08T13:05:49Z

instrument-starters/opentracing-spring-cloud-redis-starter/pom.xml

+
+  <properties>
+    <main.basedir>${project.basedir}/../..</main.basedir>
+    <opentracing.redis.spring.version>0.0.7</opentracing.redis.spring.version>


move to root readme with name pattern used in this project

I've moved it to parent POM using the naming convention

pavolloffay · 2018-10-08T13:08:45Z

...acing-spring-cloud-starter/src/test/java/io/opentracing/contrib/spring/cloud/NoDepsTest.java

@@ -50,6 +50,11 @@ public void testNoRxJavaHooks() throws ClassNotFoundException {
    this.getClass().getClassLoader().loadClass("rx.plugins.RxJavaHooks");
  }

+  @Test(expected = ClassNotFoundException.class)
+  public void testNoRedisJavaHooks() throws ClassNotFoundException {
+    this.getClass().getClassLoader().loadClass("org.springframework.data.redis.core.RedisTemplate");


Could you please also add a check on some core redis class?

I'm not sure how you mean. I could only depend on some java client class but that would restrict the integrations allowed with spring-data. IMO if someone depends on spring-data-redis is because they use Redis, that together with the autoconfig options should be sufficient to activate the aspect(?)

Test if redis classes are not on classpath like: this.getClass().getClassLoader().loadClass("redis.clients.jedis.Client");

If you want to go furthher with tests look at https://github.com/opentracing-contrib/java-spring-cloud/blob/master/opentracing-spring-cloud-starter/pom.xml#L138 profile. This will require adding a dependency on redis spring starter and adding an integration test here https://github.com/opentracing-contrib/java-spring-cloud/tree/master/opentracing-spring-cloud-starter/src/test/java/io/opentracing/contrib/spring/cloud

pavolloffay · 2018-10-08T13:09:58Z

instrument-starters/opentracing-spring-cloud-redis-starter/pom.xml

+    <dependency>
+      <groupId>io.opentracing.contrib</groupId>
+      <artifactId>opentracing-redis-spring</artifactId>
+      <version>${opentracing.redis.spring.version}</version>


This should probably exclude all transitive dependencies. See my comment about adding a dependency test on core redis class.

I've made this optional

pavolloffay

It looks fine, but dependencies have to be corrected. Instrumentation jar cannot be optional

pavolloffay · 2018-10-10T08:25:54Z

instrument-starters/opentracing-spring-cloud-redis-starter/pom.xml

+      <groupId>io.opentracing.contrib</groupId>
+      <artifactId>opentracing-redis-spring</artifactId>
+      <version>${version.io.opentracing.contrib-opentracing-spring-redis}</version>
+      <optional>true</optional>


This is not right. We want to exclude all 3rd party deps (including redis). So only instrumentation classes are included. The redis classes will be pulled when user adds a dependency on spring-boot-starter-data-redis.

https://github.com/opentracing-contrib/java-spring-cloud/blob/master/instrument-starters/opentracing-spring-cloud-jdbc-starter/pom.xml#L40

Thanks, fixed

pavolloffay · 2018-10-10T08:27:32Z

...acing-spring-cloud-starter/src/test/java/io/opentracing/contrib/spring/cloud/NoDepsTest.java

@@ -50,6 +50,11 @@ public void testNoRxJavaHooks() throws ClassNotFoundException {
    this.getClass().getClassLoader().loadClass("rx.plugins.RxJavaHooks");
  }

+  @Test(expected = ClassNotFoundException.class)
+  public void testNoRedisJavaHooks() throws ClassNotFoundException {
+    this.getClass().getClassLoader().loadClass("org.springframework.data.redis.core.RedisTemplate");


Test if redis classes are not on classpath like: this.getClass().getClassLoader().loadClass("redis.clients.jedis.Client");

pavolloffay · 2018-10-10T08:29:40Z

...acing-spring-cloud-starter/src/test/java/io/opentracing/contrib/spring/cloud/NoDepsTest.java

@@ -50,6 +50,11 @@ public void testNoRxJavaHooks() throws ClassNotFoundException {
    this.getClass().getClassLoader().loadClass("rx.plugins.RxJavaHooks");
  }

+  @Test(expected = ClassNotFoundException.class)
+  public void testNoRedisJavaHooks() throws ClassNotFoundException {
+    this.getClass().getClassLoader().loadClass("org.springframework.data.redis.core.RedisTemplate");


If you want to go furthher with tests look at https://github.com/opentracing-contrib/java-spring-cloud/blob/master/opentracing-spring-cloud-starter/pom.xml#L138 profile. This will require adding a dependency on redis spring starter and adding an integration test here https://github.com/opentracing-contrib/java-spring-cloud/tree/master/opentracing-spring-cloud-starter/src/test/java/io/opentracing/contrib/spring/cloud

ddcprg · 2018-10-17T16:33:37Z

Sorry, I'm snowed under ATM... I'll try to take a look at this tonight

ddcprg · 2018-10-22T21:38:19Z

@pavolloffay with regards adding more testing under the alldeps profile, this will require setting up an embedded Redis to be able to fetch a connection. I'm not sure whether is worth doing all this in this test but let me know and I can add it

* [issue-69] Redis instrumentation * [issue-69] PR changes * [issue-69] Fix dependecies

pavolloffay · 2018-10-31T10:42:48Z

Merged in #172. Thanks @ddcprg. I wanted to rush and do release with this feature. Thanks again.

ddcprg · 2018-10-31T10:53:04Z

cool, thanks @pavolloffay

* [issue-69] Redis instrumentation * [issue-69] PR changes * [issue-69] Fix dependecies

* 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>

pavolloffay reviewed Sep 20, 2018

View reviewed changes

[issue-69] Redis instrumentation

4321434

ddcprg force-pushed the issue-69 branch from 075c41f to 4321434 Compare September 20, 2018 15:35

pavolloffay reviewed Oct 8, 2018

View reviewed changes

[issue-69] PR changes

156b55b

pavolloffay reviewed Oct 10, 2018

View reviewed changes

[issue-69] Fix dependecies

cb902f5

pavolloffay mentioned this pull request Oct 31, 2018

Rebased #176 #182

Merged

pavolloffay added a commit that referenced this pull request Oct 31, 2018

Rebased #176 (#182)

b361c8a

* [issue-69] Redis instrumentation * [issue-69] PR changes * [issue-69] Fix dependecies

pavolloffay closed this Oct 31, 2018

ddcprg deleted the issue-69 branch October 31, 2018 10:53

pavolloffay added a commit to pavolloffay/java-spring-cloud that referenced this pull request Jan 17, 2019

Rebased opentracing-contrib#176 (opentracing-contrib#182)

a185cb4

* [issue-69] Redis instrumentation * [issue-69] PR changes * [issue-69] Fix dependecies

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[issue-69] Redis instrumentation #176

[issue-69] Redis instrumentation #176

ddcprg commented Sep 20, 2018

pavolloffay left a comment

pavolloffay Sep 20, 2018

ddcprg Sep 20, 2018

pavolloffay Sep 20, 2018

ddcprg Sep 20, 2018 •

edited

pavolloffay Sep 20, 2018

ddcprg Sep 20, 2018

ddcprg commented Sep 20, 2018

malafeev commented Sep 20, 2018 •

edited

ddcprg commented Sep 20, 2018

ddcprg commented Sep 20, 2018

malafeev commented Sep 20, 2018

ddcprg commented Sep 20, 2018

ddcprg commented Oct 8, 2018

pavolloffay Oct 8, 2018

ddcprg Oct 9, 2018

pavolloffay Oct 8, 2018

ddcprg Oct 9, 2018

pavolloffay Oct 10, 2018

pavolloffay Oct 10, 2018

pavolloffay Oct 8, 2018

ddcprg Oct 9, 2018

pavolloffay left a comment

pavolloffay Oct 10, 2018

ddcprg Oct 22, 2018

pavolloffay Oct 10, 2018

pavolloffay Oct 10, 2018

ddcprg commented Oct 17, 2018

ddcprg commented Oct 22, 2018

pavolloffay commented Oct 31, 2018

ddcprg commented Oct 31, 2018

[issue-69] Redis instrumentation #176

[issue-69] Redis instrumentation #176

Conversation

ddcprg commented Sep 20, 2018

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcprg Sep 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcprg commented Sep 20, 2018

malafeev commented Sep 20, 2018 • edited

ddcprg commented Sep 20, 2018

ddcprg commented Sep 20, 2018

malafeev commented Sep 20, 2018

ddcprg commented Sep 20, 2018

ddcprg commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcprg commented Oct 17, 2018

ddcprg commented Oct 22, 2018

pavolloffay commented Oct 31, 2018

ddcprg commented Oct 31, 2018

ddcprg Sep 20, 2018 •

edited

malafeev commented Sep 20, 2018 •

edited