fix redis trace plugin #550
Conversation
sofastack-cla
bot
added
bug
WalkthroughThe changes in this pull request focus on simplifying the tracing mechanism in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java (2)
117-132: Remove unnecessary semicolon on line 129.
The implementation correctly addresses the core issue by calling clientSend before creating the span, which fixes the parent span association problem mentioned in issue #503.
Apply this diff to remove the unnecessary semicolon:
sofaTracerSpan.setTag(Tags.COMPONENT.getKey(), COMPONENT_NAME);
sofaTracerSpan.setTag(Tags.DB_TYPE.getKey(), DB_TYPE);
- ;134-152: Enhance error logging message for better debugging.
The error message in SelfLog could be more descriptive to aid in debugging.
Apply this diff to improve the error message:
- SelfLog.errorWithTraceId("redis processed", throwable);
+ SelfLog.errorWithTraceId("Failed to process Redis trace completion", throwable);Additionally, consider adding debug logging for cases where the span or trace context is null to help identify potential issues during development.
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (1)
185-187: Consider adding method documentation.
The implementation looks good and aligns with the new tracing approach. Consider adding Javadoc to explain the method's purpose and parameter requirements.
+ /**
+ * Builds and starts a new trace span for Redis operations.
+ *
+ * @param operationName The name of the Redis operation being performed
+ * @param key The key being operated on, can be null
+ */
public void buildSpan(String operationName, Object key) {
SofaTracerSpan span = redisSofaTracer.startTrace(operationName);
span.setTag("key", nullable(key));
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (5 hunks)
- sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java (2 hunks)
🔇 Additional comments (4)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java (2)
20-36: LGTM! Well-structured constants for Redis tracing.
The new constants and imports provide clear identification for Redis operations and support the tracing functionality effectively.
Also applies to: 44-46
117-152: Verify the trace linking in integration tests.
The implementation looks solid and addresses the core issues. To ensure everything works as expected:
✅ Verification successful
Based on the code review and test implementation analysis, I can now provide a final response:
Redis tracing implementation is properly tested and verified
The implementation has been verified through:
- Integration test
SofaTracerRedisConnectionTestthat exercises the Redis connection with tracing - Proper span lifecycle management in
RedisSofaTracerwith:- Parent span association via
clientSend() - Complete span attributes including component, DB type, and command tags
- Proper error handling and span completion via
clientReceive()
- Parent span association via
The original review comment's concerns about trace linking have been addressed by the existing implementation and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for integration tests that verify parent span association
rg -l "RedisSofaTracer.*test" --type java
# Look for existing Redis trace tests
rg -l "RedisTemplate.*test" --type java
# Check for any existing span verification in tests
ast-grep --pattern 'assert$_($_->getParentSpanId())'
Length of output: 140
Script:
#!/bin/bash
# Look for test files related to Redis tracing
fd -e java test -x grep -l "Redis" {}
# Search for Redis test configurations
fd -e java -e xml test -x grep -l "redis" {}
# Look for Redis tracer usage in tests
rg "new RedisSofaTracer\(" --type java
# Check for span assertions in test files
rg "assert.*Span" --type java
Length of output: 34322
Script:
#!/bin/bash
# Look for Redis integration test content
cat ./tracer-sofa-boot-starter/src/test/java/com/alipay/sofa/tracer/boot/redis/SofaTracerRedisConnectionTest.java
# Search for Redis tracer implementation
cat ./sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java
# Look for Redis span reporter implementations
rg "RedisSpanReport" --type java
Length of output: 9226
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (2)
Line range hint 136-168: LGTM! Proper span lifecycle management implemented.
The implementation correctly handles span activation and closure, with proper error handling and consistent patterns across both Supplier and Runnable variants. This change helps fix the parent span association issue mentioned in #503.
49-50: Verify command parameter validation.
While the tracing implementation looks correct, consider adding validation for the command parameter to prevent potential NPEs and improve robustness.
public <T> T doInScope(String command, byte[] key, Supplier<T> supplier) {
+ if (command == null || command.trim().isEmpty()) {
+ throw new IllegalArgumentException("Command cannot be null or empty");
+ }
buildSpan(command, deserialize(key));
return activateAndCloseSpan(supplier);
}Let's verify the command parameter usage across the codebase:
Also applies to: 54-55, 59-60, 64-65, 69-71
| redisSofaTracer.startTrace(operateName); | ||
| return supplier.get(); | ||
| } catch (Throwable t) { | ||
| candidateThrowable = t; | ||
| throw t; | ||
| } finally { | ||
| if (candidateThrowable != null) { | ||
| span.setTag(Tags.ERROR.getKey(), candidateThrowable.getMessage()); | ||
| redisSofaTracer.clientReceive(SofaTracerConstant.RESULT_CODE_ERROR); | ||
| redisSofaTracer.endTrace(SofaTracerConstant.RESULT_CODE_ERROR, | ||
| candidateThrowable.getMessage()); | ||
| } else { | ||
| redisSofaTracer.clientReceive(SofaTracerConstant.RESULT_CODE_SUCCESS); | ||
| redisSofaTracer.endTrace(SofaTracerConstant.RESULT_CODE_SUCCESS, null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting duplicate error handling logic.
The error handling pattern is duplicated across decorate, decorateThrowing, and other methods. Consider extracting this into a reusable method to improve maintainability.
+ private void handleTraceCompletion(Throwable candidateThrowable) {
+ if (candidateThrowable != null) {
+ redisSofaTracer.endTrace(SofaTracerConstant.RESULT_CODE_ERROR,
+ candidateThrowable.getMessage());
+ } else {
+ redisSofaTracer.endTrace(SofaTracerConstant.RESULT_CODE_SUCCESS, null);
+ }
+ }
public <T> T decorate(Supplier<T> supplier, String operateName) {
Throwable candidateThrowable = null;
try {
redisSofaTracer.startTrace(operateName);
return supplier.get();
} catch (Throwable t) {
candidateThrowable = t;
throw t;
} finally {
- if (candidateThrowable != null) {
- redisSofaTracer.endTrace(SofaTracerConstant.RESULT_CODE_ERROR,
- candidateThrowable.getMessage());
- } else {
- redisSofaTracer.endTrace(SofaTracerConstant.RESULT_CODE_SUCCESS, null);
- }
+ handleTraceCompletion(candidateThrowable);
}
}Also applies to: 102-112, 121-131
Motivation:
修复redis-plugin目前无法正常打印日志的问题
Modification:
参考datasource-plugin中RedisSofaTracer中增加了startTrace和endTrace方法，并在RedisActionWrapperHelper中进行调用
Result:
Fixes #503.
Summary by CodeRabbit