Add [Agent] Jedis LETTUCE client plugin #832

lytscu · 2018-02-27T01:36:35Z

Please answer these questions before submitting pull request

Why submit this pull request?
New feature provided
Related issues
Jedis LETTUCE client plugin Jedis LETTUCE client plugin #766

Bug fix

Bug description.
How to fix?

New feature or improvement

Describe the details and related test reports.
https://github.com/SkywalkingTest/agent-integration-test-report/blob/master/2018/2/lytscu/testReport-master-2018-02-27-09-16.md

y

…into jedis y

coveralls · 2018-02-27T02:09:32Z

Coverage decreased (-0.003%) to 19.428% when pulling 49cd58f on lytscu:jedis into 9e9c08d on apache:master.

wu-sheng · 2018-02-27T03:06:41Z

@lytscu Please run your test again, after ci passed.

lytscu · 2018-02-27T06:07:22Z

https://github.com/SkywalkingTest/agent-integration-test-report/blob/master/2018/2/lytscu/testReport-master-2018-02-27-12-40.md

ascrutae · 2018-02-27T08:21:36Z

...g/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisClusterConnectionImplInterceptor.java

+            AbstractSpan span = ContextManager.activeSpan();
+            span.errorOccurred();
+        }
+        ContextManager.stopSpan();


It maybe cause exception if the commandType equals AUTH, because of I saw that StatefulRedisClusterConnectionImplInterceptor doesn't create span if the commandType equals AUTH

ascrutae · 2018-02-27T08:28:14Z

...java/org/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisConnectionImplInterceptor.java

+            AbstractSpan span = ContextManager.activeSpan();
+            span.errorOccurred();
+        }
+        ContextManager.stopSpan();


It maybe cause exception if the command type is ignore command type, because of I saw that StatefulRedisClusterConnectionImplInterceptor doesn't create span if the command type is ignore command type

Already updated

Cannot see any update

…into jedis y :

ascrutae · 2018-02-27T08:44:49Z

...java/org/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisConnectionImplInterceptor.java

+    /**
+     * Check the comandtype ,ignore  "AUTH"  "CLIENT" "CLUSTER" .
+     */
+    private Boolean checkIgnoreCommandType(String comandType) {


This method is make confuse. I suggest that the method name change to checkIfNotIgnoreCommandType

ascrutae · 2018-02-27T08:56:16Z

...g/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisClusterConnectionImplInterceptor.java

+        String comandType = String.valueOf(command.getType());
+        if (!"AUTH".equals(comandType)) {
+            Iterable<RedisURI> redisURIs = (Iterable<RedisURI>)objInst.getSkyWalkingDynamicField();
+            AbstractSpan span = ContextManager.createExitSpan("REDIS-Lettuce/" + comandType, redisURIs.toString());


I am not sure what the value about redisURIs.toString() look like. Can you show me an example?

redisURIs.toString()=[RedisURI [host='100.100.144.134', port=30001]]

This is not a typical peer. In traditional, the peer like ip1:port1,ip2:port2

ascrutae · 2018-02-27T09:01:13Z

...g/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisClusterConnectionImplInterceptor.java

+        RedisCommand command = (RedisCommand)allArguments[0];
+        String comandType = String.valueOf(command.getType());
+        if (null != command.getOutput().getError()) {
+            AbstractSpan span = ContextManager.activeSpan();


It maybe cause exception if the command type equals AUTH, because of I saw that StatefulRedisClusterConnectionImplInterceptor doesn't create span if the command type equals AUTH

ascrutae · 2018-02-28T02:45:27Z

.../org/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisConnectionImplInterceptorTest.java

+        MatcherAssert.assertThat(segmentStorage.getTraceSegments().size(), is(1));
+        TraceSegment traceSegment = segmentStorage.getTraceSegments().get(0);
+        List<AbstractTracingSpan> spans = SegmentHelper.getSpans(traceSegment);
+        assertMongoSpan(spans.get(0));


The method name should be assertLettuceSpan

ascrutae · 2018-02-28T02:46:19Z

.../org/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisConnectionImplInterceptorTest.java

+
+        interceptor = new StatefulRedisConnectionImplInterceptor();
+
+        Config.Plugin.MongoDB.TRACE_PARAM = true;


I think this line should be remove

ascrutae · 2018-02-28T02:53:30Z

...java/org/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisConnectionImplInterceptor.java

+
+    @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
+        Class<?>[] argumentsTypes, Throwable t) {
+        AbstractSpan span = ContextManager.activeSpan();


I think this should also check the command type

ascrutae · 2018-02-28T02:53:38Z

...g/apache/skywalking/apm/plugin/lettuce/v4/StatefulRedisClusterConnectionImplInterceptor.java

+
+    @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
+        Class<?>[] argumentsTypes, Throwable t) {
+        AbstractSpan span = ContextManager.activeSpan();


I think this should also check the command type

ascrutae · 2018-02-28T03:44:08Z

Could you post the latest test report? @lytscu

wu-sheng · 2018-02-28T03:45:29Z

Could you post the latest test report? @lytscu

+1 I agree we should run the test again before merge.

…into jedis y

lytscu · 2018-02-28T06:08:35Z

https://github.com/SkywalkingTest/agent-integration-test-report/blob/master/2018/2/lytscu/testReport-master-2018-02-28-11-50.md

wu-sheng · 2018-03-05T14:25:45Z

I move this pr to 5.0.0 beta, because our codes of 5.0.0 alpha is about to be frozen. @lytscu

wu-sheng · 2018-04-25T01:12:45Z

@ascrutae What is the status of this plugin? And @lytscu please resolve the conflicts.

ascrutae · 2018-04-26T07:32:50Z

The duration of ExitSpan created by StatefulRedisConnectionImplInterceptor is incorrect. because the StatefulRedisConnectionImpl#dispatch method only create an redis command and this command will execute in other thread.

wu-sheng · 2018-05-02T01:49:52Z

@lytscu Would you continue work on this pr?

wu-sheng · 2018-08-08T14:42:15Z

Closing this, no update more than 3 months.

lytscu added 2 commits February 26, 2018 16:52

Add [Agent] Jedis LETTUCE client plugin

975961f

Merge remote-tracking branch 'upstream/master' into jedis

ad93cf2

y

lytscu requested review from wu-sheng and ascrutae February 27, 2018 01:37

Merge branch 'master' into jedis

af7ca80

lytscu requested review from wu-sheng and ascrutae and removed request for wu-sheng and ascrutae February 27, 2018 01:40

lytscu added 3 commits February 27, 2018 09:46

Merge branch 'jedis' of https://github.com/lytscu/incubator-skywalking …

88f8c29

…into jedis y

Update pom.xml

6050e88

Update pom.xml 5.0.0-alpha-SNAPSHOT

0cbeb4c

Update comments (HttpClientInstrumentation.java)

f123435

wu-sheng and others added 3 commits February 27, 2018 14:17

Merge branch 'master' into jedis

79607a2

Merge branch 'master' into jedis

719cf46

Merge branch 'master' into jedis

f38c0d3

wu-sheng added this to the 5.0.0-alpha milestone Feb 27, 2018

wu-sheng added agent Language agent related. feature New feature labels Feb 27, 2018

ascrutae reviewed Feb 27, 2018

View reviewed changes

lytscu added 3 commits February 27, 2018 16:29

Update StatefulRedisClusterConnectionImplInterceptor.java

994b958

Merge branch 'master' into jedis

115f49a

Merge branch 'jedis' of https://github.com/lytscu/incubator-skywalking …

0ab6ea5

…into jedis y :

ascrutae reviewed Feb 27, 2018

View reviewed changes

ascrutae reviewed Feb 28, 2018

View reviewed changes

lytscu added 2 commits February 28, 2018 11:15

Update handleMethodException

b0b4bdc

Merge branch 'master' into jedis

6d3cd68

ascrutae previously approved these changes Feb 28, 2018

View reviewed changes

Merge branch 'master' into jedis

8f61418

lytscu added 2 commits February 28, 2018 11:46

Update StatefulRedisConnectionImplInterceptorTest

1596b61

Merge branch 'jedis' of https://github.com/lytscu/incubator-skywalking …

350ddc1

…into jedis y

lytscu dismissed ascrutae’s stale review via 350ddc1 February 28, 2018 03:49

ascrutae and others added 4 commits February 28, 2018 14:10

Merge branch 'master' into jedis

d411235

Merge branch 'master' into jedis

b4b1806

Merge branch 'master' into jedis

6dafc2e

Merge branch 'master' into jedis

49cd58f

wu-sheng modified the milestones: 5.0.0-alpha, 5.0.0-beta Mar 5, 2018

wu-sheng mentioned this pull request Mar 5, 2018

Jedis LETTUCE client plugin #766

Closed

4 tasks

wu-sheng modified the milestones: 5.0.0-beta, 5.0.0-beta2 May 3, 2018

wu-sheng closed this Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add [Agent] Jedis LETTUCE client plugin #832

Add [Agent] Jedis LETTUCE client plugin #832

lytscu commented Feb 27, 2018 •

edited

coveralls commented Feb 27, 2018 •

edited

wu-sheng commented Feb 27, 2018

lytscu commented Feb 27, 2018

ascrutae Feb 27, 2018

ascrutae Feb 27, 2018 •

edited

lytscu Feb 27, 2018 •

edited

ascrutae Feb 27, 2018

ascrutae Feb 27, 2018

lytscu Feb 27, 2018

ascrutae Feb 27, 2018

lytscu Feb 27, 2018

wu-sheng Feb 27, 2018

lytscu Feb 27, 2018

ascrutae Feb 27, 2018

lytscu Feb 27, 2018

ascrutae Feb 28, 2018

ascrutae Feb 28, 2018

ascrutae Feb 28, 2018

ascrutae Feb 28, 2018

ascrutae commented Feb 28, 2018

wu-sheng commented Feb 28, 2018

lytscu commented Feb 28, 2018

wu-sheng commented Mar 5, 2018

wu-sheng commented Apr 25, 2018

ascrutae commented Apr 26, 2018

wu-sheng commented May 2, 2018

wu-sheng commented Aug 8, 2018


		interceptor = new StatefulRedisConnectionImplInterceptor();

		Config.Plugin.MongoDB.TRACE_PARAM = true;

Add [Agent] Jedis LETTUCE client plugin #832

Add [Agent] Jedis LETTUCE client plugin #832

Conversation

lytscu commented Feb 27, 2018 • edited

Bug fix

New feature or improvement

coveralls commented Feb 27, 2018 • edited

wu-sheng commented Feb 27, 2018

lytscu commented Feb 27, 2018

Choose a reason for hiding this comment

ascrutae Feb 27, 2018 • edited

Choose a reason for hiding this comment

lytscu Feb 27, 2018 • edited

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

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

ascrutae commented Feb 28, 2018

wu-sheng commented Feb 28, 2018

lytscu commented Feb 28, 2018

wu-sheng commented Mar 5, 2018

wu-sheng commented Apr 25, 2018

ascrutae commented Apr 26, 2018

wu-sheng commented May 2, 2018

wu-sheng commented Aug 8, 2018

lytscu commented Feb 27, 2018 •

edited

coveralls commented Feb 27, 2018 •

edited

ascrutae Feb 27, 2018 •

edited

lytscu Feb 27, 2018 •

edited