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
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
👮 Files not reviewed due to content moderation or server errors (2)
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
...c/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java
Outdated
Show resolved
Hide resolved
...plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java
Outdated
Show resolved
Hide resolved
Just-CJ
force-pushed
the
redis_tracer_fix
branch
from
7aae9bf to
49a8276
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java (2)
117-131: Consider adding error handling for null operationName.While the method correctly initializes the span and sets tags, it should validate the input parameter.
Consider adding parameter validation:
public SofaTracerSpan startTrace(String operationName) { + if (StringUtils.isBlank(operationName)) { + throw new IllegalArgumentException("operationName cannot be null or empty"); + } SofaTracerSpan sofaTracerSpan = clientSend(operationName); // ... rest of the method
133-151: Enhance error logging message.The error logging in the catch block could be more descriptive to aid in debugging.
Consider enhancing the error message:
- SelfLog.errorWithTraceId("redis processed", throwable); + SelfLog.errorWithTraceId("Failed to end Redis trace for operation: " + + sofaTracerSpan.getOperationName(), throwable);sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (2)
169-171: Add parameter validation in buildSpanConsider adding validation for the
operationNameparameter as it's crucial for trace identification.public void buildSpan(String operationName, Object key) { + if (operationName == null || operationName.trim().isEmpty()) { + throw new IllegalArgumentException("operationName cannot be null or empty"); + } SofaTracerSpan span = redisSofaTracer.startTrace(operationName); span.setTag("key", nullable(key)); }
Line range hint
1-180: Add tests for the new tracing implementationThe changes look good but would benefit from comprehensive tests to verify:
- Parent span association works correctly
- Error scenarios are properly handled
- Trace data includes all necessary information (command, key, etc.)
Would you like me to help create a test suite for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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(4 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 (6)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/tracer/RedisSofaTracer.java (3)
44-46: LGTM! Well-defined tracing constants.
The constants provide clear metadata for Redis operations tracing, following standard naming conventions.
50-50: LGTM! Thread-safe application name field.
The volatile field ensures thread-safe access to the application name across multiple threads.
117-131: Verify trace context propagation.
The implementation appears to address the parent span association issue by calling clientSend before span construction. Let's verify the trace context propagation.
Also applies to: 133-151
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have enough information about the trace context propagation implementation:
Trace context propagation is properly implemented
The implementation correctly handles trace context propagation through:
- Proper initialization via
clientSend(operationName)which creates spans with correct parent-child relationships - Consistent span context management across the codebase, as verified by the core tracer implementation in
AbstractTracer.java - Comprehensive span metadata handling with proper tag propagation (LOCAL_APP, COMMAND, COMPONENT, DB_TYPE)
The Redis plugin's trace context implementation aligns with other plugins' implementations (e.g., Dubbo, DataSource) and follows the core tracer's span creation patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify trace context propagation implementation
# Look for other places in the codebase that might create spans to ensure proper context propagation
# Search for span creation patterns
ast-grep --pattern 'clientSend($$$)'
# Search for potential places where spans might be created without proper context
rg -l "SofaTracerSpan" | xargs rg "new \w*Span" -A 3
Length of output: 53865
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (3)
81-89: Well-implemented error handling centralization
The introduction of handleTraceCompletion effectively addresses the code duplication concern raised in the previous review. The implementation properly handles both success and error scenarios.
130-139: LGTM: Consistent error handling in span lifecycle methods
The activateAndCloseSpan methods properly handle span lifecycle and error scenarios using the centralized handleTraceCompletion.
Also applies to: 142-152
93-99: Verify parent span association is working
While the tracing implementation looks correct, we need to verify that it properly associates with parent spans as mentioned in issue #503. The current changes don't explicitly show how parent span association is handled.
Also applies to: 106-112, 120-126
✅ Verification successful
Let me analyze the results and generate a final response based on the findings.
Parent span association is properly implemented
The codebase shows proper parent span handling through multiple mechanisms:
- The core tracer implementation maintains parent-child relationships:
- Parent span IDs are preserved in
SofaTracerSpanContext - Child spans correctly reference parent spans through
getParentId()andsetParentId() - Span context includes proper trace ID propagation
- The Redis plugin integrates with this parent span mechanism:
- Uses standard SOFA tracer span creation/completion methods
- Inherits the established parent-child span relationship handling
- Automatically associates with any active parent spans through the tracer context
The implementation aligns with the tracing fixes in issue #503 by leveraging the core tracer's built-in parent span association capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parent span handling in RedisSofaTracer
ast-grep --pattern 'class RedisSofaTracer {
$$$
startTrace($_) {
$$$
}
$$$
}'
# Search for parent span context handling
rg -A 5 "getParentSpan|setParentSpan|SpanContext"
Length of output: 79768
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (2)
Line range hint
130-152: Consider adding documentation about trace lifecycleWhile the implementation is correct, these methods play a crucial role in the trace lifecycle. Consider adding documentation to explain:
- When the span is activated
- How it relates to the parent span
- The importance of proper cleanup in the finally block
169-171: Add null check for operationName parameterThe buildSpan method should validate the operationName parameter to prevent potential NullPointerException.
Consider adding a null check:
public void buildSpan(String operationName, Object key) { + if (operationName == null) { + throw new IllegalArgumentException("operationName cannot be null"); + } SofaTracerSpan span = redisSofaTracer.startTrace(operationName); span.setTag("key", nullable(key)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java(4 hunks)
🔇 Additional comments (3)
sofa-tracer-plugins/sofa-tracer-redis-plugin/src/main/java/com/sofa/alipay/tracer/plugins/spring/redis/common/RedisActionWrapperHelper.java (3)
81-89: Well-structured error handling extraction
Good job extracting the error handling logic into a centralized method. This improves code maintainability and reduces duplication, addressing the feedback from previous reviews.
93-126: LGTM - Consistent trace handling in decorators
The decorator methods now consistently handle trace lifecycle and error cases, properly integrating with the centralized error handling.
Line range hint 49-171: Verify parent span association fix
While the code changes look good, we should verify that they properly fix the parent span association issue mentioned in #503.
✅ Verification successful
Let me generate another verification script to check the parent span handling in the Redis plugin specifically:
Based on the verification results, I can now provide a final response:
Parent span association is properly handled
The code correctly manages parent-child span relationships through:
- RedisSofaTracer extends AbstractClientTracer which provides proper parent span handling via clientSend/clientReceive lifecycle
- RedisActionWrapperHelper properly delegates to RedisSofaTracer for span creation and maintains span context through:
- Using startTrace() which internally calls clientSend() to create child spans
- Proper error handling and span completion in activateAndCloseSpan()
- Consistent span tag management for tracing context
The implementation follows the same pattern as other plugins in the codebase and properly maintains the parent-child relationship between spans.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for clientSend/clientReceive calls to verify proper span lifecycle
echo "Checking for proper span lifecycle implementation..."
rg -A 5 "clientSend|clientReceive" --type java
# Search for parent span handling in RedisSofaTracer
echo "Checking parent span handling in RedisSofaTracer..."
ast-grep --pattern 'class RedisSofaTracer {
$$$
startTrace($$$) {
$$$
}
$$$
}'
Length of output: 54380
Script:
#!/bin/bash
# Check Redis tracer implementation
echo "Checking Redis tracer implementation..."
ast-grep --pattern 'class RedisSofaTracer {
$$$
startTrace($$$) {
$$$
}
$$$
endTrace($$$) {
$$$
}
$$$
}'
# Check span context handling in RedisActionWrapperHelper
echo "Checking span context handling in RedisActionWrapperHelper..."
rg -A 5 "SofaTraceContext|SofaTracerSpan" sofa-tracer-plugins/sofa-tracer-redis-plugin/
Length of output: 19106
Just-CJ
force-pushed
the
redis_tracer_fix
branch
from
7ce1fe3 to
4292e8e
Compare
Motivation:
修复redis-plugin目前无法正常打印日志的问题
Modification:
参考datasource-plugin中RedisSofaTracer中增加了startTrace和endTrace方法，并在RedisActionWrapperHelper中进行调用
Result:
Fixes #503.
Summary by CodeRabbit