[APIM] Transaction Count Collector and API Count Collector #8
Conversation
WalkthroughThe PR adds transaction counting and API count collection features to the APIM usage data collector, introduces a Publisher interface with retry mechanisms to replace direct HttpPublisher usage, and refactors common collectors accordingly. New OSGi service components and constants are introduced alongside POM dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Synapse as Synapse Handler
participant TCH as TransactionCountHandler
participant TCL as TransactionCountingLogic
participant TA as TransactionAggregator
participant Pub as Publisher
participant API as Receiver API
rect rgb(200, 220, 255)
note over Synapse,Pub: Transaction Counting Flow (each flow phase)
Synapse->>TCH: handleRequestIn/Out/ResponseIn/Out
TCH->>TCL: handleRequest*Flow(MessageContext)
TCL-->>TCH: count (0 or 1)
TCH->>TA: addTransactions(count)
TA->>TA: hourlyTransactionCount.addAndGet(count)
end
rect rgb(220, 255, 220)
note over TA,API: Hourly Publishing (scheduled)
TA->>TA: publishAndReset() [scheduled hourly]
TA->>TA: capture count & reset counter
TA->>Pub: publishToReceiver(ApiRequest)
Pub->>Pub: executeWithRetry(operation)
loop Retry Loop (max 3 attempts)
Pub->>Pub: callReceiverApi(ApiRequest)
Pub->>API: POST /receiver/usage-counts
alt 2xx Success
API-->>Pub: ApiResponse(success=true)
Pub-->>TA: success, break retry
else Non-2xx (retryable: 429,500,503)
API-->>Pub: ApiResponse(failure)
Pub->>Pub: exponential backoff
else Non-retryable (4xx)
API-->>Pub: ApiResponse(failure)
Pub-->>TA: PublisherException, exhausted
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/MetaInfoHolder.java (1)
54-63: Consider documenting null-return behavior or adding defensive checks.The getters
getNodeId()andgetProduct()returnnullif called beforeinitialize(). WhileisInitialized()exists for callers to check, this pattern can lead to subtle NPEs if callers forget to check.Consider either:
- Documenting the null-return behavior in the Javadoc
- Throwing an
IllegalStateExceptionif accessed before initializationThis is a minor concern since the codebase appears to initialize at startup via
MetaInformationPublisher.collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountHandler.java (2)
57-66: Unnecessary try-catch block.The try block contains only null checks and a simple assignment, neither of which throws exceptions. The catch block is dead code.
public TransactionCountHandler() { - try { - if (instance == null || instance.transactionAggregator == null) { - instance = this; - } - } catch (Exception e) { - LOG.error("TransactionCountHandler: Error in constructor", e); - this.enabled = false; + if (instance == null || instance.transactionAggregator == null) { + instance = this; } }
75-141: Consider extracting duplicated aggregation logic.All four flow handlers follow the same pattern: check enabled, get count, add to aggregator. This could be extracted to a helper method.
private void aggregateIfEnabled(int tCount) { if (tCount > 0 && instance.transactionAggregator != null && instance.transactionAggregator.isEnabled()) { instance.transactionAggregator.addTransactions(tCount); } } @Override public boolean handleRequestInFlow(MessageContext messageContext) { if (instance == null || !instance.enabled) { return true; } aggregateIfEnabled(TransactionCountingLogic.handleRequestInFlow(messageContext)); return true; } // Similar for other handlers...collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollector.java (5)
65-69: Incorrect log level inside debug guard.The
log.warn()is placed insideisDebugEnabled()check, which means warning messages will only appear when debug logging is enabled. This is semantically incorrect—warnings should typically be visible at WARN level regardless of DEBUG settings.Based on learnings, if the intent is to suppress all customer-visible logs, consider using
log.debug()instead oflog.warn():if (publisher == null) { - if(log.isDebugEnabled()) { - log.warn("Cannot collect API count - Publisher service not available"); - } + log.debug("Cannot collect API count - Publisher service not available"); return; }
80-84: Same logging issue:log.errorgated byisDebugEnabled.Consistently use
log.debug()if the intent is to hide errors from customers, or remove the guard if errors should be visible at ERROR level.} catch (Exception e) { - if(log.isDebugEnabled()) { - log.error("Error collecting and publishing API count", e); - } + log.debug("Error collecting and publishing API count", e); }
100-105: Simplify return statement.if (resultSet.next()) { - long count = resultSet.getLong("api_count"); - return count; + return resultSet.getLong("api_count"); } else { return 0; }
150-156: Redundant null check forpublisher.This method is private and only called from
collectAndPublish(), which already returns early ifpublisheris null (line 65-70). The null check here is unreachable dead code.private void publishApiCount(long apiCount, String type) { - if (publisher == null) { - if(log.isDebugEnabled()) { - log.warn("Cannot publish " + type + " - Publisher not available"); - } - return; - } - try {
93-113: Consider extracting duplicate query logic.
queryApiCount()andqueryMcpApiCount()have nearly identical structure. Consider extracting a common helper method to reduce duplication.+ private long executeCountQuery(String query, String columnName) throws PublisherException { + DataSource dataSource = publisher.getDataSource(); + try (Connection connection = dataSource.getConnection(); + PreparedStatement statement = connection.prepareStatement(query); + ResultSet resultSet = statement.executeQuery()) { + if (resultSet.next()) { + return resultSet.getLong(columnName); + } + return 0; + } catch (SQLException e) { + String errorMsg = "Failed to execute count query: " + query; + log.debug(errorMsg, e); + throw new PublisherException(errorMsg, e); + } + } + private long queryApiCount() throws PublisherException { - DataSource dataSource = publisher.getDataSource(); - - try (Connection connection = dataSource.getConnection(); - PreparedStatement statement = connection.prepareStatement(ApimUsageDataCollectorConstants.API_COUNT_QUERY); - ResultSet resultSet = statement.executeQuery()) { - - if (resultSet.next()) { - return resultSet.getLong("api_count"); - } else { - return 0; - } - } catch (SQLException e) { - String errorMsg = "Failed to query non-MCP API count from database"; - if(log.isDebugEnabled()) { - log.error(errorMsg, e); - } - throw new PublisherException(errorMsg, e); - } + return executeCountQuery(ApimUsageDataCollectorConstants.API_COUNT_QUERY, "api_count"); }Also applies to: 121-141
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollector.java (2)
102-119: UnusedApiResponsevariable.The
responsevariable at line 109 is assigned but never used. Either log success information using the response or remove the unnecessary assignment.// Publish to /deployment-information endpoint using Publisher with built-in retry - ApiResponse response = publisher.publishToReceiver(request); + publisher.publishToReceiver(request); } catch (PublisherException e) {Alternatively, if success logging is desired:
ApiResponse response = publisher.publishToReceiver(request); if (log.isDebugEnabled()) { log.debug("Successfully published deployment data, status: " + response.getStatusCode()); }
110-118: Consistent logging pattern issue.Same as in
ApiCountCollector.java:log.error()is gated byisDebugEnabled(). Based on learnings, this is intentional for customer-facing scenarios, but consider usinglog.debug()for semantic consistency.collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.java (1)
57-59: Hardcoded base URLs should be made configurable.
RECEIVER_BASE_URLandWSO2_BASE_URLare hardcoded. Based on learnings, this is a known placeholder. Consider adding a TODO comment to track this.+ // TODO: Make these URLs configurable (see learnings) private static final String APIM_DATASOURCE_NAME = "jdbc/WSO2AM_DB"; private static final String RECEIVER_BASE_URL = "https://localhost:9443"; private static final String WSO2_BASE_URL = "https://api.wso2.com";collectors/org.wso2.carbon.usage.data.collector.apim/pom.xml (1)
138-142: Version ranges may cause non-reproducible builds.Using version ranges like
[4.0.0,5.0.0)forsynapse.versioncan lead to different builds picking up different versions over time. Consider pinning to specific versions for reproducibility.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/MetaInformationPublisher.java (1)
80-93: UnusedApiResponsevariable and consistent logging pattern.Same patterns as in
DeploymentDataCollector.java:
- The
responsevariable at line 87 is unusedlog.warn()is gated byisDebugEnabled()which is semantically inconsistentSince this is a non-fatal best-effort operation, the current approach is acceptable, but consider:
// Publish to /meta-information endpoint using Publisher with built-in retry - ApiResponse response = publisher.publishToReceiver(request); - } catch (PublisherException e) { - // Log error but don't fail - meta info will be in every payload anyway - if(log.isDebugEnabled()) { - log.warn("Failed to publish MetaInformation at startup after all retries. " + - "This is not critical as meta info will be included in every payload.", e); - } + publisher.publishToReceiver(request); + log.debug("Successfully published MetaInformation at startup"); + } catch (PublisherException e) { + log.debug("Failed to publish MetaInformation at startup after all retries. " + + "This is not critical as meta info will be included in every payload.", e);collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountingLogic.java (1)
69-74: Redundant null check with identical return value.The null check at lines 70-72 is unnecessary since the method returns 0 in all cases regardless of the
messageContextvalue. This can be simplified.Apply this diff to simplify:
public static int handleResponseInFlow(MessageContext messageContext) { - if (messageContext == null) { - return 0; - } + // Response in-flow is not counted return 0; }collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorServiceComponent.java (2)
60-60: Unused constantAPI_COUNT_INTERVAL_PROPERTY.This constant is defined but never used. Either implement the configurable interval feature or remove this dead code.
141-141: Use the defined constant for shutdown timeout.The shutdown timeout is hardcoded as
60here, butApimUsageDataCollectorConstants.SHUTDOWN_TIMEOUT_SECONDSis available and used inTransactionAggregator.shutdown(). Use the constant for consistency.Apply this diff:
- if (!apiCountExecutorService.awaitTermination(60, TimeUnit.SECONDS)) { + if (!apiCountExecutorService.awaitTermination( + ApimUsageDataCollectorConstants.SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
collectors/org.wso2.carbon.usage.data.collector.apim/pom.xml(3 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollector.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollectorTask.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/aggregator/TransactionAggregator.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountHandler.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountingLogic.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorConstants.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorServiceComponent.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollector.java(8 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollectorTask.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/MetaInformationPublisher.java(3 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/CommonUsageDataCollectorConstants.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/UsageDataCollectorServiceComponent.java(4 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java(3 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/impl/HttpPublisher.java(0 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/MetaInfoHolder.java(2 hunks)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/UsageDataUtil.java(2 hunks)
💤 Files with no reviewable changes (1)
- collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/impl/HttpPublisher.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-20T06:40:16.391Z
Learnt from: RDPerera
Repo: wso2/usage-data-collector PR: 2
File: collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java:47-47
Timestamp: 2025-11-20T06:40:16.391Z
Learning: In the wso2/usage-data-collector repository, for the MI collector module (org.wso2.carbon.usage.data.collector.mi), the DataSource name constant in PublisherImpl.java intentionally omits the "jdbc/" JNDI prefix (uses "WSO2_CONSUMPTION_TRACKING_DB") while the Javadoc examples in Publisher.java show the full JNDI name with prefix ("jdbc/WSO2_CONSUMPTION_TRACKING_DB"). This mismatch is by design.
Applied to files:
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollector.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/MetaInformationPublisher.javacollectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorConstants.javacollectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollector.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/UsageDataCollectorServiceComponent.java
📚 Learning: 2025-11-07T06:07:23.073Z
Learnt from: ashanhr
Repo: wso2/usage-data-collector PR: 1
File: publisher/org.wso2.carbon.usage.data.publisher.impl/src/main/java/org/wso2/carbon/usage/data/publisher/impl/HttpPublisher.java:54-54
Timestamp: 2025-11-07T06:07:23.073Z
Learning: In the wso2/usage-data-collector repository, the URL `https://analytics.wso2.com/receiver` hardcoded in HttpPublisher.java is a placeholder/dummy URL and does not represent a real endpoint. The team plans to make it configurable in a future update.
Applied to files:
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/CommonUsageDataCollectorConstants.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/MetaInformationPublisher.javacollectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollector.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/UsageDataCollectorServiceComponent.java
📚 Learning: 2025-11-21T04:35:45.575Z
Learnt from: inthirakumaaran
Repo: wso2/usage-data-collector PR: 4
File: collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/counter/UserCounter.java:170-227
Timestamp: 2025-11-21T04:35:45.575Z
Learning: In the WSO2 usage data collector (collectors/org.wso2.carbon.usage.data.collector.identity), errors should not be logged at ERROR level in production for customer-facing scenarios. Error logging should be gated behind debug flags to avoid exposing internal issues to customers unless debug logging is explicitly enabled.
Applied to files:
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/MetaInformationPublisher.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/MetaInfoHolder.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollector.javacollectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/UsageDataUtil.java
📚 Learning: 2025-11-07T05:59:27.709Z
Learnt from: ashanhr
Repo: wso2/usage-data-collector PR: 1
File: collectors/org.wso2.carbon.deployment.data.collector/src/main/java/org/wso2/carbon/deployment/data/collector/internal/DeploymentDataCollectorServiceComponent.java:126-146
Timestamp: 2025-11-07T05:59:27.709Z
Learning: In the wso2/usage-data-collector codebase, `ReferencePolicy.DYNAMIC` is the preferred pattern for OSGi Reference annotations to maintain consistency across the codebase.
Applied to files:
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/UsageDataCollectorServiceComponent.java
🧬 Code graph analysis (8)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorServiceComponent.java (4)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.java (1)
Component(48-171)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollector.java (1)
ApiCountCollector(44-180)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollectorTask.java (1)
ApiCountCollectorTask(29-55)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountHandler.java (1)
TransactionCountHandler(28-160)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountHandler.java (2)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/aggregator/TransactionAggregator.java (1)
TransactionAggregator(36-198)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountingLogic.java (1)
TransactionCountingLogic(25-89)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollector.java (6)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/PublisherException.java (1)
PublisherException(24-33)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiRequest.java (1)
ApiRequest(27-136)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiResponse.java (1)
ApiResponse(27-118)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/UsageCount.java (1)
UsageCount(12-66)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/MetaInfoHolder.java (1)
MetaInfoHolder(29-80)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorConstants.java (1)
ApimUsageDataCollectorConstants(27-55)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java (2)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiRequest.java (1)
ApiRequest(27-136)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiResponse.java (1)
ApiResponse(27-118)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountingLogic.java (1)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorConstants.java (1)
ApimUsageDataCollectorConstants(27-55)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.java (3)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/PublisherException.java (1)
PublisherException(24-33)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiRequest.java (1)
ApiRequest(27-136)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiResponse.java (1)
ApiResponse(27-118)
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/aggregator/TransactionAggregator.java (6)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/PublisherException.java (1)
PublisherException(24-33)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiRequest.java (1)
ApiRequest(27-136)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiResponse.java (1)
ApiResponse(27-118)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/UsageCount.java (1)
UsageCount(12-66)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/MetaInfoHolder.java (1)
MetaInfoHolder(29-80)collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorConstants.java (1)
ApimUsageDataCollectorConstants(27-55)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollector.java (4)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/CommonUsageDataCollectorConstants.java (1)
CommonUsageDataCollectorConstants(24-33)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/PublisherException.java (1)
PublisherException(24-33)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiRequest.java (1)
ApiRequest(27-136)collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/model/ApiResponse.java (1)
ApiResponse(27-118)
🔇 Additional comments (26)
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/UsageDataUtil.java (2)
107-109: Debug-gating node IP resolution error logging aligns with repo logging policyWrapping the
log.errorin anisDebugEnabled()check keeps the existing fallback to127.0.0.1but prevents error logs from appearing unless debug is enabled, matching the pattern of avoiding noisy/error logs in normal production operation. This is consistent with the usage collector logging guidance.
Based on learnings, this matches the intended logging behavior.
141-143: Debug-gating SHA-256 failure logging while preserving fallback behaviorConditioning the
log.erroronlog.isDebugEnabled()maintains the existing functional fallback toinput.hashCode()while ensuring the stack trace is only emitted when debug logging is explicitly enabled. This keeps production logs cleaner while still allowing deep diagnostics when needed.
Based on learnings, this is consistent with the desired logging strategy.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/DeploymentDataCollectorTask.java (1)
42-44: Error logs should not be guarded by debug checks.The
log.error()call is wrapped insideif(log.isDebugEnabled()), which means errors will only be logged when debug logging is enabled. In production environments where debug logging is typically disabled, critical errors during deployment data collection will be completely hidden, making troubleshooting impossible.Error-level logs should always be emitted regardless of the logging level.
Apply this diff to fix the issue:
} catch (Exception e) { - if(log.isDebugEnabled()) { - log.error("Error executing deployment data collection task", e); - } + log.error("Error executing deployment data collection task", e); // Don't propagate exception - let scheduler continue }⛔ Skipped due to learnings
Learnt from: inthirakumaaran Repo: wso2/usage-data-collector PR: 4 File: collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/counter/UserCounter.java:170-227 Timestamp: 2025-11-21T04:35:45.575Z Learning: In the WSO2 usage data collector (collectors/org.wso2.carbon.usage.data.collector.identity), errors should not be logged at ERROR level in production for customer-facing scenarios. Error logging should be gated behind debug flags to avoid exposing internal issues to customers unless debug logging is explicitly enabled.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/UsageDataCollectorServiceComponent.java (4)
64-80: LGTM!The OSGi
@Referenceannotation correctly usesReferencePolicy.DYNAMIC, which is consistent with the codebase conventions. Based on learnings,ReferencePolicy.DYNAMICis the preferred pattern for OSGi Reference annotations in this repository.
125-143: LGTM!The deactivation logic properly cancels the scheduled task, shuts down the executor gracefully with a timeout, and correctly handles
InterruptedExceptionby restoring the interrupt flag.
118-122: Activation failures silently swallowed when debug is disabled.If an exception occurs during component activation, it's silently swallowed when debug logging is disabled. This makes troubleshooting production issues extremely difficult.
} catch (Exception e) { - if(log.isDebugEnabled()) { - log.error("Failed to activate Usage Data Collector Service Component", e); - } + log.error("Failed to activate Usage Data Collector Service Component", e); }⛔ Skipped due to learnings
Learnt from: inthirakumaaran Repo: wso2/usage-data-collector PR: 4 File: collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/counter/UserCounter.java:170-227 Timestamp: 2025-11-21T04:35:45.575Z Learning: In the WSO2 usage data collector (collectors/org.wso2.carbon.usage.data.collector.identity), errors should not be logged at ERROR level in production for customer-facing scenarios. Error logging should be gated behind debug flags to avoid exposing internal issues to customers unless debug logging is explicitly enabled.
85-90: Incorrect log level gating pattern.The error is logged at
ERRORlevel but only when debug is enabled, which is contradictory. If the publisher is unavailable, this is a genuine error that should be logged unconditionally (at least atWARNlevel) or consistently gated with the appropriate level.Based on learnings, if errors should be hidden in production, consider using
log.debug()instead oflog.error()inside the debug check, or uselog.warn()unconditionally:if (publisher == null) { - if(log.isDebugEnabled()) { - log.error("Publisher not available - cannot start usage data collector"); - } + log.warn("Publisher not available - cannot start usage data collector"); return; }⛔ Skipped due to learnings
Learnt from: inthirakumaaran Repo: wso2/usage-data-collector PR: 4 File: collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/counter/UserCounter.java:170-227 Timestamp: 2025-11-21T04:35:45.575Z Learning: In the WSO2 usage data collector (collectors/org.wso2.carbon.usage.data.collector.identity), errors should not be logged at ERROR level in production for customer-facing scenarios. Error logging should be gated behind debug flags to avoid exposing internal issues to customers unless debug logging is explicitly enabled.Learnt from: RDPerera Repo: wso2/usage-data-collector PR: 2 File: collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java:47-47 Timestamp: 2025-11-20T06:40:16.391Z Learning: In the wso2/usage-data-collector repository, for the MI collector module (org.wso2.carbon.usage.data.collector.mi), the DataSource name constant in PublisherImpl.java intentionally omits the "jdbc/" JNDI prefix (uses "WSO2_CONSUMPTION_TRACKING_DB") while the Javadoc examples in Publisher.java show the full JNDI name with prefix ("jdbc/WSO2_CONSUMPTION_TRACKING_DB"). This mismatch is by design.Learnt from: ashanhr Repo: wso2/usage-data-collector PR: 1 File: publisher/org.wso2.carbon.usage.data.publisher.impl/src/main/java/org/wso2/carbon/usage/data/publisher/impl/HttpPublisher.java:54-54 Timestamp: 2025-11-07T06:07:23.073Z Learning: In the wso2/usage-data-collector repository, the URL `https://analytics.wso2.com/receiver` hardcoded in HttpPublisher.java is a placeholder/dummy URL and does not represent a real endpoint. The team plans to make it configurable in a future update.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/util/MetaInfoHolder.java (1)
29-49: LGTM!The thread-safety approach using
volatilefields withsynchronizedinitialization is appropriate for this singleton-like cache pattern. Thevolatilekeyword ensures visibility whilesynchronizedon mutating methods ensures atomicity of the check-then-set logic.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/internal/CommonUsageDataCollectorConstants.java (1)
24-33: LGTM!Clean implementation of a constants holder class with private constructor to prevent instantiation. The endpoint paths align with the Publisher-based publishing flow introduced in this PR.
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/apicount/ApiCountCollectorTask.java (2)
40-47: LGTM!The task wrapper correctly delegates to the collector and the exception handling pattern (catching and not propagating) is correct for
ScheduledExecutorServicetasks, which would otherwise stop scheduling on uncaught exceptions.
48-52: Error silently swallowed when debug is disabled.Same pattern as in
UsageDataCollectorServiceComponent- errors during task execution are only logged when debug is enabled. While swallowing the exception to prevent scheduler termination is correct, the error should still be logged to aid troubleshooting.} catch (Exception e) { - if(log.isDebugEnabled()) { - log.error("Error executing API count collection task", e); - } + log.error("Error executing API count collection task", e); // Don't propagate exception - let scheduler continue }⛔ Skipped due to learnings
Learnt from: inthirakumaaran Repo: wso2/usage-data-collector PR: 4 File: collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/counter/UserCounter.java:170-227 Timestamp: 2025-11-21T04:35:45.575Z Learning: In the WSO2 usage data collector (collectors/org.wso2.carbon.usage.data.collector.identity), errors should not be logged at ERROR level in production for customer-facing scenarios. Error logging should be gated behind debug flags to avoid exposing internal issues to customers unless debug logging is explicitly enabled.collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountHandler.java (1)
51-55: Verify:unregisterPublisherdoesn't disable aggregation.After unregistering the publisher,
enabledremainstrueandtransactionAggregatorstill holds a reference to the old publisher. Transactions will continue to be counted and potentially published via the stale publisher reference in the aggregator.Should this also set
enabled = falseor calltransactionAggregator.shutdown()?collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.java (2)
89-108: Double-checked locking implementation is correct.The synchronized block with double-check for
dataSourceinitialization is thread-safe and properly implements lazy initialization. Good pattern usage.
129-170:executeApiCallonly supports POST method.The
ApiRequestsupports configurablehttpMethod, butexecuteApiCallalways createsHttpPost. If other HTTP methods are needed in the future, this will need refactoring.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/collector/MetaInformationPublisher.java (1)
45-51: Good refactoring to Publisher abstraction.The migration from
HttpPublisherto the genericPublisherinterface is clean. The constructor properly initializes both the publisher and theDeploymentDataCollectorwith the same publisher instance, maintaining consistency.collectors/org.wso2.carbon.usage.data.collector.apim/pom.xml (1)
53-57: Jackson-databind 2.16.0 is secure for the most recent CVE.Version 2.16.0 (released November 2023) is the fixed release for CVE-2023-35116 and addresses a critical DoS vulnerability. However, Jackson-databind has a history of high-severity deserialization vulnerabilities. Recommend running a dependency scanner to verify that no vulnerable transitive versions are included, and consider upgrading to the latest 2.x release or 3.x if available for additional hardening.
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorConstants.java (1)
27-55: LGTM!The constants class follows proper utility class patterns with a private constructor to prevent instantiation. The constants are well-organized by category and appropriately named.
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountingLogic.java (2)
27-53: LGTM!The
handleRequestInFlowmethod correctly handles inbound detection, WebSocket transport detection, and sets the associated incoming request property for tracking request-response pairs. Null checks are appropriately placed.
55-67: LGTM!The
handleRequestOutFlowandhandleResponseOutFlowmethods correctly implement complementary logic for counting standalone outbound messages and request-response pairs respectively.Also applies to: 76-88
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/internal/ApimUsageDataCollectorServiceComponent.java (2)
69-91: LGTM!The OSGi service binding correctly registers/unregisters the publisher with
TransactionCountHandlerduring bind/unbind lifecycle, ensuring transaction counting is enabled when a publisher is available.
97-102: Incorrect logging pattern: error/warn logs guarded byisDebugEnabled().Using
isDebugEnabled()to guardlog.error()orlog.warn()calls suppresses critical error information in production. Error and warning logs should always be emitted regardless of debug level.Apply this diff to fix the logging:
if (publisher == null) { - if(log.isDebugEnabled()) { - log.warn("Publisher not available - APIM usage data collection may not work properly"); - } + log.warn("Publisher not available - APIM usage data collection may not work properly"); return; }} catch (Exception e) { - if(log.isDebugEnabled()) { - log.error("Failed to activate APIM Usage Data Collector Service Component", e); - } + log.error("Failed to activate APIM Usage Data Collector Service Component", e); }} catch (InterruptedException e) { apiCountExecutorService.shutdownNow(); Thread.currentThread().interrupt(); - if(log.isDebugEnabled()) { - log.error("Interrupted while shutting down API count collector executor", e); - } + log.error("Interrupted while shutting down API count collector executor", e); }Also applies to: 124-128, 147-150
⛔ Skipped due to learnings
Learnt from: inthirakumaaran Repo: wso2/usage-data-collector PR: 4 File: collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/counter/UserCounter.java:170-227 Timestamp: 2025-11-21T04:35:45.575Z Learning: In the WSO2 usage data collector (collectors/org.wso2.carbon.usage.data.collector.identity), errors should not be logged at ERROR level in production for customer-facing scenarios. Error logging should be gated behind debug flags to avoid exposing internal issues to customers unless debug logging is explicitly enabled.Learnt from: RDPerera Repo: wso2/usage-data-collector PR: 2 File: collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java:47-47 Timestamp: 2025-11-20T06:40:16.391Z Learning: In the wso2/usage-data-collector repository, for the MI collector module (org.wso2.carbon.usage.data.collector.mi), the DataSource name constant in PublisherImpl.java intentionally omits the "jdbc/" JNDI prefix (uses "WSO2_CONSUMPTION_TRACKING_DB") while the Javadoc examples in Publisher.java show the full JNDI name with prefix ("jdbc/WSO2_CONSUMPTION_TRACKING_DB"). This mismatch is by design.collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java (2)
115-166: LGTM!The retry implementation correctly handles success/failure detection, retryable vs non-retryable errors, proper interruption handling, and exception chaining. The backoff delay increases linearly with each attempt.
193-208: LGTM!The
shouldRetrymethod correctly categorizes HTTP status codes: transient errors (404, 408, 429, 5xx) are retryable, while client errors indicating permanent failures (400, 401, 403, 405) fail immediately without retry.collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/aggregator/TransactionAggregator.java (3)
141-159: Period time parameters are captured but not used.The
periodStartTimeandperiodEndTimeparameters are passed topublishTransaction()but are not included in theUsageCountpayload. If these times are meant to be reported, consider adding them to the payload. Otherwise, remove the unused parameters.
174-197: LGTM with minor note on shutdown ordering.The shutdown implementation correctly publishes final data and gracefully terminates the executor. Note that there's a theoretical race where the scheduler could fire between
publishAndReset()andshutdown(), but sincegetAndSet(0)is atomic, any overlapping call would simply publish 0, which is acceptable per the comment at line 126.
49-58: LGTM!The singleton pattern is correctly implemented with double-checked locking and volatile. The
init()method properly handles idempotent initialization by checking existing executor state and cleaning up before re-initialization.Also applies to: 60-112
| <releases> | ||
| <enabled>true</enabled> | ||
| <updatePolicy>daily</updatePolicy> | ||
| <checksumPolicy>ignore</checksumPolicy> | ||
| </releases> | ||
| </repository> | ||
| <repository> | ||
| <id>wso2.releases</id> | ||
| <name>WSO2 internal Repository</name> | ||
| <url>https://maven.wso2.org/nexus/content/repositories/releases/</url> | ||
| <releases> | ||
| <enabled>true</enabled> | ||
| <updatePolicy>daily</updatePolicy> | ||
| <checksumPolicy>ignore</checksumPolicy> | ||
| </releases> |
There was a problem hiding this comment.
Checksum policy set to "ignore" may mask artifact integrity issues.
Setting <checksumPolicy>ignore</checksumPolicy> disables verification of downloaded artifact checksums. This could allow corrupted or tampered artifacts to be used without warning.
Consider using warn or fail for better security:
<releases>
<enabled>true</enabled>
<updatePolicy>daily</updatePolicy>
- <checksumPolicy>ignore</checksumPolicy>
+ <checksumPolicy>warn</checksumPolicy>
</releases>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In collectors/org.wso2.carbon.usage.data.collector.apim/pom.xml around lines 148
to 162, the repository entries use <checksumPolicy>ignore</checksumPolicy>,
which disables checksum verification; change those to a safer policy (e.g.,
<checksumPolicy>warn</checksumPolicy> or <checksumPolicy>fail</checksumPolicy>)
for both repository blocks to ensure artifact checksum validation, keeping the
tag name and XML structure unchanged and choosing "fail" for strict integrity or
"warn" if you need permissive behavior during development.
| private Publisher publisher; | ||
| private ScheduledExecutorService scheduledExecutorService; | ||
| private long currentHourStartTime; | ||
| private boolean enabled = false; |
There was a problem hiding this comment.
Field enabled should be volatile for thread visibility.
The enabled field is read by multiple threads (handler threads calling addTransactions() and isEnabled(), scheduler thread in publishAndReset()) but is not volatile. Without volatile, changes made in init() or shutdown() may not be visible to other threads.
Apply this diff:
- private boolean enabled = false;
+ private volatile boolean enabled = false;📝 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.
| private boolean enabled = false; | |
| private volatile boolean enabled = false; |
🤖 Prompt for AI Agents
In
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/aggregator/TransactionAggregator.java
around line 45, the private boolean enabled field must be made volatile to
ensure visibility across threads; change its declaration to use the volatile
modifier (private volatile boolean enabled = false;) and rebuild to ensure no
compile errors; no other behavioral changes are required.
...carbon/usage/data/collector/apim/collector/transaction/aggregator/TransactionAggregator.java
Show resolved
Hide resolved
| public class TransactionCountHandler extends AbstractExtendedSynapseHandler { | ||
| private static final Log LOG = LogFactory.getLog(TransactionCountHandler.class); | ||
| private TransactionAggregator transactionAggregator; | ||
| private Publisher publisher; | ||
| private volatile boolean enabled = false; | ||
| private static TransactionCountHandler instance; | ||
|
|
||
| public static synchronized void registerPublisher(Publisher publisher) { | ||
| if (instance == null) { | ||
| instance = new TransactionCountHandler(); | ||
| } | ||
| instance.publisher = publisher; | ||
| if (instance.transactionAggregator == null) { | ||
| instance.transactionAggregator = TransactionAggregator.getInstance(); | ||
| } | ||
| synchronized (instance.transactionAggregator) { | ||
| if (!instance.transactionAggregator.isEnabled()) { | ||
| instance.transactionAggregator.init(publisher); | ||
| } | ||
| } | ||
| instance.enabled = true; | ||
| } | ||
|
|
||
| public static synchronized void unregisterPublisher(Publisher publisher) { | ||
| if (instance != null && instance.publisher == publisher) { | ||
| instance.publisher = null; | ||
| } | ||
| } | ||
|
|
||
| public TransactionCountHandler() { | ||
| try { | ||
| if (instance == null || instance.transactionAggregator == null) { | ||
| instance = this; | ||
| } | ||
| } catch (Exception e) { | ||
| LOG.error("TransactionCountHandler: Error in constructor", e); | ||
| this.enabled = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Mixed static and instance state creates confusion and potential bugs.
The class mixes static singleton pattern with instance fields in a confusing way:
instanceis static buttransactionAggregator,publisher, andenabledare instance fields- The constructor at line 60 assigns
thistoinstance, potentially overwriting a properly initialized instance registerPublisheroperates on the staticinstance, setting its instance fields- Flow handlers use
instance.transactionAggregatorbuthandleServerShutDownusesthis.transactionAggregator
This can lead to subtle bugs where shutdown operates on a different object than the one handling traffic.
Consider making transactionAggregator, publisher, and enabled static to match the singleton pattern:
public class TransactionCountHandler extends AbstractExtendedSynapseHandler {
private static final Log LOG = LogFactory.getLog(TransactionCountHandler.class);
- private TransactionAggregator transactionAggregator;
- private Publisher publisher;
- private volatile boolean enabled = false;
+ private static TransactionAggregator transactionAggregator;
+ private static Publisher publisher;
+ private static volatile boolean enabled = false;
private static TransactionCountHandler instance;📝 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.
| public class TransactionCountHandler extends AbstractExtendedSynapseHandler { | |
| private static final Log LOG = LogFactory.getLog(TransactionCountHandler.class); | |
| private TransactionAggregator transactionAggregator; | |
| private Publisher publisher; | |
| private volatile boolean enabled = false; | |
| private static TransactionCountHandler instance; | |
| public static synchronized void registerPublisher(Publisher publisher) { | |
| if (instance == null) { | |
| instance = new TransactionCountHandler(); | |
| } | |
| instance.publisher = publisher; | |
| if (instance.transactionAggregator == null) { | |
| instance.transactionAggregator = TransactionAggregator.getInstance(); | |
| } | |
| synchronized (instance.transactionAggregator) { | |
| if (!instance.transactionAggregator.isEnabled()) { | |
| instance.transactionAggregator.init(publisher); | |
| } | |
| } | |
| instance.enabled = true; | |
| } | |
| public static synchronized void unregisterPublisher(Publisher publisher) { | |
| if (instance != null && instance.publisher == publisher) { | |
| instance.publisher = null; | |
| } | |
| } | |
| public TransactionCountHandler() { | |
| try { | |
| if (instance == null || instance.transactionAggregator == null) { | |
| instance = this; | |
| } | |
| } catch (Exception e) { | |
| LOG.error("TransactionCountHandler: Error in constructor", e); | |
| this.enabled = false; | |
| } | |
| } | |
| public class TransactionCountHandler extends AbstractExtendedSynapseHandler { | |
| private static final Log LOG = LogFactory.getLog(TransactionCountHandler.class); | |
| private static TransactionAggregator transactionAggregator; | |
| private static Publisher publisher; | |
| private static volatile boolean enabled = false; | |
| private static TransactionCountHandler instance; | |
| public static synchronized void registerPublisher(Publisher publisher) { | |
| if (instance == null) { | |
| instance = new TransactionCountHandler(); | |
| } | |
| instance.publisher = publisher; | |
| if (instance.transactionAggregator == null) { | |
| instance.transactionAggregator = TransactionAggregator.getInstance(); | |
| } | |
| synchronized (instance.transactionAggregator) { | |
| if (!instance.transactionAggregator.isEnabled()) { | |
| instance.transactionAggregator.init(publisher); | |
| } | |
| } | |
| instance.enabled = true; | |
| } | |
| public static synchronized void unregisterPublisher(Publisher publisher) { | |
| if (instance != null && instance.publisher == publisher) { | |
| instance.publisher = null; | |
| } | |
| } | |
| public TransactionCountHandler() { | |
| try { | |
| if (instance == null || instance.transactionAggregator == null) { | |
| instance = this; | |
| } | |
| } catch (Exception e) { | |
| LOG.error("TransactionCountHandler: Error in constructor", e); | |
| this.enabled = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/collector/transaction/counter/TransactionCountHandler.java
around lines 28 to 66, the class mixes a static singleton reference with
instance fields (transactionAggregator, publisher, enabled) and assigns this to
the static instance in the constructor, which can lead to different objects
being used for runtime handling vs. shutdown; fix by making the singleton
contract consistent: either convert transactionAggregator, publisher, and
enabled to static fields and remove assigning this to instance in the
constructor, or refactor to a proper singleton (private constructor, static
getInstance() returning the single instance, and all access via that instance)
and ensure all methods (including handleServerShutDown) reference the same
singleton instance rather than 'this'; update
registerPublisher/unregisterPublisher to use the chosen singleton access pattern
and remove the ambiguous constructor assignment.
| @Override | ||
| public boolean handleServerShutDown() { | ||
| LOG.debug("Shutting down Transaction Counter..."); | ||
|
|
||
| // Clean up resources | ||
| if (transactionAggregator != null && transactionAggregator.isEnabled()) { | ||
| transactionAggregator.shutdown(); | ||
| } | ||
|
|
||
| LOG.debug("Transaction Counter shutdown completed"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Shutdown uses wrong reference - may fail to clean up resources.
handleServerShutDown uses this.transactionAggregator but the aggregator is initialized on the static instance. If Synapse invokes shutdown on a different handler instance (common in OSGi/Synapse lifecycles), this condition will be false and resources won't be cleaned up.
@Override
public boolean handleServerShutDown() {
LOG.debug("Shutting down Transaction Counter...");
// Clean up resources
- if (transactionAggregator != null && transactionAggregator.isEnabled()) {
- transactionAggregator.shutdown();
+ if (instance != null && instance.transactionAggregator != null
+ && instance.transactionAggregator.isEnabled()) {
+ instance.transactionAggregator.shutdown();
}
LOG.debug("Transaction Counter shutdown completed");
return true;
}| SSLContextBuilder sslBuilder = new SSLContextBuilder(); | ||
| sslBuilder.loadTrustMaterial(null, (chain, authType) -> true); | ||
| SSLConnectionSocketFactory sslsf = new SSLConnectionSocketFactory( | ||
| sslBuilder.build(), | ||
| NoopHostnameVerifier.INSTANCE | ||
| ); | ||
|
|
||
| httpClient = HttpClientBuilder.create() | ||
| .setSSLSocketFactory(sslsf) | ||
| .build(); |
There was a problem hiding this comment.
Critical: Insecure SSL configuration trusts all certificates.
The SSL context is configured to trust all certificates ((chain, authType) -> true) and uses NoopHostnameVerifier, which completely disables SSL/TLS verification. This exposes the application to man-in-the-middle attacks.
This configuration is acceptable only for development/testing. For production, use proper certificate validation or at minimum, make this behavior configurable.
- SSLContextBuilder sslBuilder = new SSLContextBuilder();
- sslBuilder.loadTrustMaterial(null, (chain, authType) -> true);
- SSLConnectionSocketFactory sslsf = new SSLConnectionSocketFactory(
- sslBuilder.build(),
- NoopHostnameVerifier.INSTANCE
- );
-
- httpClient = HttpClientBuilder.create()
- .setSSLSocketFactory(sslsf)
- .build();
+ // TODO: Use proper SSL configuration for production
+ // For now, using permissive SSL for development only
+ if (Boolean.getBoolean("usage.collector.ssl.trustAll")) {
+ SSLContextBuilder sslBuilder = new SSLContextBuilder();
+ sslBuilder.loadTrustMaterial(null, (chain, authType) -> true);
+ SSLConnectionSocketFactory sslsf = new SSLConnectionSocketFactory(
+ sslBuilder.build(),
+ NoopHostnameVerifier.INSTANCE
+ );
+ httpClient = HttpClientBuilder.create()
+ .setSSLSocketFactory(sslsf)
+ .build();
+ } else {
+ httpClient = HttpClientBuilder.create().build();
+ }📝 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.
| SSLContextBuilder sslBuilder = new SSLContextBuilder(); | |
| sslBuilder.loadTrustMaterial(null, (chain, authType) -> true); | |
| SSLConnectionSocketFactory sslsf = new SSLConnectionSocketFactory( | |
| sslBuilder.build(), | |
| NoopHostnameVerifier.INSTANCE | |
| ); | |
| httpClient = HttpClientBuilder.create() | |
| .setSSLSocketFactory(sslsf) | |
| .build(); | |
| // TODO: Use proper SSL configuration for production | |
| // For now, using permissive SSL for development only | |
| if (Boolean.getBoolean("usage.collector.ssl.trustAll")) { | |
| SSLContextBuilder sslBuilder = new SSLContextBuilder(); | |
| sslBuilder.loadTrustMaterial(null, (chain, authType) -> true); | |
| SSLConnectionSocketFactory sslsf = new SSLConnectionSocketFactory( | |
| sslBuilder.build(), | |
| NoopHostnameVerifier.INSTANCE | |
| ); | |
| httpClient = HttpClientBuilder.create() | |
| .setSSLSocketFactory(sslsf) | |
| .build(); | |
| } else { | |
| httpClient = HttpClientBuilder.create().build(); | |
| } |
| @Deactivate | ||
| protected void deactivate() { | ||
| } |
There was a problem hiding this comment.
Missing resource cleanup in deactivate().
The HttpClient created in activate() should be closed in deactivate() to prevent resource leaks when the OSGi component is deactivated.
@Deactivate
protected void deactivate() {
+ if (httpClient instanceof java.io.Closeable) {
+ try {
+ ((java.io.Closeable) httpClient).close();
+ } catch (Exception e) {
+ log.debug("Error closing HttpClient", e);
+ }
+ }
}📝 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.
| @Deactivate | |
| protected void deactivate() { | |
| } | |
| @Deactivate | |
| protected void deactivate() { | |
| if (httpClient instanceof java.io.Closeable) { | |
| try { | |
| ((java.io.Closeable) httpClient).close(); | |
| } catch (Exception e) { | |
| log.debug("Error closing HttpClient", e); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
collectors/org.wso2.carbon.usage.data.collector.apim/src/main/java/org/wso2/carbon/usage/data/collector/apim/publisher/ApimPublisher.java
around lines 85-87, the deactivate() method is empty but must close the
HttpClient created in activate() to avoid resource leaks; update deactivate() to
safely close the HttpClient instance (check for null, call its close/shutdown
method or equivalent, catch and log any IOException/Exception), and then null
out the reference so the client can be garbage-collected.
| * <p>Retry behavior:</p> | ||
| * <ul> | ||
| * <li>Retries up to {@value #MAX_RETRIES} times</li> | ||
| * <li>Exponential backoff: 1s, 2s, 3s between retries</li> | ||
| * <li>Returns on first successful response (2xx status code)</li> | ||
| * <li>Throws exception if all retries fail</li> | ||
| * </ul> |
There was a problem hiding this comment.
Javadoc mismatch: backoff times don't match code.
The Javadoc states "Exponential backoff: 1s, 2s, 3s" but the actual implementation at line 154 uses attempt * RETRY_DELAY_MS (15s, 30s, 45s), which is linear backoff, not exponential. Update the documentation to reflect the actual behavior.
Apply this diff to fix the documentation:
- * <ul>
- * <li>Retries up to {@value #MAX_RETRIES} times</li>
- * <li>Exponential backoff: 1s, 2s, 3s between retries</li>
+ * <ul>
+ * <li>Retries up to {@value #MAX_RETRIES} times</li>
+ * <li>Linear backoff: 15s, 30s, 45s between retries</li>Also applies to: 90-96
🤖 Prompt for AI Agents
In
collectors/org.wso2.carbon.usage.data.collector.common/src/main/java/org/wso2/carbon/usage/data/collector/common/publisher/api/Publisher.java
around lines 70-76 (and also update the similar block at lines 90-96), the
Javadoc incorrectly describes the retry backoff as "Exponential backoff: 1s, 2s,
3s" while the implementation uses a linear backoff (delay = attempt *
RETRY_DELAY_MS producing 15s, 30s, 45s for attempts 1..3); update the Javadoc to
state "Linear backoff: attempts * RETRY_DELAY_MS (15s, 30s, 45s)" or equivalent
wording that matches the code and mention the actual RETRY_DELAY_MS value used.
1 participant
Purpose
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.