Skip to content

Conversation

@KalinduGandara
Copy link
Contributor

@KalinduGandara KalinduGandara commented Dec 2, 2025

fixes: wso2/product-micro-integrator#4496

Summary by CodeRabbit

  • New Features
    • Per-message option to skip main sequence execution.
    • OpenTelemetry spans can include error code, error message and custom properties.
    • Mediator-level span filtering plus a trace-filter toggle (configurable per mediator).
    • Tracing/statistics extended to support Axis2 message contexts for broader coverage.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds telemetry and tracing enhancements: new constants, trace-filter toggle, Axis2 MessageContext overloads and span-store/scope handling, custom span properties (error/code/message), mediator filtering for OpenTelemetry emission, and a SKIP_MAIN_SEQUENCE message routing guard.

Changes

Cohort / File(s) Summary
Constants & Config
modules/core/src/main/java/org/apache/synapse/SynapseConstants.java, modules/core/src/main/java/org/apache/synapse/config/xml/XMLConfigConstants.java, modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java
Added constants: SKIP_MAIN_SEQUENCE, TRACE_FILTER_ATTRIB_NAME, TRACE_FILTER_ENABLE, ERROR_CODE_ATTRIBUTE_KEY, ERROR_MESSAGE_ATTRIBUTE_KEY, OLTP_CUSTOM_SPAN_TAGS, OLTP_FILTERED_MEDIATOR_NAMES.
Trace Filter Toggle & XML parsing
modules/core/src/main/java/org/apache/synapse/aspects/AspectConfiguration.java, modules/core/src/main/java/org/apache/synapse/config/xml/AbstractMediatorFactory.java
Added trace filter flag and enable/disable API; XML attribute parsing to enable trace filter on mediators.
Mediator Span Filtering
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java, modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java
RuntimeStatisticCollector loads mediator-name filter set and exposes isSpanFilteredMediator(...); OpenEventCollector gates OpenTelemetry emission when mediator is filtered.
Statistic Data Models
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/BasicStatisticDataUnit.java, .../StatisticDataUnit.java, .../StatisticsLog.java
Added customProperties map; added componentTypeString, errorCode, errorMessage, isOuterLayerSpan fields with accessors; StatisticsLog now carries customProperties.
Telemetry Event Handler APIs
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/OpenEventHandler.java, .../CloseEventHandler.java
Added Axis2 org.apache.axis2.context.MessageContext overloads to open/close event handler interfaces.
Span Management — Axis2 integration
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java, .../stores/SpanStore.java, .../scoping/TracingScopeManager.java
Added Axis2-aware overloads for start/finish/add span operations, tracing-scope extraction from Axis2 MessageContext, SpanStore lifecycle methods, and Axis2-specific span handling (transport headers, custom tags, outer-level span handling).
Span Tagging & Enrichment
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java
Exported customProperties, errorCode, errorMessage and other metadata as span attributes; added Axis2 MessageContext overload for tag extraction.
Message Routing Guard
modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java
Added check to skip mediating with main sequence when SynapseConstants.SKIP_MAIN_SEQUENCE is set true on Axis2 MessageContext.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Client as Client/Transport
    participant Axis2MsgCtx as org.apache.axis2.context.MessageContext
    participant SynapseMsgCtx as org.apache.synapse.MessageContext
    participant TracingScopeMgr as TracingScopeManager
    participant OpenCollector as OpenEventCollector
    participant SpanHandler as SpanHandler
    participant SpanStore as SpanStore

    Client->>Axis2MsgCtx: deliver request
    Axis2MsgCtx->>TracingScopeMgr: getTracingScope(msgCtx)
    TracingScopeMgr-->>Axis2MsgCtx: TracingScope (create/lookup)
    Axis2MsgCtx->>OpenCollector: reportOpenEvent(statDataUnit, msgCtx)
    OpenCollector->>SpanHandler: handleOpenEntryEvent(statDataUnit, msgCtx)
    SpanHandler->>SpanStore: addSpanWrapper(key, span, statDataUnit, parent, msgCtx)
    SpanStore-->>SpanHandler: stored SpanWrapper
    Note right of SpanHandler: span tags set (customProps, errors, headers)
    SpanHandler->>SpanStore: finishSpan(spanWrapper, msgCtx, optionalError)
    SpanStore-->>Axis2MsgCtx: update/remove spans
    Axis2MsgCtx->>Client: respond
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas needing extra attention:

  • SpanHandler.java — Axis2 overloads, synchronization, transport header propagation, outer-level span lifecycle and scope cleanup.
  • RuntimeStatisticCollector.java — initialization and case-insensitive parsing of mediator filter list.
  • OpenEventCollector.java — correctness and placement of mediator filtering checks across all open-event paths.
  • SpanStore.java & TracingScopeManager.java — consistent FLOW_STATISTICS_ID extraction/propagation between Synapse and Axis2 MessageContexts.
  • SpanTagger.java — mapping/export of customProperties, error code/message and payload handling.

Poem

🐰 I hop with joy, with spans in hand,
Filters set neat across the land,
Axis2 whispers context true,
Custom tags and errors join the view,
A little rabbit cheers this telemetry crew!

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 2 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is severely incomplete, containing only a single issue link with no elaboration on purpose, goals, approach, user stories, testing, or other required template sections. Expand the description to include Purpose, Goals, Approach, User stories, Release notes, Documentation, Testing details, and Security checks following the repository template.
Docstring Coverage ⚠️ Warning Docstring coverage is 47.22% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Linked Issues check ❓ Inconclusive The linked issue #4496 describes 'Adding OpenTelemetry support on Dataservices' but lacks implementation details. The PR code changes include trace filtering, custom properties, and OpenTelemetry improvements that appear related but the issue description is too vague to fully validate alignment. Clarify whether the code changes directly address Dataservices OpenTelemetry support or provide foundational improvements; consider updating the linked issue with more specific implementation requirements.
Out of Scope Changes check ❓ Inconclusive While most changes align with OpenTelemetry and trace filter objectives, the scope cannot be fully determined due to the vague linked issue description. Several utility enhancements (e.g., SKIP_MAIN_SEQUENCE constant, custom span properties) appear foundational but their necessity for the stated Dataservices objective is unclear. Provide more specific details in the PR description or linked issue about which changes are core to the objective and which are supporting infrastructure changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'Add trace filter support, custom headers and improve opentelemetry' accurately describes the primary changes: introducing trace filtering, custom header support, and OpenTelemetry enhancements.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b69a5fd and 3d0610e.

📒 Files selected for processing (1)
  • modules/core/src/main/java/org/apache/synapse/SynapseConstants.java

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (7)
modules/core/src/main/java/org/apache/synapse/config/xml/XMLConfigConstants.java (1)

148-150: LGTM! Consider adding a disable constant for completeness.

The constants follow existing naming conventions. For consistency with TRACE_DISABLE and STATISTICS_DISABLE, you may want to add TRACE_FILTER_DISABLE = "disable" for future use, though this is optional.

modules/core/src/main/java/org/apache/synapse/aspects/AspectConfiguration.java (1)

125-135: Consider adding guard checks for consistency.

The existing enableStatistics() and disableStatistics() methods include guards (if (!statisticsEnable) / if (statisticsEnable)) to avoid redundant assignments. The new trace filter methods don't follow this pattern. While functionally correct, adding guards would maintain consistency.

     public void enableTraceFilter() {
-        this.traceFilterEnable = true;
+        if (!traceFilterEnable) {
+            this.traceFilterEnable = true;
+        }
     }

     public void disableTraceFilter() {
-        this.traceFilterEnable = false;
+        if (traceFilterEnable) {
+            this.traceFilterEnable = false;
+        }
     }
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1)

162-241: Axis2 overload has asymmetric feature set compared to Synapse version.

This method omits context property/variable map collection and endpoint attributes present in the Synapse MessageContext overload, but adds error code/message handling (lines 224-232). If this asymmetry is intentional (due to Axis2 context limitations), consider adding the error code/message handling to the Synapse version as well for consistency.

Consider adding error code/message handling to the Synapse MessageContext version:

         if (spanWrapper.getCloseEventStatisticDataUnit() != null) {
             if (spanWrapper.getCloseEventStatisticDataUnit().getCustomProperties() != null) {
                 spanWrapper.getCloseEventStatisticDataUnit().getCustomProperties().forEach(
                         (key, value) -> span.setAttribute(key, String.valueOf(value))
                 );
             }
+            if (spanWrapper.getCloseEventStatisticDataUnit().getErrorCode() != null) {
+                span.setAttribute(TelemetryConstants.ERROR_CODE_ATTRIBUTE_KEY,
+                        spanWrapper.getCloseEventStatisticDataUnit().getErrorCode());
+            }
+            if (spanWrapper.getCloseEventStatisticDataUnit().getErrorMessage() != null) {
+                span.setAttribute(TelemetryConstants.ERROR_MESSAGE_ATTRIBUTE_KEY,
+                        spanWrapper.getCloseEventStatisticDataUnit().getErrorMessage());
+            }
         }
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/scoping/TracingScopeManager.java (1)

62-82: Consider null-safety for tracingScopeId.

The Axis2 overload correctly mirrors the Synapse version. However, if FLOW_STATISTICS_ID is not set on the MessageContext, extractTracingScopeId returns null, which could lead to storing/retrieving scopes with a null key.

Consider adding a null check:

     public TracingScope getTracingScope(org.apache.axis2.context.MessageContext msgCtx) {
         synchronized (tracingScopes) {
             String tracingScopeId = extractTracingScopeId(msgCtx);
+            if (tracingScopeId == null) {
+                return null; // or throw an exception/log a warning
+            }
             if (tracingScopes.containsKey(tracingScopeId)) {

This would also be beneficial for the existing Synapse MessageContext overload.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1)

91-93: Consider using OTEL prefix instead of OLTP.

The constants use OLTP prefix (e.g., oltp.custom.span.header.tags), but OpenTelemetry is typically abbreviated as OTEL. Unless OLTP is an established convention in this codebase, consider renaming for consistency with OpenTelemetry naming conventions:

-    public static final String OLTP_CUSTOM_SPAN_TAGS = "oltp.custom.span.header.tags";
+    public static final String OTEL_CUSTOM_SPAN_TAGS = "otel.custom.span.header.tags";

-    public static final String OLTP_FILTERED_MEDIATOR_NAMES = "oltp.filtered.mediator.names";
+    public static final String OTEL_FILTERED_MEDIATOR_NAMES = "otel.filtered.mediator.names";
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/CloseEventHandler.java (1)

51-58: Consider adding missing Axis2 overloads for completeness.

The interface adds Axis2 overloads for handleCloseEntryEvent, handleCloseFlowForcefully, and handleCloseEntryWithErrorEvent, but not for handleTryEndFlow and handleScatterGatherFinishEvent. If Axis2 context support is needed for those operations in the future, this inconsistency may cause issues.

Verify whether handleTryEndFlow and handleScatterGatherFinishEvent need Axis2 overloads for complete Dataservices OpenTelemetry support.

Also applies to: 81-88

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (1)

252-279: Significant code duplication in custom span tags logic.

The custom span tags extraction logic is duplicated between the Synapse startSpan (lines 255-279) and Axis2 startSpan (lines 355-378). Consider extracting to a shared helper method.

Extract to a private helper method:

private void extractAndSetCustomSpanTags(Map headersMap, StatisticDataUnit statisticDataUnit, 
                                          Object msgContext, boolean isSynapseContext) {
    Object prevCustomSpanTagsObj = isSynapseContext 
        ? ((MessageContext) msgContext).getProperty(TelemetryConstants.OLTP_CUSTOM_SPAN_TAGS)
        : ((org.apache.axis2.context.MessageContext) msgContext).getProperty(TelemetryConstants.OLTP_CUSTOM_SPAN_TAGS);
    // ... rest of shared logic
}

Also applies to: 355-378

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b10bd4 and 064637b.

📒 Files selected for processing (17)
  • modules/core/src/main/java/org/apache/synapse/SynapseConstants.java (1 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/AspectConfiguration.java (2 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java (5 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (5 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/BasicStatisticDataUnit.java (3 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticDataUnit.java (3 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticsLog.java (3 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/CloseEventHandler.java (2 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/OpenEventHandler.java (1 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (11 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/scoping/TracingScopeManager.java (2 hunks)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/stores/SpanStore.java (3 hunks)
  • modules/core/src/main/java/org/apache/synapse/config/xml/AbstractMediatorFactory.java (1 hunks)
  • modules/core/src/main/java/org/apache/synapse/config/xml/XMLConfigConstants.java (1 hunks)
  • modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/OpenTelemetryManagerHolder.java (1)
  • OpenTelemetryManagerHolder (31-87)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1)
  • TelemetryConstants (21-94)
modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java (1)
modules/core/src/main/java/org/apache/synapse/SynapseConstants.java (1)
  • SynapseConstants (30-745)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/stores/SpanStore.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/TracingUtils.java (1)
  • TracingUtils (28-65)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1)
  • SpanTagger (33-242)
modules/core/src/main/java/org/apache/synapse/config/xml/AbstractMediatorFactory.java (1)
modules/core/src/main/java/org/apache/synapse/config/xml/XMLConfigConstants.java (1)
  • XMLConfigConstants (30-151)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (3)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1)
  • TelemetryConstants (21-94)
modules/core/src/main/java/org/apache/synapse/config/SynapsePropertiesLoader.java (1)
  • SynapsePropertiesLoader (32-119)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/TracingUtils.java (1)
  • TracingUtils (28-65)
🔇 Additional comments (18)
modules/core/src/main/java/org/apache/synapse/config/xml/AbstractMediatorFactory.java (1)

186-196: LGTM! Trace filter attribute handling follows existing patterns.

The implementation correctly mirrors the existing patterns for trace and statistics attribute handling, maintaining consistency in the codebase.

modules/core/src/main/java/org/apache/synapse/SynapseConstants.java (1)

743-744: LGTM!

The new constant follows existing naming conventions and is appropriately placed with related constants.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticsLog.java (1)

173-174: LGTM!

The customProperties field follows the existing pattern for other map fields like contextPropertyMap and transportPropertyMap, with proper initialization in the constructor and a getter method.

Also applies to: 206-206, 434-437

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java (3)

105-110: LGTM! Filtering logic is correctly applied.

The trace filtering logic properly skips OpenTelemetry handling when the mediator is in the filtered list or when trace filtering is explicitly enabled on the component.


139-144: Consistent filtering applied across event methods.

The filtering logic is consistently applied across reportChildEntryEvent, reportFlowContinuableEvent, reportFlowSplittingEvent, and reportFlowAggregateEvent, maintaining uniform behavior for trace filtering.

Also applies to: 175-180, 212-217, 248-253


266-281: Verify: reportFlowAsynchronousEvent lacks trace filtering.

Unlike other event reporting methods in this class, reportFlowAsynchronousEvent does not include an isFiltered check before calling the OpenTelemetry handler. Confirm whether this omission is intentional for asynchronous events or represents an inconsistency that should be addressed.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/BasicStatisticDataUnit.java (1)

71-75: LGTM! The new customProperties field follows existing patterns.

The addition of customProperties with getter/setter is consistent with other fields in this data carrier class (e.g., elasticMetadata, messageContext). The field appropriately allows null values and follows the same mutable reference pattern used throughout the class.

Also applies to: 139-146

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1)

140-152: LGTM! Custom properties propagation for Synapse MessageContext.

The addition correctly exports custom properties from both open and close event data units as span attributes, ensuring complete property propagation.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (3)

145-159: LGTM! Mediator span filter initialization is well-implemented.

The initialization correctly:

  • Guards against null/empty configuration values
  • Uses stream API for clean parsing
  • Normalizes to lowercase for case-insensitive matching
  • Logs the filter list at debug level

181-193: LGTM! Axis2 MessageContext overload provides necessary compatibility.

The method correctly mirrors the Synapse MessageContext version for setting trace IDs in Axis2 contexts.


286-297: LGTM! Filter check is correctly implemented.

The method safely handles:

  • Null componentName via early check
  • Component names without colons (split(":")[0] returns original string)
  • Case-insensitive matching via lowercase normalization
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/scoping/TracingScopeManager.java (1)

94-102: LGTM! Axis2 context ID extraction is consistent with existing pattern.

The overloaded method correctly extracts the tracing scope ID from the Axis2 MessageContext using the same property key.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/OpenEventHandler.java (1)

37-43: Interface extension requires implementations to be updated.

The new Axis2 MessageContext overload is appropriate for enabling Axis2-based tracing. Verify that all classes implementing OpenEventHandler have been updated to implement this new method signature.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1)

85-86: LGTM!

The error attribute constants follow OpenTelemetry semantic conventions for error tracking (error.code, error.message).

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/CloseEventHandler.java (1)

35-42: LGTM!

The new Axis2 MessageContext overloads maintain consistency with existing method signatures and Javadoc style.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticDataUnit.java (2)

82-86: LGTM!

The new fields (componentTypeString, errorCode, errorMessage, isOuterLayerSpan) are well-structured and follow the existing patterns in the class. Default initialization for isOuterLayerSpan = false is appropriate.

Also applies to: 134-144


319-350: LGTM!

The new accessor methods follow JavaBean conventions and are consistent with the existing codebase style.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (1)

568-585: Both finishSpan overloads have identical logic in the outer-level span handling block.

The Synapse and Axis2 versions (lines 554–567 and 568–585, respectively) both execute the same code path in their else blocks: spanStore.finishSpan() followed by tracingScopeManager.cleanupTracingScope(). There is no difference in how outer-level spans are finished between the two overloads. Neither method contains calls to isEventCollectionFinished or cleanupContinuationStateSequences — these methods do not exist in this class.

Likely an incorrect or invalid review comment.

Comment on lines +267 to +272
String[] customTags = customTagsString.split(",");
Map<String, Object> customTagsMap = new HashMap<>();
for (String tag : customTags) {
if (headersMap.containsKey(tag.trim())) {
customTagsMap.put(tag, headersMap.get(tag));
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Inconsistent key handling in custom tags map.

In the Synapse version (line 271), the key is stored with the original (potentially untrimmed) value, but the header lookup uses tag.trim(). In the Axis2 version (line 370), the same pattern exists. This may cause issues if the configured tag has leading/trailing whitespace.

                     for (String tag : customTags) {
-                        if (headersMap.containsKey(tag.trim())) {
-                            customTagsMap.put(tag, headersMap.get(tag));
+                        String trimmedTag = tag.trim();
+                        if (headersMap.containsKey(trimmedTag)) {
+                            customTagsMap.put(trimmedTag, headersMap.get(trimmedTag));
                         }
                     }

Also applies to: 365-371

🤖 Prompt for AI Agents
In
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java
around lines 267-272 (and similarly around 365-371), the code trims the tag when
looking up headers but uses the untrimmed original tag as the map key, which can
cause mismatched keys when configured tags have surrounding whitespace; update
the logic to trim the tag once into a local variable (e.g., String key =
tag.trim()), use that trimmed key for the headersMap lookup and also as the key
when putting into customTagsMap so both lookup and storage are consistent.

}
}

statisticDataUnit.setTransportHeaderMap(headersMap);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Remove redundant setTransportHeaderMap call.

statisticDataUnit.setTransportHeaderMap(headersMap) is already called at line 352. This duplicate call at line 381 is unnecessary.

-        statisticDataUnit.setTransportHeaderMap(headersMap);
-
         String spanId = TracingUtils.extractId(statisticDataUnit);
📝 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.

Suggested change
statisticDataUnit.setTransportHeaderMap(headersMap);
🤖 Prompt for AI Agents
In
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java
around line 381, there is a redundant call to
statisticDataUnit.setTransportHeaderMap(headersMap) (it was already set at line
352); remove the duplicate call at line 381 so the transport header map is only
assigned once, leaving the earlier assignment intact.

Comment on lines 474 to 489
@Override
public void handleCloseFlowForcefully(BasicStatisticDataUnit basicStatisticDataUnit,
org.apache.axis2.context.MessageContext msgCtx) {
TracingScope tracingScope = tracingScopeManager.getTracingScope(msgCtx);
SpanStore spanStore = tracingScope.getSpanStore();
String spanWrapperId = TracingUtils.extractId(basicStatisticDataUnit);
SpanWrapper spanWrapper = spanStore.getSpanWrapper(spanWrapperId);

// finish the current span
handleCloseEvent(basicStatisticDataUnit, msgCtx, true);

// finish outer level spans since the control is not returned to the original flow to close them gracefully.
while (spanWrapper.getParentSpanWrapper() != null) {
spanWrapper = spanWrapper.getParentSpanWrapper();
spanStore.finishSpan(spanWrapper, msgCtx, true);
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Inconsistent error flag behavior between Synapse and Axis2 overloads.

The Axis2 handleCloseFlowForcefully always calls handleCloseEvent with isError=true (line 483) and finishes parent spans with isError=true (line 488), whereas the Synapse version uses isError=false (line 465). This behavioral difference may cause inconsistent span status reporting.

         // finish the current span
-        handleCloseEvent(basicStatisticDataUnit, msgCtx, true);
+        handleCloseEvent(basicStatisticDataUnit, msgCtx, false);

         // finish outer level spans since the control is not returned to the original flow to close them gracefully.
         while (spanWrapper.getParentSpanWrapper() != null) {
             spanWrapper = spanWrapper.getParentSpanWrapper();
-            spanStore.finishSpan(spanWrapper, msgCtx, true);
+            spanStore.finishSpan(spanWrapper, msgCtx);
         }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java
around lines 474 to 490: the Axis2 overload incorrectly marks the close flow as
an error by passing isError=true to handleCloseEvent and to spanStore.finishSpan
for parent spans, creating inconsistent span status versus the Synapse overload
which uses isError=false; change the calls to use isError=false (or otherwise
mirror the Synapse overload behavior) so
handleCloseEvent(basicStatisticDataUnit, msgCtx, false) is called and
spanStore.finishSpan(spanWrapper, msgCtx, false) is used in the parent loop,
ensuring consistent non-error closure semantics.

Comment on lines 183 to 209
/**
* Denotes the end of a span.
* Adds tags to the span and removes reference to the appropriate span wrapper in activeSpanWrappers.
*
* @param spanWrapper Span wrapper object, which has been already created
* @param msgCtx Synapse message context
* @param isError finishing the span with an error
*/
public void finishSpan(SpanWrapper spanWrapper, org.apache.axis2.context.MessageContext msgCtx, boolean isError) {
if (spanWrapper != null && spanWrapper.getSpan() != null) {
if (spanWrapper.getStatisticDataUnit() != null) {
SpanTagger.setSpanTags(spanWrapper, msgCtx);
}
if (isError) {
spanWrapper.getSpan().setStatus(StatusCode.ERROR);
}
spanWrapper.getSpan().end();
activeSpanWrappers.remove(spanWrapper);
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix Javadoc comment: incorrect parameter description.

The Javadoc states @param msgCtx Synapse message context but the parameter is actually an Axis2 MessageContext.

     /**
      * Denotes the end of a span.
      * Adds tags to the span and removes reference to the appropriate span wrapper in activeSpanWrappers.
      *
      * @param spanWrapper Span wrapper object, which has been already created
-     * @param msgCtx      Synapse message context
+     * @param msgCtx      Axis2 message context
      * @param isError     finishing the span with an error
      */
🤖 Prompt for AI Agents
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/stores/SpanStore.java
around lines 183 to 202: the Javadoc for finishSpan incorrectly describes the
msgCtx parameter as a "Synapse message context"; update the @param tag to
correctly state that msgCtx is an Axis2 MessageContext (e.g., "Axis2
MessageContext" or "org.apache.axis2.context.MessageContext") and ensure wording
matches the method signature and imports.


if (isOpenTelemetryEnabled()) {
boolean isFiltered = isSpanFilteredMediator(componentName) ||
(aspectConfiguration != null && aspectConfiguration.isTraceFilterEnable());
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we consider reordering the conditions to check isTraceFilterEnable() first before isSpanFilteredMediator(componentName)? This way, when trace filtering is enabled (common case), we skip the mediator list lookup entirely and avoid unnecessary computation. WDYT

boolean isFiltered = (aspectConfiguration != null && aspectConfiguration.isTraceFilterEnable()) || isSpanFilteredMediator(componentName);


if (isOpenTelemetryEnabled()) {
boolean isFiltered = isSpanFilteredMediator(componentName) ||
(aspectConfiguration != null && aspectConfiguration.isTraceFilterEnable());
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we check the possibility of moving isTraceEnabled check inside isSpanFilteredMediator() method for readability and simplicity?

public Map<String, Object> getCustomProperties() {
return customProperties;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a EOF line here

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

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/aspects/flow/statistics/tracing/opentelemetry/management/scoping/TracingScopeManager.java (1)

62-102: Eliminate code duplication between overloaded methods.

Both getTracingScope overloads (lines 48-60 and 70-82) contain identical logic, as do both extractTracingScopeId helper methods (lines 90-92 and 100-102). This violates the DRY principle and increases maintenance burden.

🔎 Proposed refactor to eliminate duplication
     public TracingScope getTracingScope(MessageContext synCtx) {
-        synchronized (tracingScopes) {
-            String tracingScopeId = extractTracingScopeId(synCtx);
-            if (tracingScopes.containsKey(tracingScopeId)) {
-                // Already existing scope. Return its reference
-                return tracingScopes.get(tracingScopeId);
-            } else {
-                TracingScope tracingScope = new TracingScope(tracingScopeId);
-                tracingScopes.put(tracingScopeId, tracingScope);
-                return tracingScope;
-            }
-        }
+        return getTracingScope(extractTracingScopeId(synCtx));
     }

     public TracingScope getTracingScope(org.apache.axis2.context.MessageContext msgCtx) {
+        return getTracingScope(extractTracingScopeId(msgCtx));
+    }
+
+    private TracingScope getTracingScope(String tracingScopeId) {
         synchronized (tracingScopes) {
-            String tracingScopeId = extractTracingScopeId(msgCtx);
             if (tracingScopes.containsKey(tracingScopeId)) {
                 // Already existing scope. Return its reference
                 return tracingScopes.get(tracingScopeId);
             } else {
                 TracingScope tracingScope = new TracingScope(tracingScopeId);
                 tracingScopes.put(tracingScopeId, tracingScope);
                 return tracingScope;
             }
         }
     }
♻️ Duplicate comments (3)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (3)

252-279: Fix inconsistent key handling in custom tags extraction.

The code trims the tag for the header lookup (line 270) but uses the untrimmed original tag as the map key (line 271) and for retrieval. This can cause mismatched keys when configured tags have surrounding whitespace.

🔎 Proposed fix
                 } else {
                     String[] customTags = customTagsString.split(",");
                     Map<String, Object> customTagsMap = new HashMap<>();
                     for (String tag : customTags) {
-                        if (headersMap.containsKey(tag.trim())) {
-                            customTagsMap.put(tag, headersMap.get(tag));
+                        String trimmedTag = tag.trim();
+                        if (headersMap.containsKey(trimmedTag)) {
+                            customTagsMap.put(trimmedTag, headersMap.get(trimmedTag));
                         }
                     }

Based on past review comments.


365-371: Fix inconsistent key handling in custom tags extraction (Axis2 version).

Same issue as in the Synapse version: the code trims for lookup (line 368) but uses untrimmed tag for map operations (line 369).

🔎 Proposed fix
                 } else {
                     String[] customTags = customTagsString.split(",");
                     Map<String, Object> customTagsMap = new HashMap<>();
                     for (String tag : customTags) {
-                        if (headersMap.containsKey(tag.trim())) {
-                            customTagsMap.put(tag, headersMap.get(tag));
+                        String trimmedTag = tag.trim();
+                        if (headersMap.containsKey(trimmedTag)) {
+                            customTagsMap.put(trimmedTag, headersMap.get(trimmedTag));
                         }
                     }

Based on past review comments.


473-489: Inconsistent error flag behavior between Synapse and Axis2 versions.

The Axis2 handleCloseFlowForcefully passes isError=true to both handleCloseEvent (line 482) and parent span finishing (line 487), while the Synapse version at line 464 uses isError=false. This creates inconsistent span status reporting between the two context types.

Verify whether forceful flow closure should be marked as an error in Axis2 contexts but not in Synapse contexts, or if this is a bug.

#!/bin/bash
# Search for other usages of handleCloseFlowForcefully to understand the intended semantics
rg -n "handleCloseFlowForcefully" --type java -C 3

Based on past review comments.

🧹 Nitpick comments (2)
modules/core/src/main/java/org/apache/synapse/config/xml/XMLConfigConstants.java (1)

149-150: Consider consolidating the "enable" string literal.

Both TRACE_FILTER_ENABLE and the existing TRACE_ENABLE (line 107) use the same literal value "enable". Consider introducing a shared constant (e.g., ENABLE_VALUE = "enable") to reduce duplication and improve maintainability.

🔎 Proposed refactor
+    /** Common enable value */
+    public static final String ENABLE_VALUE = "enable";
     /** The Trace value 'enable' */
-    public static final String TRACE_ENABLE = "enable";
+    public static final String TRACE_ENABLE = ENABLE_VALUE;
     /** The Trace value 'disable' */
     public static final String TRACE_DISABLE = "disable";
     
     // ...
     
-    public static final String TRACE_FILTER_ENABLE = "enable";
+    public static final String TRACE_FILTER_ENABLE = ENABLE_VALUE;
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1)

155-240: Consider extracting common span attribute setting logic.

The new Axis2 overload duplicates significant attribute-setting logic from the Synapse MessageContext version. While the data sources differ, many attribute-setting patterns are similar (component metadata, payloads, custom properties, etc.).

Consider extracting common attribute-setting operations into private helper methods to reduce duplication and improve maintainability.

🔎 Example refactor for common attribute setting
+    private static void setComponentAttributes(Span span, StatisticDataUnit dataUnit) {
+        if (dataUnit.getComponentName() != null) {
+            span.setAttribute(TelemetryConstants.COMPONENT_NAME_ATTRIBUTE_KEY, dataUnit.getComponentName());
+        }
+        if (dataUnit.getComponentType() != null) {
+            span.setAttribute(TelemetryConstants.COMPONENT_TYPE_ATTRIBUTE_KEY, 
+                    dataUnit.getComponentType().toString());
+        }
+        if (dataUnit.getComponentId() != null) {
+            span.setAttribute(TelemetryConstants.COMPONENT_ID_ATTRIBUTE_KEY, dataUnit.getComponentId());
+        }
+        if (dataUnit.getHashCode() != null) {
+            span.setAttribute(TelemetryConstants.HASHCODE_ATTRIBUTE_KEY, dataUnit.getHashCode());
+        }
+    }
+
+    private static void setCustomProperties(Span span, StatisticDataUnit dataUnit) {
+        if (dataUnit.getCustomProperties() != null) {
+            dataUnit.getCustomProperties().forEach(
+                    (key, value) -> span.setAttribute(key, String.valueOf(value))
+            );
+        }
+    }

Then call these helpers from both overloads.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 064637b and b69a5fd.

📒 Files selected for processing (17)
  • modules/core/src/main/java/org/apache/synapse/SynapseConstants.java
  • modules/core/src/main/java/org/apache/synapse/aspects/AspectConfiguration.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/BasicStatisticDataUnit.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticDataUnit.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticsLog.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/CloseEventHandler.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/OpenEventHandler.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/scoping/TracingScopeManager.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/stores/SpanStore.java
  • modules/core/src/main/java/org/apache/synapse/config/xml/AbstractMediatorFactory.java
  • modules/core/src/main/java/org/apache/synapse/config/xml/XMLConfigConstants.java
  • modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java
🚧 Files skipped from review as they are similar to previous changes (7)
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/CloseEventHandler.java
  • modules/core/src/main/java/org/apache/synapse/core/axis2/Axis2SynapseEnvironment.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/BasicStatisticDataUnit.java
  • modules/core/src/main/java/org/apache/synapse/SynapseConstants.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticDataUnit.java
  • modules/core/src/main/java/org/apache/synapse/aspects/AspectConfiguration.java
  • modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/data/raw/StatisticsLog.java
🧰 Additional context used
🧬 Code graph analysis (3)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/stores/SpanStore.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/TracingUtils.java (1)
  • TracingUtils (28-65)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1)
  • SpanTagger (33-242)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (3)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1)
  • TelemetryConstants (21-94)
modules/core/src/main/java/org/apache/synapse/config/SynapsePropertiesLoader.java (1)
  • SynapsePropertiesLoader (32-119)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/TracingUtils.java (1)
  • TracingUtils (28-65)
modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (2)
modules/core/src/main/java/org/apache/synapse/aspects/AspectConfiguration.java (1)
  • AspectConfiguration (35-136)
modules/core/src/main/java/org/apache/synapse/config/SynapsePropertiesLoader.java (1)
  • SynapsePropertiesLoader (32-119)
🔇 Additional comments (19)
modules/core/src/main/java/org/apache/synapse/config/xml/AbstractMediatorFactory.java (1)

187-196: LGTM!

The trace filter attribute parsing follows the same pattern as the existing trace and statistics handling. The implementation is consistent and integrates well with the existing code structure.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/TelemetryConstants.java (1)

85-93: LGTM!

The new constants follow existing naming conventions and provide clear, descriptive values for error attributes, custom span tags, and filtered mediator names.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/OpenEventCollector.java (1)

105-109: Filtering logic is consistently applied.

The filtering gate for OpenTelemetry emission is consistently implemented across all event reporting methods. The approach is clear and maintainable.

Note: A previous reviewer suggested reordering conditions to check isTraceFilterEnable() first for potential optimization, which is worth considering but not critical.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/event/OpenEventHandler.java (1)

37-43: LGTM!

The Axis2 MessageContext overload addition is straightforward and properly documented. The interface extension maintains consistency with the existing method signature pattern.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/helpers/SpanTagger.java (1)

141-153: LGTM!

The addition of custom property export from both open and close statistic data units enriches span attributes and provides valuable tracing context.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/collectors/RuntimeStatisticCollector.java (2)

146-160: LGTM!

The mediator span filter initialization properly handles configuration, builds a case-insensitive filter set, and provides appropriate debug logging.


287-302: Component name format handling is robust; no defensive handling needed.

The code correctly handles component names both with and without colons. While some mediators (PropertyMediator, HeaderMediator, VariableMediator) append a colon and additional context to distinguish instances, base mediators return only their class name (e.g., LogMediator, SendMediator). The split(":")[0] operation safely extracts the mediator type in both cases—for "PropertyMediator:myProperty" it returns "PropertyMediator", and for "LogMediator" it returns "LogMediator" unchanged.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/stores/SpanStore.java (3)

116-147: LGTM! Anonymous sequence handling correctly implemented.

The Axis2 overload now includes the anonymous sequence handling (lines 136-142) that was previously flagged in the past review. The logic is identical to the Synapse MessageContext version (lines 103-109), ensuring consistent parent-child span relationship management across both context types.


159-167: LGTM! Consistent delegation pattern.

The Axis2 overload correctly delegates to the 3-parameter version with isError=false, mirroring the pattern used in the Synapse MessageContext overload at line 156.


190-209: LGTM! Javadoc corrected and implementation consistent.

The Javadoc has been corrected to properly state "Axis2 message context" (line 195), addressing the past review comment. The implementation is consistent with the Synapse MessageContext version, properly handling span tagging, error status, ending, and cleanup.

modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java (9)

28-32: LGTM! Standard Java collections imports.

The addition of standard Java collections imports (HashMap, List, Map, Objects, ConcurrentHashMap) supports the new custom span tags functionality.


42-42: LGTM! Required imports for new functionality.

The imports for TelemetryConstants and SynapsePropertiesLoader are necessary for the custom span tags feature introduced in this PR.

Also applies to: 51-51


136-143: LGTM! Axis2 context support for open events.

The new public overload properly handles Axis2 MessageContext by obtaining the tracing scope and synchronizing on the span store before delegating to the private startSpan method.


301-388: Review the Axis2 startSpan implementation.

The Axis2 startSpan overload differs from the Synapse version in parent resolution (lines 310-311 use direct parentIndex lookup rather than ParentResolver). Ensure this simplified approach is correct for Axis2 contexts where the parent relationship might be established differently.

Additionally, the custom span tags logic at lines 365-371 has the same inconsistent key handling issue as the Synapse version (see comment on lines 252-279).


380-380: Clarification: This is NOT a redundant call.

A past review comment flagged this as redundant, but line 380 calls statisticDataUnit.setTransportHeaderMap(headersMap) while line 352 calls msgCtx.setProperty(...). These set headers on different objects and both are necessary.


415-420: LGTM! Enhanced outer level span detection.

The addition of statisticDataUnit.isOuterLayerSpan() check (line 418) and the MAIN_SEQUENCE check (lines 419-420) properly extends outer level span detection to cover more scenarios.


438-442: LGTM! Axis2 close event handlers.

The new public Axis2 overloads for handleCloseEntryEvent and handleCloseEntryWithErrorEvent properly delegate to handleCloseEvent with appropriate error flags.

Also applies to: 450-454


467-467: LGTM! NPE protection added.

The null check for spanWrapper has been added, addressing the past review comment about potential NullPointerException in the parent span traversal.


501-509: LGTM! Consistent handleCloseEvent implementation.

The Axis2 handleCloseEvent overload follows the same pattern as the Synapse version, properly synchronizing on the span store and delegating to finishSpan.

Comment on lines +567 to +584
private void finishSpan(BasicStatisticDataUnit basicStatisticDataUnit,
org.apache.axis2.context.MessageContext msgCtx,
SpanStore spanStore,
TracingScope tracingScope, boolean isError) {
String spanWrapperId = TracingUtils.extractId(basicStatisticDataUnit);
SpanWrapper spanWrapper = spanStore.getSpanWrapper(spanWrapperId);
//Set the statistic data unit of the close event into the span wrapper
if (spanWrapper != null && (basicStatisticDataUnit instanceof StatisticDataUnit)) {
spanWrapper.setCloseEventStatisticDataUnit((StatisticDataUnit) basicStatisticDataUnit);
}
if (!Objects.equals(spanWrapper, spanStore.getOuterLevelSpanWrapper())) {
spanStore.finishSpan(spanWrapper, msgCtx, isError);
} else {
spanStore.finishSpan(spanWrapper, msgCtx, isError);
tracingScopeManager.cleanupTracingScope(tracingScope.getTracingScopeId());

}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the SpanHandler.java file and examine it for isEventCollectionFinished and finishSpan
find . -type f -name "SpanHandler.java" | grep -i opentelemetry

Repository: wso2/wso2-synapse

Length of output: 196


🏁 Script executed:

#!/bin/bash
# Read the SpanHandler.java file to examine the finishSpan method and isEventCollectionFinished
wc -l ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 200


🏁 Script executed:

#!/bin/bash
# Read the finishSpan method around lines 567-584
sed -n '550,600p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 2753


🏁 Script executed:

#!/bin/bash
# Search for isEventCollectionFinished implementation to understand its purpose
rg -A 10 "isEventCollectionFinished" ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/

Repository: wso2/wso2-synapse

Length of output: 6147


🏁 Script executed:

#!/bin/bash
# Get the exact line numbers for both finishSpan methods
grep -n "private void finishSpan" ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 418


🏁 Script executed:

#!/bin/bash
# Read all three finishSpan methods to understand their differences
sed -n '536,606p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 3825


🏁 Script executed:

#!/bin/bash
# Let me check if there are any comments or documentation about the difference between these two methods
sed -n '500,580p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java | head -100

Repository: wso2/wso2-synapse

Length of output: 4600


🏁 Script executed:

#!/bin/bash
# Check what methods are calling these two different finishSpan methods to understand usage patterns
grep -n "finishSpan(basicStatisticDataUnit" ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 380


🏁 Script executed:

#!/bin/bash
# Let me check the method signatures more carefully - check lines around 543 and 567
sed -n '543,550p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java
echo "---"
sed -n '567,574p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 1237


🏁 Script executed:

#!/bin/bash
# Let me trace the calls to understand which method is used in which context
grep -B 5 "finishSpan(basicStatisticDataUnit, synCtx," ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java | head -20
echo "---"
grep -B 5 "finishSpan(basicStatisticDataUnit, msgCtx," ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java | head -20

Repository: wso2/wso2-synapse

Length of output: 1397


🏁 Script executed:

#!/bin/bash
# Verify the exact parameter types for both finishSpan methods to confirm which is which
sed -n '543,575p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java | grep -E "(private void finishSpan|MessageContext|import)"

Repository: wso2/wso2-synapse

Length of output: 337


🏁 Script executed:

#!/bin/bash
# Check the imports to understand which MessageContext types are being used
head -50 ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java | grep -E "^import.*MessageContext"

Repository: wso2/wso2-synapse

Length of output: 102


🏁 Script executed:

#!/bin/bash
# Let me confirm the exact line range for the Axis2 finishSpan method
sed -n '567,585p' ./modules/core/src/main/java/org/apache/synapse/aspects/flow/statistics/tracing/opentelemetry/management/handling/span/SpanHandler.java

Repository: wso2/wso2-synapse

Length of output: 1101


Inconsistent outer level span finishing behavior between Axis2 and Synapse contexts.

The Synapse version (lines 543-566) conditionally finishes the outer level span only after verifying event collection is complete via isEventCollectionFinished(), whereas the Axis2 version (lines 567-584) unconditionally finishes and cleans up the tracing scope immediately. This difference could cause premature span closure in Axis2 contexts when async callbacks or operations are still pending, as the isEventCollectionFinished() check ensures all callbacks have completed and pending operations are settled before span closure.

@KalinduGandara KalinduGandara merged commit 0a996db into wso2:master Dec 22, 2025
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding OpenTelemetry support on Dataservices

2 participants