Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bugs in Redisson plugin #693

Merged
merged 3 commits into from
May 22, 2024
Merged

Fix bugs in Redisson plugin #693

merged 3 commits into from
May 22, 2024

Conversation

CzyerChen
Copy link
Contributor

Fix bugs in Redisson plugin

  • Fix NPE in Redisson plugin since Redisson 3.20.0.

Fix NPE in Redisson tracing since 3.20.0, because ServiceManager.Config replaces Config in MasterSlaveConnectionManager.

ERROR 2024-05-20 09:42:35.670 redisson-netty-2-19 InstMethodsInter : class[class org.redisson.client.RedisConnection] after method[send] intercept failure 
java.lang.NullPointerException
	at org.apache.skywalking.apm.agent.core.context.ContextManager.stopSpan(ContextManager.java:194)
	at org.apache.skywalking.apm.plugin.redisson.v3.RedisConnectionMethodInterceptor.afterMethod(RedisConnectionMethodInterceptor.java:98)
	at org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstMethodsInter.intercept(InstMethodsInter.java:97)
	at org.redisson.client.RedisConnection.send(RedisConnection.java)
	at org.redisson.client.RedisConnection.async(RedisConnection.java:252)
	at org.redisson.client.RedisConnection.async(RedisConnection.java:228)
	at org.redisson.client.handler.PingConnectionHandler.sendPing(PingConnectionHandler.java:65)
  • Support for showing batch command details and ignoring PING commands in Redisson plugin.

Add params: skywalking.plugin.redisson.show_batch_commands=true(default false), to show batch_execute details
Add params: skywalking.plugin.redisson.ping_ignored=true(default true), to avoid ping tracing cost, the filtering configuration of PING since 3.13.6 that default ping per 30s
2024-05-16 211518

  • Fix peer value of Master-Slave mode in Redisson plugin.

connect to redis in Master-Slave mode

Config config = new Config();
config.useMasterSlaveServers()
    .setMasterAddress("redis://127.0.0.1:7001")
    .addSlaveAddress("redis://127.0.0.1:7002");

RedissonClient redisson = Redisson.create(config);

peer before:
redisson-0516-ms-scenario

peer now:
General-Root General-Service

  • About the redisson-scenario, for redis single node , 3.5.0 - 3.30.0 has been tested, but 3.12.5 - 3.26.0 not included in the testcase because of the difference in expectedData
When startup params: -Dskywalking.plugin.redisson.trace_redis_parameters=true -Dskywalking.plugin.redisson.show_batch_commands=true -Dskywalking.plugin.redisson.ping_ignored=true
3.5.0 - 3.12.4： 
segmentItems:
- serviceName: redisson-scenario
  segmentSize: 2
  segments:
  - segmentId: 8113e722beb74d1b9afd97ba51a3881a.77.17162612625310000
    spans:
    - operationName: HEAD:/case/healthCheck
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716261262557
      endTime: 1716261262881
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/healthCheck'}
      - {key: http.method, value: HEAD}
      - {key: http.status_code, value: '200'}
  - segmentId: 8113e722beb74d1b9afd97ba51a3881a.78.17162612629290000
    spans:
    - operationName: Redisson/SET
      parentSpanId: 0
      spanId: 1
      spanLayer: Cache
      startTime: 1716261263432
      endTime: 1716261263511
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: SET}
      - {key: cache.key, value: key_a}
      - {key: cache.op, value: write}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 0
      spanId: 2
      spanLayer: Cache
      startTime: 1716261263553
      endTime: 1716261263554
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:SET;SET;PEXPIRE'}
    - operationName: Redisson/EVAL
      parentSpanId: 3
      spanId: 4
      spanLayer: Cache
      startTime: 1716261263638
      endTime: 1716261263638
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: EVAL}
      - {key: cache.key, value: 'if (redis.call(''exists'', KEYS[1]) == 0) then redis.call(''hincrby'',
          KEYS[1], ARGV[2], 1); redis.call(''pexpire'', KEYS[1], ARGV[1])'}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 3
      spanLayer: Cache
      startTime: 1716261263635
      endTime: 1716261263648
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_a}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '10000'}
      - {key: thread.id, value: '78'}
    - operationName: Redisson/EVAL
      parentSpanId: 5
      spanId: 6
      spanLayer: Cache
      startTime: 1716261263648
      endTime: 1716261263648
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: EVAL}
      - {key: cache.key, value: 'if (redis.call(''exists'', KEYS[1]) == 0) then redis.call(''hincrby'',
          KEYS[1], ARGV[2], 1); redis.call(''pexpire'', KEYS[1], ARGV[1])'}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 5
      spanLayer: Cache
      startTime: 1716261263648
      endTime: 1716261263656
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_b}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '20'}
      - {key: thread.id, value: '78'}
    - operationName: GET:/case/redisson-case
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716261262929
      endTime: 1716261263661
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/redisson-case'}
      - {key: http.method, value: GET}
      - {key: http.status_code, value: '200'}

3.12.5 - 3.16.8   
segmentItems:
- serviceName: redisson-scenario
  segmentSize: 2
  segments:
  - segmentId: 7fe18ab87b544234b4c277cd05e4ebe7.77.17162614778760000
    spans:
    - operationName: HEAD:/case/healthCheck
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716261477894
      endTime: 1716261478140
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/healthCheck'}
      - {key: http.method, value: HEAD}
      - {key: http.status_code, value: '200'}
  - segmentId: 7fe18ab87b544234b4c277cd05e4ebe7.78.17162614781750000
    spans:
    - operationName: Redisson/SET
      parentSpanId: 0
      spanId: 1
      spanLayer: Cache
      startTime: 1716261478235
      endTime: 1716261478242
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: SET}
      - {key: cache.key, value: key_a}
      - {key: cache.op, value: write}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 0
      spanId: 2
      spanLayer: Cache
      startTime: 1716261478290
      endTime: 1716261478290
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:SET;SET;PEXPIRE'}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 3
      spanId: 4
      spanLayer: Cache
      startTime: 1716261478357
      endTime: 1716261478357
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:EVAL'}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 3
      spanLayer: Cache
      startTime: 1716261478346
      endTime: 1716261478369
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_a}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '10000'}
      - {key: thread.id, value: '78'}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 5
      spanId: 6
      spanLayer: Cache
      startTime: 1716261478371
      endTime: 1716261478371
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:EVAL'}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 5
      spanLayer: Cache
      startTime: 1716261478371
      endTime: 1716261478374
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_b}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '20'}
      - {key: thread.id, value: '78'}
    - operationName: GET:/case/redisson-case
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716261478175
      endTime: 1716261478384
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/redisson-case'}
      - {key: http.method, value: GET}
      - {key: http.status_code, value: '200'}

3.17.0 - 3.26.0   
segmentItems:
- serviceName: redisson-scenario
  segmentSize: 2
  segments:
  - segmentId: 344f0a5bd7f6403484ea074501c26d70.93.17162619129550000
    spans:
    - operationName: HEAD:/case/healthCheck
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716261912970
      endTime: 1716261913186
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/healthCheck'}
      - {key: http.method, value: HEAD}
      - {key: http.status_code, value: '200'}
  - segmentId: 344f0a5bd7f6403484ea074501c26d70.94.17162619132080000
    spans:
    - operationName: Redisson/SET
      parentSpanId: 0
      spanId: 1
      spanLayer: Cache
      startTime: 1716261913394
      endTime: 1716261913396
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: SET}
      - {key: cache.key, value: key_a}
      - {key: cache.op, value: write}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 0
      spanId: 2
      spanLayer: Cache
      startTime: 1716261913424
      endTime: 1716261913424
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:SET;SET;EVAL'}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 3
      spanId: 4
      spanLayer: Cache
      startTime: 1716261913498
      endTime: 1716261913498
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:EVAL'}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 3
      spanLayer: Cache
      startTime: 1716261913497
      endTime: 1716261913502
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_a}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '10000'}
      - {key: thread.id, value: '94'}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 5
      spanId: 6
      spanLayer: Cache
      startTime: 1716261913507
      endTime: 1716261913507
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:EVAL'}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 5
      spanLayer: Cache
      startTime: 1716261913507
      endTime: 1716261913509
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_b}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '20'}
      - {key: thread.id, value: '94'}
    - operationName: GET:/case/redisson-case
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716261913208
      endTime: 1716261913514
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/redisson-case'}
      - {key: http.method, value: GET}
      - {key: http.status_code, value: '200'}

3.26.1 - 3.30.0  
segmentItems:
- serviceName: redisson-scenario
  segmentSize: 2
  segments:
  - segmentId: cffeac304ea5483fafb79376e257353c.93.17162625084100000
    spans:
    - operationName: HEAD:/case/healthCheck
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716262508451
      endTime: 1716262509297
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/healthCheck'}
      - {key: http.method, value: HEAD}
      - {key: http.status_code, value: '200'}
  - segmentId: cffeac304ea5483fafb79376e257353c.94.17162625094090000
    spans:
    - operationName: Redisson/SET
      parentSpanId: 0
      spanId: 1
      spanLayer: Cache
      startTime: 1716262509855
      endTime: 1716262509861
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: SET}
      - {key: cache.key, value: key_a}
      - {key: cache.op, value: write}
    - operationName: Redisson/BATCH_EXECUTE
      parentSpanId: 0
      spanId: 2
      spanLayer: Cache
      startTime: 1716262509992
      endTime: 1716262509993
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: 'BATCH_EXECUTE:SET;SET;EVAL'}
    - operationName: Redisson/EVAL
      parentSpanId: 3
      spanId: 4
      spanLayer: Cache
      startTime: 1716262510177
      endTime: 1716262510177
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: EVAL}
      - {key: cache.key, value: 'if ((redis.call(''exists'', KEYS[1]) == 0) or (redis.call(''hexists'',
          KEYS[1], ARGV[2]) == 1)) then redis.call(''hincrby'', KEYS[1], '}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 3
      spanLayer: Cache
      startTime: 1716262510176
      endTime: 1716262510184
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_a}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '10000'}
      - {key: thread.id, value: '94'}
    - operationName: Redisson/EVAL
      parentSpanId: 5
      spanId: 6
      spanLayer: Cache
      startTime: 1716262510198
      endTime: 1716262510198
      componentId: 56
      isError: false
      spanType: Exit
      peer: redis-server:6379
      skipAnalysis: false
      tags:
      - {key: cache.type, value: Redis}
      - {key: cache.instance, value: '172.18.0.2:6379'}
      - {key: cache.cmd, value: EVAL}
      - {key: cache.key, value: 'if ((redis.call(''exists'', KEYS[1]) == 0) or (redis.call(''hexists'',
          KEYS[1], ARGV[2]) == 1)) then redis.call(''hincrby'', KEYS[1], '}
    - operationName: Redisson/LOCK
      parentSpanId: 0
      spanId: 5
      spanLayer: Cache
      startTime: 1716262510197
      endTime: 1716262510201
      componentId: 56
      isError: false
      spanType: Local
      peer: ''
      skipAnalysis: false
      tags:
      - {key: lock.name, value: lock_b}
      - {key: cache.type, value: Redis}
      - {key: lease.time, value: '20'}
      - {key: thread.id, value: '94'}
    - operationName: GET:/case/redisson-case
      parentSpanId: -1
      spanId: 0
      spanLayer: Http
      startTime: 1716262509409
      endTime: 1716262510226
      componentId: 14
      isError: false
      spanType: Entry
      peer: ''
      skipAnalysis: false
      tags:
      - {key: url, value: 'http://localhost:8080/redisson-scenario/case/redisson-case'}
      - {key: http.method, value: GET}
      - {key: http.status_code, value: '200'}
  • Explain briefly why the bug exists and how to fix it.
  • Update the CHANGES log.

@wu-sheng wu-sheng added this to the 9.3.0 milestone May 22, 2024
@wu-sheng
Copy link
Member

Note, the plugin tests fail about this plugin, please recheck.

@wu-sheng
Copy link
Member

Meanwhile, with your ASF ID created, you should be able to join ASF org(GitHub), and make the CI running automatically without my manual approval.

@CzyerChen
Copy link
Contributor Author

Meanwhile, with your ASF ID created, you should be able to join ASF org(GitHub), and make the CI running automatically without my manual approval.

OK

@wu-sheng wu-sheng merged commit ffbd90c into apache:main May 22, 2024
190 of 191 checks passed
Ricehomesky
Ricehomesky reviewed Jun 23, 2024
View reviewed changes
...main/java/org/apache/skywalking/apm/plugin/redisson/v3/RedisConnectionMethodInterceptor.java
} else if (allArguments[0] instanceof CommandData) {
CommandData commandData = (CommandData) allArguments[0];
command = commandData.getCommand().getName();
operationName = operationName + command;
arguments = commandData.getParams();
if ("PING".equals(command) && !RedissonPluginConfig.Plugin.Redisson.SHOW_PING_COMMAND) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wu-sheng This change will lead to a problem that the plugin cannot find an active span to close after "Ping" execution. Skywalking would throw a NullPointerException. Maybe it needs a flag to duel this situation.
NullPointerException

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to write that PR? I think this is easy. You just need to add if ("PING".equals(command) && !RedissonPluginConfig.Plugin.Redisson.SHOW_PING_COMMAND) { check in the #afterMethod and avoid ContextManager.stopSpan(); in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CzyerChen I think this is definitely going to happen.

Copy link
Contributor

@lujiajing1126 lujiajing1126 Jun 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue has been resolved by @Ricehomesky from our team in our private code several days ago. I suppose he can raise a PR very soon.

@Ricehomesky Ricehomesky mentioned this pull request Jun 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lujiajing1126 lujiajing1126 lujiajing1126 left review comments

@wu-sheng wu-sheng wu-sheng approved these changes

@Ricehomesky Ricehomesky Ricehomesky left review comments

Assignees
No one assigned
Labels
plugin
Projects
None yet
Milestone
9.3.0
4 participants
@CzyerChen @wu-sheng @lujiajing1126 @Ricehomesky