[Feature] Show connector operations as a single span in traces #2446
Conversation
WalkthroughThe changes implement a SKIP_CHILD_EVENTS control mechanism to consolidate connector invocations into single tracing spans. When processing an InvokeMediator:Connector, child events are suppressed during execution and re-enabled upon closure, enabling connectors to be reported as atomic units in OpenTelemetry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenEventCollector
participant MessageContext
participant CloseEventCollector
Client->>OpenEventCollector: Process "InvokeMediator:Connector"<br/>(reportFlowContinuableEvent)
OpenEventCollector->>MessageContext: Set SKIP_CHILD_EVENTS = true
Note over OpenEventCollector,MessageContext: Connector span initiated<br/>(single unit)
OpenEventCollector->>OpenEventCollector: Skip child event emissions<br/>(SKIP_CHILD_EVENTS=true)
Note over Client: Connector mediators execute<br/>(events suppressed)
Client->>CloseEventCollector: Connector closes<br/>(reportCloseEvent)
CloseEventCollector->>MessageContext: Set SKIP_CHILD_EVENTS = false
Note over CloseEventCollector,MessageContext: Connector span completed
CloseEventCollector->>OpenEventCollector: Resume event emission<br/>(SKIP_CHILD_EVENTS=false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (1)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/CloseEventCollector.java (1)
126-131: Consider extracting the magic string to a constant.The literal
"InvokeMediator:Connector"is duplicated here and inOpenEventCollector. Extracting it to a constant inStatisticsConstants(e.g.,CONNECTOR_MEDIATOR_NAME) would improve maintainability and ensure consistency.🔎 Proposed refactor
In
StatisticsConstants.java:+public static final String CONNECTOR_MEDIATOR_NAME = "InvokeMediator:Connector";Then update both collectors to use the constant:
-if (componentType == ComponentType.MEDIATOR && Objects.equals(componentName, "InvokeMediator:Connector")) { +if (componentType == ComponentType.MEDIATOR && Objects.equals(componentName, StatisticsConstants.CONNECTOR_MEDIATOR_NAME)) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/CloseEventCollector.javamodules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.javamodules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.javamodules/core/src/main/java/org/apache/synapse/mediators/AbstractMediator.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/aspects/flow/statistics/collectors/CloseEventCollector.java (1)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
StatisticsConstants(26-204)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
StatisticsConstants(26-204)modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/OpenTelemetryManagerHolder.java (1)
OpenTelemetryManagerHolder(31-87)
🔇 Additional comments (7)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/util/StatisticsConstants.java (1)
202-203: LGTM!The new
SKIP_CHILD_EVENTSconstant follows the existing naming conventions and is appropriately placed with the other message context property keys. It serves as the coordination key for the connector span suppression mechanism.modules/core/src/main/java/org/apache/synapse/mediators/AbstractMediator.java (1)
478-483: LGTM!Minor Javadoc improvement that clarifies the return value of
getMediatorName(). This provides useful context given thatInvokeMediatornow overrides this method with custom naming logic.modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/CloseEventCollector.java (1)
119-132: Verify:SKIP_CHILD_EVENTSis not reset when a connector closes with an error.The flag reset only occurs in the non-error branch. If a connector closes with an error (
isError == true), subsequent mediators in any fault handling sequence may not create spans. Please confirm this is the intended behavior.modules/core/src/main/java/org/apache/synapse/mediators/template/InvokeMediator.java (1)
281-285: LGTM!The override clearly differentiates connectors from templates by appending the appropriate suffix. This enables the statistics collectors to identify connector operations and apply the span suppression logic. The
isDynamicMediator()check is the correct discriminator.modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java (3)
66-69: LGTM!Good defensive initialization of
SKIP_CHILD_EVENTStofalseat entry points. This ensures the flag has a defined state at the start of each message flow.
217-237: Verify:reportFlowSplittingEventdoes not respectSKIP_CHILD_EVENTS.Flow splitting mediators (Clone, Iterate) will create spans even when inside a connector operation. If a connector uses these mediators internally, they would not be suppressed. Please confirm this is intentional behavior, or if these should also check
SKIP_CHILD_EVENTS.
191-195: LGTM on the connector identification logic.The special-case handling correctly identifies
InvokeMediator:Connectorand sets theSKIP_CHILD_EVENTSflag to suppress child spans. This is the core mechanism enabling connectors to appear as single spans.Note: The magic string
"InvokeMediator:Connector"is also duplicated inCloseEventCollector.java— see the earlier suggestion to extract it to a constant.
| if (!isFiltered && isOpenTelemetryEnabled() | ||
| && messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS).equals(false)) { | ||
| OpenTelemetryManagerHolder.getOpenTelemetryManager().getHandler() | ||
| .handleOpenEntryEvent(statisticDataUnit, messageContext); | ||
| } |
There was a problem hiding this comment.
Potential NPE if SKIP_CHILD_EVENTS is unexpectedly null.
Line 114 calls .equals(false) directly on the property value without a null check. While the initialization at lines 67-69 should prevent this in normal flows, the pattern differs from the safer null-checks used in reportChildEntryEvent and reportFlowContinuableEvent.
🔎 Proposed fix for consistency
boolean isFiltered = isSpanFilteredMediator(componentName, aspectConfiguration);
- if (!isFiltered && isOpenTelemetryEnabled()
- && messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS).equals(false)) {
+ boolean skipChildEvents = Boolean.TRUE.equals(
+ messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS));
+ if (!isFiltered && isOpenTelemetryEnabled() && !skipChildEvents) {
OpenTelemetryManagerHolder.getOpenTelemetryManager().getHandler()
.handleOpenEntryEvent(statisticDataUnit, messageContext);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isFiltered && isOpenTelemetryEnabled() | |
| && messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS).equals(false)) { | |
| OpenTelemetryManagerHolder.getOpenTelemetryManager().getHandler() | |
| .handleOpenEntryEvent(statisticDataUnit, messageContext); | |
| } | |
| boolean isFiltered = isSpanFilteredMediator(componentName, aspectConfiguration); | |
| boolean skipChildEvents = Boolean.TRUE.equals( | |
| messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS)); | |
| if (!isFiltered && isOpenTelemetryEnabled() && !skipChildEvents) { | |
| OpenTelemetryManagerHolder.getOpenTelemetryManager().getHandler() | |
| .handleOpenEntryEvent(statisticDataUnit, messageContext); | |
| } |
🤖 Prompt for AI Agents
In
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java
around lines 113 to 117, the condition calls
messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS).equals(false)
which can NPE if the property is null; change the check to use a null-safe
Boolean comparison (e.g.,
Boolean.FALSE.equals(messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS)))
or explicitly test for null before equals so the overall if remains: if
(!isFiltered && isOpenTelemetryEnabled() &&
Boolean.FALSE.equals(messageContext.getProperty(StatisticsConstants.SKIP_CHILD_EVENTS)))
then call handleOpenEntryEvent, matching the null-safety used in
reportChildEntryEvent and reportFlowContinuableEvent.
1 participant
Purpose
Goals
Approach
Problem:
Connectors (which are dynamic InvokeMediator templates) contain multiple internal mediators (property mediators, call mediators, class mediators, etc.), and by default each creates its own span, cluttering the trace with implementation details that users don't need to see.
Solution:
The PR introduces a SKIP_CHILD_EVENTS flag that controls span creation for child mediators:
Differentiate connectors from templates - InvokeMediator.getMediatorName() now appends ":Connector" for dynamic mediators and ":Template" for regular templates, allowing the statistics collectors to identify connector operations
Initialize skip flag - OpenEventCollector initializes SKIP_CHILD_EVENTS property to false on every message to ensure clean state
Activate suppression on connector entry - When OpenEventCollector detects an "InvokeMediator:Connector" component opening, it sets SKIP_CHILD_EVENTS = true, which signals that all subsequent child mediators should not create spans
Suppress child spans - In openFlowContinuableEvent() and openChildEntryEvent(), before calling the OpenTelemetry handler, the code checks if SKIP_CHILD_EVENTS is true and skips span creation if so, allowing only the connector's own span to be created
Deactivate suppression on connector exit - When CloseEventCollector detects "InvokeMediator:Connector" closing, it resets SKIP_CHILD_EVENTS = false, restoring normal span creation for subsequent mediators outside the connector
Traces after Changes
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.