[BugFix] Incorrect Parent spans in tracing on Clone, ScatterGather and Invoke Mediators #2445
Conversation
WalkthroughThese changes introduce statistics context preservation mechanisms across async boundaries and continuation states in Apache Synapse. New interface methods enable tracking parent relationships for sequences in cloned/parallel contexts, with corresponding implementations and state persistence through continuation stacks during mediation phases. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CStack as ContinuationStackManager
participant CState as ContinuationState
participant Env as Axis2SynapseEnvironment
participant MsgCtx as MessageContext
participant Stats as Statistics Subsystem
App->>CStack: updateSeqContinuationState()
activate CStack
CStack->>Stats: capture parent index & list<br/>(if enabled)
CStack->>CState: setStatisticsParentIndex()
CStack->>CState: setStatisticsParentList()
deactivate CStack
App->>Env: mediateFromContinuationStateStack()
activate Env
Env->>CState: getStatisticsParentIndex()
Env->>CState: getStatisticsParentList()
rect rgba(200, 220, 255, 0.3)
note right of Env: Restore context
Env->>MsgCtx: set parent index property
Env->>MsgCtx: set parent list property
end
deactivate Env
App->>Env: callMediatorPostMediate()
activate Env
rect rgba(200, 220, 255, 0.3)
note right of Env: Restore in response path<br/>(if statistics enabled)
Env->>CState: getStatisticsParentIndex()
Env->>MsgCtx: restore parent context
end
deactivate Env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/core/src/main/java/org/apache/synapse/mediators/template/InvokeMediator.java (1)
281-347: Ensure statistics are closed in all execution paths including exceptions.The
mediate(MessageContext, ContinuationState)method opens statistics at line 294 but only closes them at specific points (lines 313, 317, 338, 343). If an exception occurs between opening and any of these close points, statistics may not be properly closed, potentially causing resource leaks or incorrect trace data.Consider wrapping the mediation logic in a try-finally block to guarantee statistics closure:
🔎 Proposed fix to ensure statistics closure
boolean isStatisticsEnabled = RuntimeStatisticCollector.isStatisticsEnabled(); Integer statisticReportingIndex = null; if (isStatisticsEnabled) { statisticReportingIndex = reportOpenStatistics(synCtx, false); } - if (subBranch == 0) { - // Default flow - ... - } else { - // Pre fetching invoke mediator flow - ... - } - - if (isStatisticsEnabled && statisticReportingIndex != null) { - reportCloseStatistics(synCtx, statisticReportingIndex); - } - - return result; + try { + if (subBranch == 0) { + // Default flow + ... + } else { + // Pre fetching invoke mediator flow + ... + } + return result; + } finally { + if (isStatisticsEnabled && statisticReportingIndex != null) { + reportCloseStatistics(synCtx, statisticReportingIndex); + } + }
🧹 Nitpick comments (4)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/parentresolving/MessageFlowRepresentationBasedParentResolver.java (1)
101-114: Consider performance implications of linear scan for parent resolution.The
findRecentCloneOrScatterGatherMediatormethod performs a linear backward scan through all span wrappers in the span store for every sequence component. For large span stores with many mediators, this could become a performance bottleneck, especially if sequences are frequently executed.Consider whether:
- The span store size is typically bounded/small enough that linear scans are acceptable
- An index or cache of Clone/ScatterGather mediators could optimize lookups
Based on learnings or profiling data, evaluate if optimization is needed.
modules/core/src/main/java/org/apache/synapse/continuation/ContinuationStackManager.java (1)
128-138: Add defensive type checking when retrieving statistics context properties.The code retrieves
MEDIATION_FLOW_STATISTICS_PARENT_LISTfrom the message context and casts it toList<Integer>without validation (lines 131-132). If the property exists but contains an incompatible type, this could throw aClassCastExceptionat runtime.While the code that sets these properties should ensure type correctness, consider adding defensive checks or documenting the contract that these properties must be
IntegerandList<Integer>respectively when present.🔎 Proposed defensive type checking
if (RuntimeStatisticCollector.isStatisticsEnabled()) { Integer parentIndex = (Integer) synCtx.getProperty( StatisticsConstants.MEDIATION_FLOW_STATISTICS_PARENT_INDEX); - java.util.List<Integer> parentList = (java.util.List<Integer>) synCtx.getProperty( - StatisticsConstants.MEDIATION_FLOW_STATISTICS_PARENT_LIST); + Object parentListObj = synCtx.getProperty( + StatisticsConstants.MEDIATION_FLOW_STATISTICS_PARENT_LIST); + java.util.List<Integer> parentList = null; + if (parentListObj instanceof java.util.List) { + parentList = (java.util.List<Integer>) parentListObj; + } // Store parent context in the continuation state leafChild.setStatisticsParentIndex(parentIndex); leafChild.setStatisticsParentList(parentList != null ? new java.util.LinkedList<>(parentList) : null); }modules/core/src/main/java/org/apache/synapse/continuation/AbstractContinuationState.java (1)
149-159: Consider defensive copying for mutable list field.The
getStatisticsParentList()method returns the internalList<Integer>reference directly (line 150), allowing external callers to modify the internal state. While thesetStatisticsParentList()method accepts the list reference directly (line 158), callers are creating defensive copies when setting (as seen inContinuationStackManager.javaline 137 andAxis2SynapseEnvironment.javaline 871).For consistency and safety, consider whether
getStatisticsParentList()should return a defensive copy or an unmodifiable view to prevent unintended mutations. However, if the current usage pattern guarantees immutability elsewhere, the current implementation is acceptable.modules/core/src/main/java/org/apache/synapse/ContinuationState.java (1)
82-104: Enhance interface Javadoc to clarify statistics parent context and usage patterns.The four methods are correctly implemented in AbstractContinuationState with appropriate field-level documentation. However, the interface method Javadoc could be more informative. Currently, it only states "The parent index for statistics tracing," but field-level comments already note these fields preserve "statistics parent index for tracing context preservation across async boundaries."
To further improve clarity, document:
- What "parent index" and "parent list" represent (flow position identifiers or span IDs for distributed tracing)
- The distinction:
parentIndexfor single-parent async chains (e.g., InvokeMediator),parentListfor multi-parent aggregations (Clone/ScatterGather)- Why both properties coexist and their relationship
Implementations exist and are actively used in Axis2SynapseEnvironment and ContinuationStackManager for preserving parent context across continuation boundaries.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
modules/core/src/main/java/org/apache/synapse/ContinuationState.javamodules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/parentresolving/AbstractParentResolver.javamodules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/parentresolving/MessageFlowRepresentationBasedParentResolver.javamodules/core/src/main/java/org/apache/synapse/continuation/AbstractContinuationState.javamodules/core/src/main/java/org/apache/synapse/continuation/ContinuationStackManager.javamodules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.javamodules/core/src/main/java/org/apache/synapse/mediators/template/InvokeMediator.java
🧰 Additional context used
🧬 Code graph analysis (2)
modules/core/src/main/java/org/apache/synapse/continuation/ContinuationStackManager.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
StatisticsConstants(26-202)modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (1)
RuntimeStatisticCollector(40-489)
modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
StatisticsConstants(26-202)modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java (1)
OpenEventCollector(39-324)
🔇 Additional comments (5)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/parentresolving/MessageFlowRepresentationBasedParentResolver.java (1)
65-69: Verify intended behavior when no Clone/ScatterGather mediator is found.When
isSequence(child)is true, the code returns the result offindRecentCloneOrScatterGatherMediator(spanStore)directly, which can be null if no Clone or ScatterGather mediator is found in the span store. This means sequences executing outside of cloned/parallel contexts will have a null parent resolved by this method.Verify that this is the intended behavior and that downstream consumers correctly handle null parents for sequences. If sequences should fall back to alternative parent resolution when not in a Clone/ScatterGather context, consider adding that logic here.
modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java (2)
845-873: Good defensive copying of statistics context across async boundaries.The implementation correctly preserves statistics parent context before opening continuation events and restores it afterward. The use of defensive copying when restoring
parentList(line 871:new java.util.LinkedList<>(savedParentList)) prevents unintended mutations across the async boundary.The sequence of operations—save before
openContinuationEvents, restore after—ensures the parent context is available for subsequent mediation while protecting against modifications during event processing.
900-917: Verify restoration timing for blocking call scenarios.The statistics context restoration in
callMediatorPostMediateoccurs at the method's start, before processing the response payload (lines 919-956). Confirm that this timing is correct for blocking Call mediator scenarios where the continuation state contains the saved context.If any post-mediation logic modifies the continuation state or message context properties before this restoration, the parent context could be lost or incorrect.
modules/core/src/main/java/org/apache/synapse/continuation/AbstractContinuationState.java (1)
34-42: LGTM! Clear documentation of statistics context fields.The new fields for statistics tracing context are well-documented with clear purpose statements explaining their use for "tracing context preservation across async boundaries."
modules/core/src/main/java/org/apache/synapse/ContinuationState.java (1)
22-22: LGTM!The
Listimport is necessary for the newgetStatisticsParentList()andsetStatisticsParentList()methods.
| protected static boolean isCloneMediator(StatisticDataUnit statisticDataUnit) { | ||
| return ComponentType.MEDIATOR.equals(statisticDataUnit.getComponentType()) && | ||
| "CloneMediator".equalsIgnoreCase(statisticDataUnit.getComponentName()); | ||
| } | ||
|
|
||
| protected static boolean isScatterGatherMediator(StatisticDataUnit statisticDataUnit) { | ||
| return ComponentType.MEDIATOR.equals(statisticDataUnit.getComponentType()) && | ||
| "ScatterGatherMediator".equalsIgnoreCase(statisticDataUnit.getComponentName()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the AbstractParentResolver file
find . -name "AbstractParentResolver.java" -type f 2>/dev/null | head -5Repository: wso2/wso2-synapse
Length of output: 209
🏁 Script executed:
#!/bin/bash
# Read the AbstractParentResolver file to see existing predicates
cat ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/parentresolving/AbstractParentResolver.javaRepository: wso2/wso2-synapse
Length of output: 3077
🏁 Script executed:
#!/bin/bash
# Find mediator implementations
find . -name "CloneMediator.java" -o -name "ScatterGatherMediator.java" -o -name "ForeachMediator.java" -o -name "CallMediator.java" -o -name "SendMediator.java" 2>/dev/null | head -10Repository: wso2/wso2-synapse
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Search for ForeachMediator and ScatterGatherMediator more broadly
find . -type f -name "*Foreach*.java" -o -type f -name "*ScatterGather*.java" 2>/dev/nullRepository: wso2/wso2-synapse
Length of output: 432
🏁 Script executed:
#!/bin/bash
# Check what getComponentName() or getType() returns for these mediators
# Start with CallMediator
echo "=== CallMediator ==="
grep -n "getComponentName\|getType" ./modules/core/src/main/java/org/apache/synapse/mediators/builtin/CallMediator.java | head -20
echo -e "\n=== SendMediator ==="
grep -n "getComponentName\|getType" ./modules/core/src/main/java/org/apache/synapse/mediators/builtin/SendMediator.java | head -20
echo -e "\n=== CloneMediator ==="
grep -n "getComponentName\|getType" ./modules/core/src/main/java/org/apache/synapse/mediators/eip/splitter/CloneMediator.java | head -20Repository: wso2/wso2-synapse
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Search for getComponentName in the codebase to understand how it's implemented
echo "=== Searching for getComponentName implementation ==="
rg -n "getComponentName|getMediatorName" --type java | head -30Repository: wso2/wso2-synapse
Length of output: 6369
🏁 Script executed:
#!/bin/bash
# Look for getMediatorName() implementation in AbstractMediator
echo "=== Searching for getMediatorName implementation ==="
rg -A 3 "getMediatorName()" ./modules/core/src/main/java/org/apache/synapse/mediators/AbstractMediator.java | head -30
# Also look for how component names are set in statistics
echo -e "\n=== Searching for componentName assignment in StatisticDataUnit ==="
rg -B 2 -A 2 "componentName\s*=" ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticDataUnit.java | head -40Repository: wso2/wso2-synapse
Length of output: 1726
🏁 Script executed:
#!/bin/bash
# Find ForeachMediator implementation to verify the naming
find . -type f -name "*ForeachMediator.java" -o -name "*IterateMediator.java" 2>/dev/null | head -10
# Also check what the actual class names are for the mediators in question
echo -e "\n=== Check actual class names ==="
rg "^public class (Call|Send|Clone|Foreach|Iterate|ScatterGather)" --type java | grep -i mediatorRepository: wso2/wso2-synapse
Length of output: 5297
🏁 Script executed:
#!/bin/bash
# Check what IterateMediator class name is (for "ForeachMediator" reference)
echo "=== IterateMediator class definition ==="
grep "^public class IterateMediator" ./modules/core/src/main/java/org/apache/synapse/mediators/eip/splitter/IterateMediator.java
# Check ScatterGather actual class name
echo -e "\n=== ScatterGather class definition ==="
grep "^public class ScatterGather" ./modules/core/src/main/java/org/apache/synapse/mediators/v2/ScatterGather.java
# Look for any references to "ForeachMediator" in the codebase
echo -e "\n=== Search for ForeachMediator references ==="
rg "ForeachMediator|foreachmediator" --type java modules/core/src/main/java/org/apache/synapse/mediators/ 2>/dev/null | head -20Repository: wso2/wso2-synapse
Length of output: 378
🏁 Script executed:
#!/bin/bash
# Let's verify by checking what getMediatorName() would return for each class
# We know it extracts the simple class name
# Test with a simple Python verification
python3 << 'EOF'
# Given that getMediatorName() returns the simple class name from the full qualified name:
# org.apache.synapse.mediators.builtin.CallMediator -> CallMediator
# org.apache.synapse.mediators.builtin.SendMediator -> SendMediator
# org.apache.synapse.mediators.eip.splitter.CloneMediator -> CloneMediator
# org.apache.synapse.mediators.eip.splitter.IterateMediator -> IterateMediator
# org.apache.synapse.mediators.v2.ScatterGather -> ScatterGather
actual_names = {
"CallMediator": "CallMediator",
"SendMediator": "SendMediator",
"CloneMediator": "CloneMediator",
"IterateMediator": "IterateMediator",
"ScatterGather": "ScatterGather"
}
predicates = {
"isCallMediator": "callmediator",
"isSendMediator": "sendmediator",
"isForeachMediator": "foreachmediator",
"isCloneMediator": "CloneMediator",
"isScatterGatherMediator": "ScatterGatherMediator"
}
print("Comparison of actual class names vs strings in predicates:")
print("=" * 80)
for pred_name, pred_string in predicates.items():
matching_class = None
for class_name in actual_names:
if class_name.lower() == pred_string.lower():
matching_class = class_name
break
if matching_class:
actual_value = actual_names[matching_class]
match = "✓ MATCHES (case-insensitive)" if actual_value.lower() == pred_string.lower() else "✗ MISMATCH"
print(f"{pred_name:30s} | Predicate: '{pred_string:25s}' | Actual class: {actual_value:20s} | {match}")
else:
print(f"{pred_name:30s} | Predicate: '{pred_string:25s}' | Actual class: {'NOT FOUND':20s} | ✗ NO MATCH")
print("\n" + "=" * 80)
print("Issue Summary:")
print("- isCallMediator checks 'callmediator' but CallMediator.getMediatorName() returns 'CallMediator'")
print("- isSendMediator checks 'sendmediator' but SendMediator.getMediatorName() returns 'SendMediator'")
print("- isForeachMediator checks 'foreachmediator' but the class is IterateMediator (not ForeachMediator)")
print("- isCloneMediator checks 'CloneMediator' ✓ matches actual")
print("- isScatterGatherMediator checks 'ScatterGatherMediator' but actual class is ScatterGather")
EOFRepository: wso2/wso2-synapse
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Simple check: let's look at the actual tests or usage to see what component names are expected
echo "=== Check test files for expected component names ==="
rg -A 5 -B 5 "callmediator|sendmediator|foreachmediator" --type java modules/core/src/test/ 2>/dev/null | head -50
echo -e "\n=== Check for ScatterGather usage ==="
rg "ScatterGather|scattergather" --type java modules/core/src/main/java/org/apache/synapse/aspects/ 2>/dev/null | head -20Repository: wso2/wso2-synapse
Length of output: 2155
🏁 Script executed:
#!/bin/bash
# Let's verify by looking for where component names are actually set
# Check OpenEventCollector or similar to see how component names are collected
echo "=== Looking for where component names are set in statistics collection ==="
rg -B 3 -A 3 "getMediatorName()" ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/ | head -60
echo -e "\n=== Check if there's any name transformation happening ==="
rg -A 5 "reportChildEntryEvent\|reportFlowContinuableEvent" ./modules/core/src/main/java/org/apache/synapse/mediators/AbstractMediator.java | head -40Repository: wso2/wso2-synapse
Length of output: 195
🏁 Script executed:
#!/bin/bash
# Let's check the actual mediator implementations directly
echo "=== CloneMediator.java - check for getMediatorName override ==="
rg -A 2 "getMediatorName" ./modules/core/src/main/java/org/apache/synapse/mediators/eip/splitter/CloneMediator.java
echo -e "\n=== CallMediator.java - check for getMediatorName override ==="
rg -A 2 "getMediatorName" ./modules/core/src/main/java/org/apache/synapse/mediators/builtin/CallMediator.java
echo -e "\n=== ScatterGather.java - check for getMediatorName override ==="
rg -A 2 "getMediatorName" ./modules/core/src/main/java/org/apache/synapse/mediators/v2/ScatterGather.java
echo -e "\n=== SendMediator.java - check for getMediatorName override ==="
rg -A 2 "getMediatorName" ./modules/core/src/main/java/org/apache/synapse/mediators/builtin/SendMediator.javaRepository: wso2/wso2-synapse
Length of output: 2277
🏁 Script executed:
#!/bin/bash
# Now let's verify our findings by checking what isForeachMediator actually matches
echo "=== Verify: Does 'foreachmediator' match 'IterateMediator'? ==="
cat > /tmp/test_case.java << 'EOF'
public class Test {
public static void main(String[] args) {
String actual = "IterateMediator";
String predicate = "foreachmediator";
System.out.println("Does '" + predicate + "' equalsIgnoreCase '" + actual + "'? " +
predicate.equalsIgnoreCase(actual));
String actual2 = "ScatterGather";
String predicate2 = "ScatterGatherMediator";
System.out.println("Does '" + predicate2 + "' equalsIgnoreCase '" + actual2 + "'? " +
predicate2.equalsIgnoreCase(actual2));
}
}
EOF
cd /tmp && javac test_case.java && java TestRepository: wso2/wso2-synapse
Length of output: 188
🏁 Script executed:
#!/bin/bash
# Let's do a final comprehensive check
echo "=== Final verification: All mediator class names and their getMediatorName() output ==="
echo ""
echo "Based on the code analysis:"
echo "getMediatorName() in AbstractMediator extracts simple class name from full qualified name"
echo ""
echo "Class Name -> getMediatorName() output"
echo "=====================================================";
echo "CallMediator -> CallMediator"
echo "SendMediator -> SendMediator"
echo "CloneMediator -> CloneMediator"
echo "IterateMediator -> IterateMediator"
echo "ScatterGather -> ScatterGather"
echo ""
echo "=== Predicates in AbstractParentResolver ==="
echo ""
echo "Predicate | String checked | Actual class name | Match?"
echo "=====================================================================";
echo "isForeachMediator | 'foreachmediator' | IterateMediator | NO!"
echo "isCallMediator | 'callmediator' | CallMediator | YES (case-insensitive)"
echo "isSendMediator | 'sendmediator' | SendMediator | YES (case-insensitive)"
echo "isCloneMediator | 'CloneMediator' | CloneMediator | YES"
echo "isScatterGatherMediator | 'ScatterGatherMediator' | ScatterGather | NO!"
echo ""
echo "=== Check if there's a ScatterGatherMediator class anywhere ==="
find . -type f -name "*ScatterGatherMediator.java" 2>/dev/nullRepository: wso2/wso2-synapse
Length of output: 1270
Fix component name strings to match actual mediator class names.
The predicates have inconsistencies with the actual component names returned by getMediatorName() (which extracts the simple class name from the full qualified name):
isForeachMediatorchecks"foreachmediator"but the actual class isIterateMediator— should be"iteratemediator"isScatterGatherMediatorchecks"ScatterGatherMediator"but the actual class isScatterGather— should be"scattergather"isCloneMediatorcorrectly checks"CloneMediator"matching the actual class- Existing predicates (
isCallMediator,isSendMediator) use lowercase ("callmediator","sendmediator") which work due toequalsIgnoreCase()but don't match the actual class names exactly
For consistency and correctness, either:
- Update the new predicates to follow the existing lowercase convention:
"cloneMediator"→"clonemediator","ScatterGatherMediator"→"scattergather" - Or fix all predicates to match the actual class name casing:
"CallMediator","SendMediator","CloneMediator","IterateMediator","ScatterGather"
🤖 Prompt for AI Agents
In
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/parentresolving/AbstractParentResolver.java
around lines 57 to 65, the component name string checks are inconsistent with
the actual mediator simple class names and the project's existing lowercase
convention; update the predicate string literals so they consistently match the
simple class names in lowercase (e.g., change the foreach/iterate check to
"iteratemediator", change the scatter/gather check to "scattergather", and
ensure clone uses "clonemediator") while keeping equalsIgnoreCase() to preserve
case-insensitive comparison.
1 participant
Purpose
Goals
Solution
Issue 01
Issue 02
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.