Skip to content

Conversation

@KalinduGandara
Copy link
Contributor

@KalinduGandara KalinduGandara commented Dec 2, 2025

fixes: #4496

Summary by CodeRabbit

  • New Features
    • Integrated OpenTelemetry distributed tracing across data service request handling and query execution pipelines
    • Added configuration support for filtered mediator names and custom span header tags
    • Enhanced OData endpoint request/response tracking with improved error flow tracing
    • Enabled per-request tracing for batch and boxcarring operations

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

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

This pull request introduces OpenTelemetry tracing support to WSO2 Data Services by adding a centralized tracing collector and instrumenting key message receivers, request processors, and OData handlers with entry/exit tracing events. It updates method signatures in request dispatch classes to accept MessageContext parameters for context propagation.

Changes

Cohort / File(s) Summary
OpenTelemetry Infrastructure
org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java, DataServicesTracingConstants.java
New tracing collector class extending RuntimeStatisticCollector with methods for reporting/closing entry, query execution, OData, and multi-request events; new constants class defining index values and string keys for tracing configuration.
Message Receivers Instrumentation
...dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOnlyMessageReceiver.java, DBInOutMessageReceiver.java
Adds OpenTelemetry tracing lifecycle (reportEntryEvent, closeEntryEvent, closeFlowForcefully) around in-only and in-out message processing with error handling.
Request Dispatch Refactoring
...dispatch/DataServiceRequest.java, BatchDataServiceRequest.java, BoxcarringDataServiceRequest.java, RequestBoxRequest.java, SingleDataServiceRequest.java
Updates public method signatures to accept MessageContext parameter (dispatch(MessageContext), processRequest(MessageContext)) to enable context propagation; adds overloaded dispatch() method for backward compatibility.
Boxcarring & Query Execution
...boxcarring/RequestBox.java, DataServiceProcessor.java
Changes RequestBox.execute() signature to accept MessageContext; adds tracing around query execution with reportQueryExecutionEvent and closeQueryExecutionEvent calls.
OData Support
...odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataEndpoint.java, ODataPassThroughHandler.java, ODataServletRequest.java
Adds OData-specific tracing (reportOdataEntryEvent, closeOdataEntryEvent) in ODataPassThroughHandler; changes getServiceDetails() visibility to public in ODataEndpoint; adds getAxis2MessageContext() accessor in ODataServletRequest.
Mediator Integration
...mediator.dataservice/src/main/java/org/wso2/micro/integrator/mediator/dataservice/DataServiceCallMediator.java
Instruments data service dispatch with reportEntryEvent and closeEntryEvent tracing around request execution and error paths.
Configuration
distribution/src/resources/config-tool/key-mappings.json
Adds two new OpenTelemetry configuration key mappings: opentelemetry.filtered.mediator.names and opentelemetry.custom.span.header.tags.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas requiring extra attention:

  • Public API changes: Multiple method signature updates across dispatch classes (DataServiceRequest.dispatch(MessageContext), processRequest(MessageContext), etc.) require validation of all call sites and impact on existing implementations.
  • MessageContext propagation: Verify that MessageContext is correctly threaded through the entire request flow from entry points through boxcarring, batch processing, and OData handling.
  • DataServicesTracingCollector logic: Review the new 350+ line tracing collector class for correctness in event reporting, property extraction, and error handling paths, especially the closeFlowForcefully and multi-event logic.
  • ODataEndpoint visibility change: Confirm that making getServiceDetails() public is intentional and does not expose internal implementation details; validate external usage patterns.
  • Backward compatibility: Ensure the overloaded dispatch() method correctly delegates to dispatch(null) and that all callers using the no-arg version continue to work.

Poem

🐰 Traces now flow through our data streams so fine,
OpenTelemetry marks each context line,
From inbox to query, we're tracing it all,
Events dispatched and collected, standing tall!
Observability blooms where data once flew,
A rabbit's delight—transparent and true!

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning PR description provides only a link to issue #4496 but is missing required template sections like Purpose, Goals, Approach, User Stories, Release Notes, Documentation, Testing details, and others. Complete the PR description using the provided template to include Purpose, Goals, Approach, release notes, documentation impact, test coverage, and other required sections.
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title comprehensively summarizes the main changes: adding trace filter support, custom headers, and OpenTelemetry integration to dataservices.
Linked Issues check ✅ Passed Code changes successfully implement OpenTelemetry tracing across dataservices components by adding DataServicesTracingCollector, constants, and instrumentation calls, addressing the objective stated in issue #4496.
Out of Scope Changes check ✅ Passed All changes are within scope: OpenTelemetry tracing integration to dataservices, new constants/collectors, method signature updates, and configuration mappings all align with the stated objective of adding OpenTelemetry support to dataservices.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

", Operation - " + msgContext.getSoapAction() + ", Request body - " +
msgContext.getEnvelope().getText() + ", ThreadID - " + Thread.currentThread().getId());
}
DataServicesTracingCollector.reportEntryEvent(msgContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 1

Suggested change
DataServicesTracingCollector.reportEntryEvent(msgContext);
DataServicesTracingCollector.reportEntryEvent(msgContext);
log.info("Processing in-only message for Data Service: " + msgContext.getServiceContext().getName() + ", Operation: " + msgContext.getSoapAction());

DataServicesTracingCollector.reportEntryEvent(msgContext);
DataServiceProcessor.dispatch(msgContext);
msgContext.setProperty(DBConstants.TENANT_IN_ONLY_MESSAGE, Boolean.TRUE);
DataServicesTracingCollector.closeEntryEvent(msgContext, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 2

Suggested change
DataServicesTracingCollector.closeEntryEvent(msgContext, null);
DataServicesTracingCollector.closeEntryEvent(msgContext, null);
log.info("Successfully completed in-only message processing for Data Service: " + msgContext.getServiceContext().getName());

Comment on lines 95 to +98
newMsgContext.setProperty(Constants.Configuration.MESSAGE_TYPE,
HTTPConstants.MEDIA_TYPE_APPLICATION_JSON);
}
DataServicesTracingCollector.closeEntryEvent(msgContext, result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 4

Suggested change
newMsgContext.setProperty(Constants.Configuration.MESSAGE_TYPE,
HTTPConstants.MEDIA_TYPE_APPLICATION_JSON);
}
DataServicesTracingCollector.closeEntryEvent(msgContext, result);
newMsgContext.setProperty(Constants.Configuration.MESSAGE_TYPE,
HTTPConstants.MEDIA_TYPE_APPLICATION_JSON);
}
log.info("Data service request completed successfully - MessageID: " + msgContext.getMessageID());

Comment on lines 40 to +41
DataServiceRequest request = DataServiceRequest.createDataServiceRequest(msgContext);
OMElement result = request.dispatch();
DataServicesTracingCollector.reportQueryExecutionEvent(msgContext, request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 5

Suggested change
DataServiceRequest request = DataServiceRequest.createDataServiceRequest(msgContext);
OMElement result = request.dispatch();
DataServicesTracingCollector.reportQueryExecutionEvent(msgContext, request);
DataServiceRequest request = DataServiceRequest.createDataServiceRequest(msgContext);
log.info("Processing data service request: " + request.getRequestName());
DataServicesTracingCollector.reportQueryExecutionEvent(msgContext, request);

Comment on lines +63 to 64
public synchronized OMElement execute(MessageContext messageContext) throws DataServiceFault {
OMElement result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 7

Suggested change
public synchronized OMElement execute(MessageContext messageContext) throws DataServiceFault {
OMElement result;
public synchronized OMElement execute(MessageContext messageContext) throws DataServiceFault {
log.info("Starting boxcarring session execution with " + this.getRequests().size() + " requests");
OMElement result;

Comment on lines 68 to +69
for (int i = 0; i < n; i++) {
result = reqList.get(i).dispatch();
DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 8

Suggested change
for (int i = 0; i < n; i++) {
result = reqList.get(i).dispatch();
DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));
for (int i = 0; i < n; i++) {
if (log.isDebugEnabled()) {
log.debug("Executing request " + (i + 1) + " of " + n + " in boxcarring session");
}
DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));

Comment on lines 66 to 70
if (!this.getDataService().isInDTX()) {
this.getDataService().getDSSTxManager().begin();
}
OMElement lastRequestResult = DSSessionManager.getCurrentRequestBox().execute();
OMElement lastRequestResult = DSSessionManager.getCurrentRequestBox().execute(messageContext);
error = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 12

Suggested change
if (!this.getDataService().isInDTX()) {
this.getDataService().getDSSTxManager().begin();
}
OMElement lastRequestResult = DSSessionManager.getCurrentRequestBox().execute();
OMElement lastRequestResult = DSSessionManager.getCurrentRequestBox().execute(messageContext);
error = false;
if (!this.getDataService().isInDTX()) {
this.getDataService().getDSSTxManager().begin();
}
if (log.isDebugEnabled()) {
log.debug("Executing boxcarring requests for session");
}
OMElement lastRequestResult = DSSessionManager.getCurrentRequestBox().execute(messageContext);
error = false;
log.info("Successfully executed boxcarring session");

Comment on lines 359 to +360
*/
public OMElement dispatch() throws DataServiceFault {
public OMElement dispatch(MessageContext messageContext) throws DataServiceFault {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 13

Suggested change
*/
public OMElement dispatch() throws DataServiceFault {
public OMElement dispatch(MessageContext messageContext) throws DataServiceFault {
public OMElement dispatch(MessageContext messageContext) throws DataServiceFault {
log.debug("Dispatching data service request: " + this.requestName);
/* set user */

Comment on lines +369 to 370
OMElement result = this.processRequest(messageContext);
/* check disable streaming */
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 14

Suggested change
OMElement result = this.processRequest(messageContext);
/* check disable streaming */
OMElement result = this.processRequest(messageContext);
if (log.isDebugEnabled()) {
log.debug("Request processed successfully for: " + this.requestName);
}
/* check disable streaming */

public OMElement processRequest(MessageContext messageContext) throws DataServiceFault {

boolean error = true;
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 15

Suggested change
try {
DispatchStatus.setBoxcarringRequest();
log.info("Processing request box with " + this.requestBox.getRequestCount() + " requests");

this.getDataService().getDSSTxManager().begin();
}
OMElement lastRequestResult = this.requestBox.execute();
OMElement lastRequestResult = this.requestBox.execute(messageContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 16

Suggested change
OMElement lastRequestResult = this.requestBox.execute(messageContext);
OMElement lastRequestResult = this.requestBox.execute(messageContext);
log.debug("Request box execution completed successfully");

Comment on lines 61 to +62
@Override
public OMElement processRequest() throws DataServiceFault {
public OMElement processRequest(MessageContext messageContext) throws DataServiceFault {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 17

Suggested change
@Override
public OMElement processRequest() throws DataServiceFault {
public OMElement processRequest(MessageContext messageContext) throws DataServiceFault {
@Override
public OMElement processRequest(MessageContext messageContext) throws DataServiceFault {
log.info("Processing single data service request");

Comment on lines +61 to +64
* Report entry event for data service invocation.
* @param messageContext Axis2 message context
*/
public static void reportEntryEvent(MessageContext messageContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 18

Suggested change
* Report entry event for data service invocation.
* @param messageContext Axis2 message context
*/
public static void reportEntryEvent(MessageContext messageContext) {
public static void reportEntryEvent(MessageContext messageContext) {
if (isOpenTelemetryEnabled() && messageContext != null) {
String componentName = messageContext.getAxisService().getName();
log.info("Starting data service invocation for service: " + componentName);

Comment on lines +356 to +359
private static void setPayload(OMElement payload, StatisticDataUnit dataUnit) {
try {
if (payload != null) {
dataUnit.setPayload(payload.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 19

Suggested change
private static void setPayload(OMElement payload, StatisticDataUnit dataUnit) {
try {
if (payload != null) {
dataUnit.setPayload(payload.toString());
} catch (Exception e) {
// We are catching exception to avoid any issues with toString() and
// We should not fail the main flow due to tracing issues
log.error("Error while setting payload for tracing span: " + e.getMessage());

Comment on lines 106 to 108
try {
DataServicesTracingCollector.reportOdataQueryExecutionEvent(request.getAxis2MessageContext());
oDataServiceHandler.process(request, response, serviceRootPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 20

Suggested change
try {
DataServicesTracingCollector.reportOdataQueryExecutionEvent(request.getAxis2MessageContext());
oDataServiceHandler.process(request, response, serviceRootPath);
try {
log.info("Processing OData request for service: " + request.getRawODataPath());
DataServicesTracingCollector.reportOdataQueryExecutionEvent(request.getAxis2MessageContext());
oDataServiceHandler.process(request, response, serviceRootPath);

Comment on lines 110 to 113
} catch (Exception e) {
if (log.isDebugEnabled()) {
log.debug("Failed to process the servlet request. " + e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 21

Suggested change
} catch (Exception e) {
if (log.isDebugEnabled()) {
log.debug("Failed to process the servlet request. " + e);
}
} catch (Exception e) {
log.error("Failed to process OData request: " + e.getMessage());
if (log.isDebugEnabled()) {
log.debug("Failed to process the servlet request. " + e);
}

Comment on lines 52 to +54
if (axis2MessageContext.getProperty(TRANSPORT_IN_URL) != null && isODataService != null) {
// Marking the message to skip the main sequence in Synapse Axis2 Environment.
messageContext.setProperty(SynapseConstants.SKIP_MAIN_SEQUENCE, Boolean.TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 22

Suggested change
if (axis2MessageContext.getProperty(TRANSPORT_IN_URL) != null && isODataService != null) {
// Marking the message to skip the main sequence in Synapse Axis2 Environment.
messageContext.setProperty(SynapseConstants.SKIP_MAIN_SEQUENCE, Boolean.TRUE);
if (axis2MessageContext.getProperty(TRANSPORT_IN_URL) != null && isODataService != null) {
// Marking the message to skip the main sequence in Synapse Axis2 Environment.
messageContext.setProperty(SynapseConstants.SKIP_MAIN_SEQUENCE, Boolean.TRUE);
LOG.info("Processing OData service request for URL: " + axis2MessageContext.getProperty(TRANSPORT_IN_URL));

Comment on lines 63 to +65
streamResponseBack(response, messageContext, axis2MessageContext);
}
DataServicesTracingCollector.closeOdataEntryEvent(axis2MessageContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 23

Suggested change
streamResponseBack(response, messageContext, axis2MessageContext);
}
DataServicesTracingCollector.closeOdataEntryEvent(axis2MessageContext,
}
DataServicesTracingCollector.closeOdataEntryEvent(axis2MessageContext,
axis2MessageContext.getEnvelope());
LOG.debug("Successfully processed OData service request");

Comment on lines +445 to +447
public MessageContext getAxis2MessageContext() {
return axis2MessageContext;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 24

Suggested change
public MessageContext getAxis2MessageContext() {
return axis2MessageContext;
}
public MessageContext getAxis2MessageContext() {
log.debug("Retrieving Axis2 message context");
return axis2MessageContext;
}

Comment on lines 210 to +211
try {
DataServicesTracingCollector.reportEntryEvent(axis2MessageContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 25

Suggested change
try {
DataServicesTracingCollector.reportEntryEvent(axis2MessageContext);
try {
log.info("Starting data service dispatch for operation: " + axis2MessageContext.getOperationContext().getOperationName());
DataServicesTracingCollector.reportEntryEvent(axis2MessageContext);

Comment on lines 246 to +247
} catch (DataServiceFault dataServiceFault) {
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, dataServiceFault);
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Improvement Suggestion No: 26

Suggested change
} catch (DataServiceFault dataServiceFault) {
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, dataServiceFault);
} catch (DataServiceFault dataServiceFault) {
log.error("Data service fault occurred: " + dataServiceFault.getMessage());
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, dataServiceFault);

Copy link
Contributor

@wso2-engineering wso2-engineering bot left a comment

Choose a reason for hiding this comment

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

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

  • The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
  • Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.

✅ Before merging this pull request:

  • Review all AI-generated comments for accuracy and relevance.
  • Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Comment Accepted (Y/N) Reason
#### Log Improvement Suggestion No: 1
#### Log Improvement Suggestion No: 2
#### Log Improvement Suggestion No: 4
#### Log Improvement Suggestion No: 5
#### Log Improvement Suggestion No: 7
#### Log Improvement Suggestion No: 8
#### Log Improvement Suggestion No: 12
#### Log Improvement Suggestion No: 13
#### Log Improvement Suggestion No: 14
#### Log Improvement Suggestion No: 15
#### Log Improvement Suggestion No: 16
#### Log Improvement Suggestion No: 17
#### Log Improvement Suggestion No: 18
#### Log Improvement Suggestion No: 19
#### Log Improvement Suggestion No: 20
#### Log Improvement Suggestion No: 21
#### Log Improvement Suggestion No: 22
#### Log Improvement Suggestion No: 23
#### Log Improvement Suggestion No: 24
#### Log Improvement Suggestion No: 25
#### Log Improvement Suggestion No: 26

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: 6

🧹 Nitpick comments (4)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOnlyMessageReceiver.java (1)

48-71: Tracing lifecycle is wired correctly; consider minor @OverRide annotation improvement

The try/catch/finally now cleanly wraps the whole flow: entry event before dispatch, normal close on success, and closeFlowForcefully on any exception, while preserving the original logging, fault-name setting, and AxisFault wrapping. Semantics of the in-only receiver look unchanged apart from tracing.

As a small improvement, you could add @Override above invokeBusinessLogic to make the override explicit and catch signature drift at compile time.

-    public void invokeBusinessLogic(MessageContext msgContext) throws AxisFault {
+    @Override
+    public void invokeBusinessLogic(MessageContext msgContext) throws AxisFault {
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/RequestBoxRequest.java (1)

27-27: Unused import.

DataServicesTracingCollector is imported but not used in this file.

-import org.wso2.micro.integrator.dataservices.core.opentelemetry.DataServicesTracingCollector;
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/DataServiceRequest.java (1)

393-397: Document nullable parameter in Javadoc.

The messageContext parameter can be null (as shown by dispatch() calling dispatch(null)). Consider updating the Javadoc to explicitly document this:

 	/**
 	 * This method must implement the request specific request processing logic.
-     * @param messageContext Axis2 message context of the request
+     * @param messageContext Axis2 message context of the request, may be null if tracing is not required
 	 * @return The result of the request invocation
 	 * @throws DataServiceFault
      */
     public abstract OMElement processRequest(MessageContext messageContext) throws DataServiceFault;
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingConstants.java (1)

21-37: Consider making this a utility/constants class.

Constants-only classes are typically made final with a private constructor to prevent instantiation and inheritance, following Java best practices.

-public class DataServicesTracingConstants {
+public final class DataServicesTracingConstants {
+
+    private DataServicesTracingConstants() {
+        // Prevent instantiation
+    }
+
     public static final int DEFAULT_PARENT_INDEX = -1;
     public static final int DATA_SERVICE_INDEX = 0;

This follows the same pattern used in DBConstants.FaultCodes shown in the relevant snippets.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c64468 and 686a52a.

📒 Files selected for processing (16)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOnlyMessageReceiver.java (2 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOutMessageReceiver.java (3 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DataServiceProcessor.java (3 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/boxcarring/RequestBox.java (2 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/BatchDataServiceRequest.java (3 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/BoxcarringDataServiceRequest.java (3 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/DataServiceRequest.java (4 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/RequestBoxRequest.java (2 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/SingleDataServiceRequest.java (2 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (1 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingConstants.java (1 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataEndpoint.java (3 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataPassThroughHandler.java (2 hunks)
  • components/data/data-services/org.wso2.micro.integrator.dataservices.odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataServletRequest.java (1 hunks)
  • components/mediation/mediators/dataservices-mediator/org.wso2.micro.integrator.mediator.dataservice/src/main/java/org/wso2/micro/integrator/mediator/dataservice/DataServiceCallMediator.java (4 hunks)
  • distribution/src/resources/config-tool/key-mappings.json (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOutMessageReceiver.java (2)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (1)
  • DataServicesTracingCollector (57-370)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingConstants.java (1)
  • DataServicesTracingConstants (21-37)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/boxcarring/RequestBox.java (3)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/DataServiceRequest.java (1)
  • DataServiceRequest (48-398)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (1)
  • DataServicesTracingCollector (57-370)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingConstants.java (1)
  • DataServicesTracingConstants (21-37)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DataServiceProcessor.java (1)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (1)
  • DataServicesTracingCollector (57-370)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/RequestBoxRequest.java (2)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/boxcarring/RequestBox.java (1)
  • RequestBox (38-96)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (1)
  • DataServicesTracingCollector (57-370)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (2)
components/data/data-services/org.wso2.micro.integrator.dataservices.common/src/main/java/org/wso2/micro/integrator/dataservices/common/DBConstants.java (2)
  • DBConstants (27-1108)
  • FaultCodes (92-103)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingConstants.java (1)
  • DataServicesTracingConstants (21-37)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOnlyMessageReceiver.java (2)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (1)
  • DataServicesTracingCollector (57-370)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingConstants.java (1)
  • DataServicesTracingConstants (21-37)
🔇 Additional comments (18)
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOnlyMessageReceiver.java (1)

27-29: Tracing imports look consistent with the new OpenTelemetry infrastructure

Static import of DATA_SERVICE_INDEX and the addition of DataServicesTracingCollector match the patterns used elsewhere in the PR and keep the call sites readable. No issues here.

components/mediation/mediators/dataservices-mediator/org.wso2.micro.integrator.mediator.dataservice/src/main/java/org/wso2/micro/integrator/mediator/dataservice/DataServiceCallMediator.java (1)

48-48: LGTM!

The imports for OpenTelemetry tracing support are correctly added.

Also applies to: 60-60

distribution/src/resources/config-tool/key-mappings.json (1)

119-120: Naming inconsistency: 'oltp' vs 'opentelemetry' prefix requires verification.

Lines 119-120 map OpenTelemetry properties using the 'oltp' prefix, while the established OpenTelemetry mappings on lines 113-117 use the 'opentelemetry' prefix. This inconsistency warrants verification, as 'oltp' may be a typo (the standard term in OpenTelemetry ecosystem is OTLP, not oltp), or these property names may need to be confirmed against actual Synapse property definitions.

Verify whether the target Synapse property names are:

  • synapse_properties.'opentelemetry.filtered.mediator.names' and synapse_properties.'opentelemetry.custom.span.header.tags' (consistent with lines 113-117), or
  • The 'oltp' prefix is intentional for a different configuration namespace
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/BoxcarringDataServiceRequest.java (1)

24-24: LGTM! Clean integration of MessageContext for tracing support.

The method signature update and parameter threading to DSSessionManager.getCurrentRequestBox().execute(messageContext) correctly propagates the context needed for OpenTelemetry tracing downstream.

Also applies to: 52-55, 69-69

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DataServiceProcessor.java (1)

41-51: LGTM! Tracing correctly wraps the dispatch operation.

The tracing calls appropriately bracket the dispatch operation, and the closeQueryExecutionEvent receives the final result after any request status generation. Exception handling is correctly delegated to the caller (DBInOutMessageReceiver) which calls closeFlowForcefully on errors.

components/data/data-services/org.wso2.micro.integrator.dataservices.odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataServletRequest.java (1)

444-448: LGTM! Simple accessor for tracing context propagation.

The getter enables OData components to access the underlying Axis2 MessageContext for OpenTelemetry tracing purposes, as noted in the AI summary.

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/DBInOutMessageReceiver.java (1)

65-65: LGTM! Proper tracing lifecycle management.

The tracing integration correctly:

  • Reports entry event before business logic processing
  • Closes the entry event on the success path with the result
  • Closes the flow forcefully on the exception path with error details

This ensures spans are properly closed in all execution paths.

Also applies to: 98-98, 102-102

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/BatchDataServiceRequest.java (1)

110-113: Verify method name closerMultiEvent - possible typo.

The method name closerMultiEvent on line 113 appears inconsistent with the naming pattern used elsewhere (e.g., closeEntryEvent, closeQueryExecutionEvent). Confirm whether this should be closeMultiEvent.

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/SingleDataServiceRequest.java (1)

59-62: Method signature updated for interface consistency.

The messageContext parameter is added to align with the updated DataServiceRequest contract but is not used within this implementation. This is acceptable since this class handles single requests that don't require per-request tracing context propagation—the parent dispatch(MessageContext) method in DataServiceRequest already handles the outer tracing context.

components/data/data-services/org.wso2.micro.integrator.dataservices.odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataPassThroughHandler.java (2)

53-66: OpenTelemetry tracing integration for OData requests looks correct.

The tracing lifecycle follows the proper pattern: entry event is reported before processing (line 56), and closed after successful completion (lines 65-66). The SKIP_MAIN_SEQUENCE property ensures OData requests bypass the normal Synapse flow.


69-74: Proper trace finalization on error path.

The closeFlowForcefully call ensures OpenTelemetry spans are properly closed with error details when an exception occurs, preventing orphaned spans.

components/data/data-services/org.wso2.micro.integrator.dataservices.odata.endpoint/src/main/java/org/wso2/micro/integrator/dataservices/odata/endpoint/ODataEndpoint.java (2)

131-131: Visibility change from private to public.

This method is now exposed publicly. Ensure this is intentional and that all callers handle the ODataServiceFault exception appropriately.


105-122: Verify OpenTelemetry context propagation to the spawned thread.

Tracing events are reported inside streamThread, but OpenTelemetry context propagation across thread boundaries requires explicit handling. When Axis2MessageContext is passed to the thread, the OpenTelemetry trace context associated with the incoming request must be made current on the new thread to ensure spans are correctly linked to the parent.

Confirm that either:

  1. The framework (or OpenTelemetryManagerHolder) automatically propagates and activates context when request.getAxis2MessageContext() is passed to the thread, or
  2. The tracing implementation explicitly calls context.makeCurrent() or equivalent within the thread to activate the extracted context for downstream span creation.

Without this, spans created in streamThread may not be properly correlated with the parent trace.

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/RequestBoxRequest.java (1)

59-70: Method signature and context propagation updated correctly.

The MessageContext is properly propagated to requestBox.execute(), enabling the tracing instrumentation added in RequestBox.java.

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java (3)

64-85: LGTM! Well-structured entry event reporting.

The method properly checks isOpenTelemetryEnabled() and null messageContext before proceeding, sets up the StatisticDataUnit with appropriate properties, and uses helper methods for HTTP properties. Good defensive approach with setPayload wrapped in try-catch.


264-288: Good error handling in closeFlowForcefully.

The method appropriately handles both DataServiceFault and generic exceptions, extracting error codes and messages accordingly. The adjustment for multi-request indices is well-documented.


356-369: Good defensive coding in setPayload.

The try-catch block prevents tracing failures from impacting the main flow. Setting a fallback payload and error message provides useful debugging information while maintaining resilience.

components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/dispatch/DataServiceRequest.java (1)

360-389: LGTM! Good backward-compatible design for MessageContext propagation.

The overloaded dispatch() method that delegates to dispatch(null) maintains backward compatibility while enabling tracing support. This allows existing callers to continue working without modification.

However, verification of null-handling in all processRequest implementations could not be completed due to repository access limitations. The design pattern shown in the provided code snippet appears sound, but complete validation requires examining all implementing classes to ensure they properly handle the nullable MessageContext parameter.

Comment on lines +63 to +72
public synchronized OMElement execute(MessageContext messageContext) throws DataServiceFault {
OMElement result;
List<DataServiceRequest> reqList = this.getRequests();
int n = reqList.size();
OMElement resultElement = null;
for (int i = 0; i < n; i++) {
result = reqList.get(i).dispatch();
DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));
messageContext.setProperty(MULTI_REQUEST_LAST_INDEX_PROPERTY, i);
result = reqList.get(i).dispatch(messageContext);
DataServicesTracingCollector.closerMultiEvent(messageContext, i, result);
Copy link

@coderabbitai coderabbitai bot Dec 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Potential NullPointerException if messageContext is null.

Line 70 calls messageContext.setProperty(...) without a null check. While DataServicesTracingCollector methods internally check for null, this direct property access will throw NPE if messageContext is null. The DataServiceRequest.dispatch() no-arg method calls dispatch(null), making this a possible code path.

Consider adding a null guard:

 for (int i = 0; i < n; i++) {
     DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));
-    messageContext.setProperty(MULTI_REQUEST_LAST_INDEX_PROPERTY, i);
+    if (messageContext != null) {
+        messageContext.setProperty(MULTI_REQUEST_LAST_INDEX_PROPERTY, i);
+    }
     result = reqList.get(i).dispatch(messageContext);
     DataServicesTracingCollector.closerMultiEvent(messageContext, i, result);
📝 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
public synchronized OMElement execute(MessageContext messageContext) throws DataServiceFault {
OMElement result;
List<DataServiceRequest> reqList = this.getRequests();
int n = reqList.size();
OMElement resultElement = null;
for (int i = 0; i < n; i++) {
result = reqList.get(i).dispatch();
DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));
messageContext.setProperty(MULTI_REQUEST_LAST_INDEX_PROPERTY, i);
result = reqList.get(i).dispatch(messageContext);
DataServicesTracingCollector.closerMultiEvent(messageContext, i, result);
public synchronized OMElement execute(MessageContext messageContext) throws DataServiceFault {
OMElement result;
List<DataServiceRequest> reqList = this.getRequests();
int n = reqList.size();
OMElement resultElement = null;
for (int i = 0; i < n; i++) {
DataServicesTracingCollector.reportMultiEvent(messageContext, i, reqList.get(i));
if (messageContext != null) {
messageContext.setProperty(MULTI_REQUEST_LAST_INDEX_PROPERTY, i);
}
result = reqList.get(i).dispatch(messageContext);
DataServicesTracingCollector.closerMultiEvent(messageContext, i, result);
🤖 Prompt for AI Agents
In
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/boxcarring/RequestBox.java
around lines 63 to 72, the code calls messageContext.setProperty(...) and
DataServicesTracingCollector methods without checking if messageContext is null
which can cause a NullPointerException (dispatch may be called with null). Add a
null guard: if messageContext is non-null, perform the tracing/reporting and
setProperty calls as written; otherwise skip those calls and invoke
reqList.get(i).dispatch(null) (or dispatch with the existing null) so dispatch
still runs but no property access or tracing occurs on a null messageContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check this suggestion

Copy link

Choose a reason for hiding this comment

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

@AmilaSamith, I've reviewed the suggestion. The concern is valid based on the code flow:

Tracing the null path:

  1. In DataServiceRequest.java, the no-arg dispatch() method delegates to dispatch(null) (lines visible in the AI summary)
  2. This null MessageContext is then passed to processRequest(messageContext)
  3. For BoxcarringDataServiceRequest, processRequest() calls execute(messageContext) in RequestBox
  4. At line 70 in RequestBox.execute(), messageContext.setProperty(...) is called without a null check

Result: If the no-arg dispatch() is called anywhere in the codebase, it will cause a NullPointerException at line 70.

The suggested null guard is appropriate since:

  • DataServicesTracingCollector methods already handle null messageContext internally (based on the pattern)
  • The setProperty call is purely for tracing infrastructure and shouldn't break the core boxcarring flow
  • The dispatch(messageContext) call on line 71 should work fine with null (delegates back to no-arg version)

Would you like to apply this fix, or is there a guarantee that execute() will never receive a null messageContext in your deployment?

Comment on lines +253 to +255
public static void closerMultiEvent(MessageContext messageContext, int currentIndex, OMElement result) {
handleCloseEvents(messageContext, result, currentIndex + MULTI_REQUEST_BASE_INDEX);
}
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

Typo in method name: closerMultiEvent should be closeMultiEvent.

This appears to be a typo. All other close methods follow the pattern close*Event.

-    public static void closerMultiEvent(MessageContext messageContext, int currentIndex, OMElement result) {
+    public static void closeMultiEvent(MessageContext messageContext, int currentIndex, OMElement result) {
         handleCloseEvents(messageContext, result, currentIndex + MULTI_REQUEST_BASE_INDEX);
     }
🤖 Prompt for AI Agents
In
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java
around lines 253-255, the method name closerMultiEvent is a typo and should be
renamed to closeMultiEvent; change the method declaration to public static void
closeMultiEvent(MessageContext messageContext, int currentIndex, OMElement
result) and update all internal references/usages (and any tests or javadocs) to
call closeMultiEvent instead of closerMultiEvent to keep naming consistent.

Comment on lines +302 to +322
private static String[] getOdataServiceDetails(MessageContext messageContext) {
String odataServiceName;
String odataServiceUri;
String configID;
Object transportInURL = messageContext.getProperty(TRANSPORT_IN_URL);
String uri = transportInURL != null ? transportInURL.toString() : "";
int index = uri.indexOf(ODATA_SERVICE);
if (-1 != index) {
int serviceStart = index + ODATA_SERVICE.length();
if (uri.length() > serviceStart + 1) {
odataServiceUri = uri.substring(serviceStart);
if (-1 != odataServiceUri.indexOf(URL_SEPARATOR_CHAR)) {
String[] params = odataServiceUri.split(URL_SEPARATOR);
odataServiceName = params[0];
configID = params[1];
return new String[] {odataServiceName, configID};
}
}
}
return new String[] {DEFAULT_ODATA_SERVICE_NAME, null};
}
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

Potential ArrayIndexOutOfBoundsException when parsing OData service URI.

If the URI contains only one segment after splitting (e.g., odata/serviceName without a config ID), accessing params[1] at Line 316 will throw an exception.

     private static String[] getOdataServiceDetails(MessageContext messageContext) {
         String odataServiceName;
         String odataServiceUri;
         String configID;
         Object transportInURL = messageContext.getProperty(TRANSPORT_IN_URL);
         String uri = transportInURL != null ? transportInURL.toString() : "";
         int index = uri.indexOf(ODATA_SERVICE);
         if (-1 != index) {
             int serviceStart = index + ODATA_SERVICE.length();
             if (uri.length() > serviceStart + 1) {
                 odataServiceUri = uri.substring(serviceStart);
                 if (-1 != odataServiceUri.indexOf(URL_SEPARATOR_CHAR)) {
                     String[] params = odataServiceUri.split(URL_SEPARATOR);
-                    odataServiceName = params[0];
-                    configID = params[1];
-                    return new String[] {odataServiceName, configID};
+                    if (params.length >= 2) {
+                        odataServiceName = params[0];
+                        configID = params[1];
+                        return new String[] {odataServiceName, configID};
+                    } else if (params.length == 1) {
+                        return new String[] {params[0], null};
+                    }
                 }
             }
         }
         return new String[] {DEFAULT_ODATA_SERVICE_NAME, null};
     }
🤖 Prompt for AI Agents
In
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java
around lines 302 to 322, the code assumes params[1] exists after splitting the
OData URI which can throw ArrayIndexOutOfBoundsException; change the parsing to
validate params length (or filter out empty segments) before accessing
params[1], assign configID = null if there is no second segment (and still set
odataServiceName = params[0] when present), and return appropriately so a
missing config ID is handled safely instead of throwing.

Comment on lines +337 to +354
private static Map<String, Object> getDataServiceProperties(DataServiceRequest request) {
DataService dataService = request.getDataService();
String requestName = request.getRequestName();
CallQuery callQuery = dataService.getCallableRequest(requestName).getCallQuery();
Map<String, Object> customProperties = new HashMap<>();
if (callQuery != null) {
Query query = callQuery.getQuery();
String queryId = query.getQueryId();
String configId = query.getConfigId();
if (queryId != null) {
customProperties.put(DB_QUERY_ID, queryId);
}
if (configId != null) {
customProperties.put(DB_CONFIG_ID, configId);
}
}
return customProperties;
}
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

Potential NullPointerException if getCallableRequest returns null or getCallQuery returns null.

The code assumes getCallableRequest(requestName) returns a non-null value and that getCallQuery() is also non-null. If either is null, an NPE will occur.

     private static Map<String, Object> getDataServiceProperties(DataServiceRequest request) {
         DataService dataService = request.getDataService();
         String requestName = request.getRequestName();
-        CallQuery callQuery = dataService.getCallableRequest(requestName).getCallQuery();
         Map<String, Object> customProperties = new HashMap<>();
-        if (callQuery != null) {
+        var callableRequest = dataService.getCallableRequest(requestName);
+        if (callableRequest != null) {
+            CallQuery callQuery = callableRequest.getCallQuery();
+            if (callQuery == null) {
+                return customProperties;
+            }
             Query query = callQuery.getQuery();
             String queryId = query.getQueryId();
             String configId = query.getConfigId();
             if (queryId != null) {
                 customProperties.put(DB_QUERY_ID, queryId);
             }
             if (configId != null) {
                 customProperties.put(DB_CONFIG_ID, configId);
             }
         }
         return customProperties;
     }
📝 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
private static Map<String, Object> getDataServiceProperties(DataServiceRequest request) {
DataService dataService = request.getDataService();
String requestName = request.getRequestName();
CallQuery callQuery = dataService.getCallableRequest(requestName).getCallQuery();
Map<String, Object> customProperties = new HashMap<>();
if (callQuery != null) {
Query query = callQuery.getQuery();
String queryId = query.getQueryId();
String configId = query.getConfigId();
if (queryId != null) {
customProperties.put(DB_QUERY_ID, queryId);
}
if (configId != null) {
customProperties.put(DB_CONFIG_ID, configId);
}
}
return customProperties;
}
private static Map<String, Object> getDataServiceProperties(DataServiceRequest request) {
DataService dataService = request.getDataService();
String requestName = request.getRequestName();
Map<String, Object> customProperties = new HashMap<>();
var callableRequest = dataService.getCallableRequest(requestName);
if (callableRequest != null) {
CallQuery callQuery = callableRequest.getCallQuery();
if (callQuery == null) {
return customProperties;
}
Query query = callQuery.getQuery();
String queryId = query.getQueryId();
String configId = query.getConfigId();
if (queryId != null) {
customProperties.put(DB_QUERY_ID, queryId);
}
if (configId != null) {
customProperties.put(DB_CONFIG_ID, configId);
}
}
return customProperties;
}
🤖 Prompt for AI Agents
In
components/data/data-services/org.wso2.micro.integrator.dataservices.core/src/main/java/org/wso2/micro/integrator/dataservices/core/opentelemetry/DataServicesTracingCollector.java
around lines 337 to 354, the code dereferences
request.getDataService().getCallableRequest(requestName) and then getCallQuery()
without null checks which can cause NPEs; guard against null by first retrieving
the callableRequest into a local variable, verify callableRequest != null before
calling getCallQuery(), then verify callQuery != null (and Query != null) before
accessing queryId/configId, and if any are null simply skip adding those
properties (optionally log a debug/warn) and return the customProperties map.

Comment on lines +211 to +213
DataServicesTracingCollector.reportEntryEvent(axis2MessageContext);
OMElement omElement = DataServiceProcessor.dispatch(axis2MessageContext);
DataServicesTracingCollector.closeEntryEvent(axis2MessageContext, omElement);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Tracing instrumentation looks good, but exception handling is incomplete.

The entry/exit tracing pattern around the dispatch call is implemented correctly. However, there's a critical gap in exception handling: if an AxisFault is thrown after reportEntryEvent is called (line 211), the tracing flow won't be closed because the AxisFault catch block (lines 255-260) doesn't call closeFlowForcefully.

This issue is addressed in the next comment on line 247.

🤖 Prompt for AI Agents
In
components/mediation/mediators/dataservices-mediator/org.wso2/micro/integrator/mediator/dataservice/DataServiceCallMediator.java
around lines 211-213 (and the AxisFault catch at 255-260), ensure tracing is
always closed when reportEntryEvent has been called: either wrap the dispatch
call and subsequent closeEntryEvent in a try/finally so closeEntryEvent or
closeFlowForcefully runs on all exits, or explicitly invoke
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext) inside the
AxisFault catch (and any other exception paths) before rethrowing/handling the
fault; keep the existing closeEntryEvent for normal flow and use
closeFlowForcefully only for error paths to guarantee the tracing flow is
terminated.

axisMsgCtx.setEnvelope(createDefaultSOAPEnvelope(messageContext));
}
} catch (DataServiceFault dataServiceFault) {
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, dataServiceFault);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Add tracing cleanup to the AxisFault catch block.

The DataServiceFault catch block correctly calls closeFlowForcefully, but the AxisFault catch block (lines 255-260) doesn't. If an AxisFault is thrown after reportEntryEvent is called on line 211, the tracing flow will remain open, leading to incomplete traces and potential resource leaks.

Apply this diff to add tracing cleanup to the AxisFault handler:

         } catch (AxisFault axisFault) {
+            DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, axisFault);
             if (synLog.isTraceOrDebugEnabled()) {
                 synLog.traceOrDebug(axisFault.getMessage());
             }
📝 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
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, dataServiceFault);
} catch (AxisFault axisFault) {
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext, DATA_SERVICE_INDEX, axisFault);
if (synLog.isTraceOrDebugEnabled()) {
synLog.traceOrDebug(axisFault.getMessage());
}
🤖 Prompt for AI Agents
In
components/mediation/mediators/dataservices-mediator/org.wso2/micro/integrator/mediator/dataservice/src/main/java/org/wso2/micro/integrator/mediator/dataservice/DataServiceCallMediator.java
around line 247, the AxisFault catch block does not perform tracing cleanup
which can leave the tracing flow open; update the AxisFault handler to call
DataServicesTracingCollector.closeFlowForcefully(axis2MessageContext,
DATA_SERVICE_INDEX, dataServiceFault) just like the DataServiceFault handler —
if dataServiceFault is not in scope create one from the AxisFault (or pass null
only if safe) before calling closeFlowForcefully, and keep the existing
exception handling/logging intact.

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