Add lettuce 5.2 instrumentation that uses lettuce's native tracing functionality. #535

anuraaga · 2020-06-16T06:54:34Z

If muzzle lets me through in one shot I'd be amazed :)

While Tracing was added in 5.1, including the command was added in 5.2, and it doesn't seeem worth branching on this since this is new instrumentation, I just went ahead and targeted 5.2.

Differences in tracing

connection and some system commands aren't traced.
net / command tags are filled in. A couple of events that lettuce provides are too
No corner cases, while I preserved the async/reactive tests, there isn't as much value since there is no special-casing for the APIs

Fixes #408
Fixes #504

Co-authored-by: Dustin Neray <[email protected]>

instrumentation/lettuce/lettuce-5.2/lettuce-5.2.gradle

...rc/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceClientDecorator.java

.../src/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/OpenTelemetryTracing.java

iNikem · 2020-06-16T10:17:11Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+    when:
+    ConnectionFuture connectionFuture = testConnectionClient.connectAsync(StringCodec.UTF8,
+      new RedisURI(HOST, port, 3, TimeUnit.SECONDS))
+    StatefulConnection connection = connectionFuture.get()


This can block indefinitely, not good

Don't tests have a test timeout?

iNikem · 2020-06-16T10:17:49Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+    when:
+    ConnectionFuture connectionFuture = testConnectionClient.connectAsync(StringCodec.UTF8,
+      new RedisURI(HOST, incorrectPort, 3, TimeUnit.SECONDS))
+    StatefulConnection connection = connectionFuture.get()


iNikem · 2020-06-16T10:21:28Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+            "net.peer.ip" "127.0.0.1"
+            "net.peer.port" port
+            "db.type" "redis"
+            "db.statement" "key<TESTSETKEY> value<TESTSETVAL>"


Can you fill more attributes? https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md

Unfortunately the interface doesn't provide RedisURI, I have filed an issue for it

redis/lettuce#1312

I think user and db_index are less commonly used with redis and we can probably live without them for now.

trask

thanks @anuraaga, this looks great!

for the muzzle issues, try locally:

./gradlew :instrumentation:lettuce:lettuce-5.0:muzzle
./gradlew :instrumentation:lettuce:lettuce-5.2:muzzle

and post back with any questions, i'll answer and add those questions/answers to the muzzle faq i'm building for #536

.../src/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/OpenTelemetryTracing.java

instrumentation/lettuce/lettuce-5.0/lettuce-5.0.gradle

...rc/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceClientDecorator.java

.../src/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/OpenTelemetryTracing.java

trask · 2020-06-16T22:07:03Z

.../src/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/OpenTelemetryTracing.java

+    @Nullable private final Span parent;
+
+    @Nullable private OpenTelemetryEndpoint endpoint;
+    @Nullable private String name;
+
+    @Nullable private List<Object> events;
+    @Nullable private List<String> tags;


what do u think about replacing parent, endpoint, name and tags with Span.Builder?

Unfortunately had to keep name since spanBuilder doesn't allow setting it, and some awkwardness for endpoint, but it seems to still be worth it.

trask · 2020-06-16T22:21:01Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+          event(0) {
+            eventName "redis.encode.start"
+          }
+          event(1) {
+            eventName "redis.encode.end"
+          }


are these events worth capturing, given they are not part of the semantic conventions?

I tend to lean towards instrumenting what we can, and users can use a downstream mechanism, e.g. span processor, to remove fields they don't need. Might help someone and while data can be filtered, there's no way to work around the lack of instrumentation.

anuraaga

Thanks! I realized that this instrumentation can be applied to 5.1 just fine, the new method on the SPI will just be ignored, so I reduced the version floor. It also made it simpler to restrict the instrumentation for 5.0 and 5.1.

instrumentation/lettuce/lettuce-5.2/lettuce-5.2.gradle

.../src/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/OpenTelemetryTracing.java

anuraaga · 2020-06-17T05:13:23Z

.../src/main/java8/io/opentelemetry/auto/instrumentation/lettuce/v5_2/OpenTelemetryTracing.java

+    @Nullable private final Span parent;
+
+    @Nullable private OpenTelemetryEndpoint endpoint;
+    @Nullable private String name;
+
+    @Nullable private List<Object> events;
+    @Nullable private List<String> tags;


Unfortunately had to keep name since spanBuilder doesn't allow setting it, and some awkwardness for endpoint, but it seems to still be worth it.

anuraaga · 2020-06-17T05:14:36Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+    when:
+    ConnectionFuture connectionFuture = testConnectionClient.connectAsync(StringCodec.UTF8,
+      new RedisURI(HOST, port, 3, TimeUnit.SECONDS))
+    StatefulConnection connection = connectionFuture.get()


Don't tests have a test timeout?

anuraaga · 2020-06-17T05:33:30Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+          event(0) {
+            eventName "redis.encode.start"
+          }
+          event(1) {
+            eventName "redis.encode.end"
+          }


I tend to lean towards instrumenting what we can, and users can use a downstream mechanism, e.g. span processor, to remove fields they don't need. Might help someone and while data can be filtered, there's no way to work around the lack of instrumentation.

anuraaga · 2020-06-17T05:40:48Z

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_2/LettuceAsyncClientTest.groovy

+            "net.peer.ip" "127.0.0.1"
+            "net.peer.port" port
+            "db.type" "redis"
+            "db.statement" "key<TESTSETKEY> value<TESTSETVAL>"


Unfortunately the interface doesn't provide RedisURI, I have filed an issue for it

redis/lettuce#1312

I think user and db_index are less commonly used with redis and we can probably live without them for now.

iNikem · 2020-06-17T06:39:32Z

instrumentation/lettuce/lettuce-5.1/lettuce-5.1.gradle

+}
+
+dependencies {
+  compileOnly group: 'io.lettuce', name: 'lettuce-core', version: '5.2.0.RELEASE'


Shouldn't this be 5.1.0.RELEASE?

Yeah guess so

...t/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_1/LettuceReactiveClientTest.groovy

...test/groovy/io/opentelemetry/auto/instrumentation/lettuce/v5_1/LettuceAsyncClientTest.groovy

anuraaga · 2020-06-17T08:04:44Z

Codenarc seems to have crossed a threshold due to the package name having an underscore. But I notice this pattern throughout the repo - can we disable this check?


Rule Name | Priority | Line # | Source Line / Message
-- | -- | -- | --
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1
PackageName | 2 |   | [MSG]package=io.opentelemetry.auto.instrumentation.lettuce.v5_1


  | PackageName | Verifies  that the package name for a class matches a regular expression. By  default it checks that the package name consists of only lowercase  letters, separated by periods. The regex property specifies the regular expression used to validate the package name. The packageNameRequired property indicates whether a package name declaration is required for all classes.
-- | -- | --

trask · 2020-06-17T21:49:23Z

nice muzzle work! 😄

Codenarc seems to have crossed a threshold due to the package name having an underscore. But I notice this pattern throughout the repo - can we disable this check?

yes, please do

anuraaga · 2020-06-18T07:36:33Z

Looks ready now :)

…nctionality. (open-telemetry#535) * Copy Lettuce 5.0 to start 5.1 instrumentation * Begin tracing adapter implementation Co-authored-by: Dustin Neray <[email protected]> * Set floor to 5.2 instead * Move around * Finish * Cleanups * Instrument 5.1+ instead * Cleanup * 5.1 * Remove latestDepTest from lettuce-5.0 since we have a newer lettuce-5.1. * Remove * Remove package check * Spotless Co-authored-by: Dustin Neray <[email protected]>

Anuraag Agrawal and others added 5 commits June 11, 2020 16:56

Copy Lettuce 5.0 to start 5.1 instrumentation

4ed7438

Begin tracing adapter implementation

51ae574

Co-authored-by: Dustin Neray <[email protected]>

Set floor to 5.2 instead

b73576b

Move around

80fb2c5

Finish

87e8547

anuraaga requested review from iNikem, jkwatson, trask and tylerbenson as code owners June 16, 2020 06:54

iNikem suggested changes Jun 16, 2020

View reviewed changes

trask reviewed Jun 16, 2020

View reviewed changes

Anuraag Agrawal added 2 commits June 17, 2020 14:50

Cleanups

7cdf7ec

Instrument 5.1+ instead

36b141e

anuraaga commented Jun 17, 2020

View reviewed changes

iNikem reviewed Jun 17, 2020

View reviewed changes

Anuraag Agrawal added 2 commits June 17, 2020 16:24

Cleanup

5fab23d

5.1

d8ee86f

Anuraag Agrawal added 2 commits June 17, 2020 17:06

Remove latestDepTest from lettuce-5.0 since we have a newer lettuce-5.1.

e516d1a

Remove

09a2b49

Anuraag Agrawal added 2 commits June 18, 2020 14:26

Remove package check

3faa4de

Spotless

8b5e8c7

Merge branch 'master' into lettuce-51

8595dd8

iNikem approved these changes Jun 18, 2020

View reviewed changes

iNikem merged commit 9a2a0b5 into open-telemetry:master Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lettuce 5.2 instrumentation that uses lettuce's native tracing functionality. #535

Add lettuce 5.2 instrumentation that uses lettuce's native tracing functionality. #535

anuraaga commented Jun 16, 2020 •

edited

Loading

iNikem Jun 16, 2020

anuraaga Jun 17, 2020

iNikem Jun 16, 2020

iNikem Jun 16, 2020

anuraaga Jun 17, 2020

trask left a comment

trask Jun 16, 2020

anuraaga Jun 17, 2020

trask Jun 16, 2020

anuraaga Jun 17, 2020

anuraaga left a comment

anuraaga Jun 17, 2020

anuraaga Jun 17, 2020

anuraaga Jun 17, 2020

anuraaga Jun 17, 2020

iNikem Jun 17, 2020

anuraaga Jun 17, 2020

anuraaga commented Jun 17, 2020

trask commented Jun 17, 2020

anuraaga commented Jun 18, 2020 •

edited

Loading

Add lettuce 5.2 instrumentation that uses lettuce's native tracing functionality. #535

Add lettuce 5.2 instrumentation that uses lettuce's native tracing functionality. #535

Conversation

anuraaga commented Jun 16, 2020 • edited Loading

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

trask 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

anuraaga 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Jun 17, 2020

trask commented Jun 17, 2020

anuraaga commented Jun 18, 2020 • edited Loading

anuraaga commented Jun 16, 2020 •

edited

Loading

anuraaga commented Jun 18, 2020 •

edited

Loading