[IS] Add publisher to identity collectors. #9
Conversation
WalkthroughAdds an HTTP-based publishing pipeline to the identity usage collector: new Publisher implementation and HTTP client, app credential loader, OSGi registration of the publisher, collector changes to emit and send usage metrics, and Maven dependency/import-range updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UDC as UsageDataCollector
participant PUB as PublisherImp
participant HTTP as HTTPClient
participant REC as Receiver/WSO2 Endpoint
UDC->>UDC: collectAndPublish(report)
UDC->>UDC: publishUsageMetrics(report)
loop for each metric (TOTAL_USERS, TOTAL_B2B_ORGS, TOTAL_ROOT_ORGS)
UDC->>HTTP: createUsageDataRequest(count, type)
HTTP-->>UDC: ApiRequest
UDC->>PUB: callReceiverApi(ApiRequest)
PUB->>PUB: resolve endpoint (tenant scope or custom)
PUB->>HTTP: executeApiRequest(ApiRequest, endpoint, label)
HTTP->>REC: HTTP POST (JSON + optional auth/timeout)
REC-->>HTTP: HTTP Response (2xx / error)
HTTP-->>PUB: ApiResponse
PUB-->>UDC: ApiResponse
end
UDC->>UDC: log outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
55-57: Add error handling for HTTP client initialization.If
HTTPClientUtils.createClientWithCustomHostnameVerifier().build()fails during class loading, it will cause anExceptionInInitializerErrorthat's difficult to diagnose.Consider wrapping the initialization in a try-catch block and providing a fallback or clear error message:
static { - httpClient = HTTPClientUtils.createClientWithCustomHostnameVerifier().build(); + try { + httpClient = HTTPClientUtils.createClientWithCustomHostnameVerifier().build(); + } catch (Exception e) { + LOG.error("Failed to initialize HTTP client", e); + throw new ExceptionInInitializerError(e); + } }collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (2)
72-72: Make init() private to prevent external reinitialization.The
init()method is public, allowing external callers to reinitialize credentials at any time, which could cause race conditions.Apply this diff:
/** * Initializes and loads app credentials from configuration file */ -public void init() { +private void init() {
156-169: Add synchronization for thread-safe credential access.The
appNameandappPasswordfields are accessed without synchronization, which could cause visibility issues in multi-threaded environments.Consider making the getter methods synchronized or using
volatilefor the fields:-private String appName; -private char[] appPassword; +private volatile String appName; +private volatile char[] appPassword;collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (2)
45-57: Reuse HTTPClient instance instead of creating new instances.Lines 49 and 56 create new
HTTPClient()instances for each API call. SinceHTTPClientuses a staticCloseableHttpClient, creating multiple instances is wasteful.Consider making
HTTPClienta singleton or storing a single instance as a field:public class PublisherImp implements Publisher { private static final String RECEIVER_ENDPOINT = "/usage/data/receiver"; private static final String WSO2_ENDPOINT = "https://api.choreo.dev/test"; + private static final HTTPClient httpClient = new HTTPClient(); @Override public ApiResponse callReceiverApi(ApiRequest request) throws PublisherException { String endpoint = getEndpoint(request, false); - return new HTTPClient().executeApiRequest(request, endpoint, "receiver API"); + return httpClient.executeApiRequest(request, endpoint, "receiver API"); } @Override public ApiResponse callWso2Api(ApiRequest request) throws PublisherException { String endpoint = getEndpoint(request, true); - return new HTTPClient().executeApiRequest(request, endpoint, "WSO2 API"); + return httpClient.executeApiRequest(request, endpoint, "WSO2 API"); }
74-85: Improve error handling for URLBuilderException.Wrapping
URLBuilderExceptionin a genericRuntimeExceptionloses important context about the failure.Consider wrapping it in a more descriptive exception or logging the error:
private static String getReceiverEndpoint() { try { return ServiceURLBuilder.create() .addPath(RECEIVER_ENDPOINT) .setTenant(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME) .build() .getAbsoluteInternalURL(); } catch (URLBuilderException e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to build receiver endpoint URL: " + RECEIVER_ENDPOINT, e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
collectors/org.wso2.carbon.usage.data.collector.identity/pom.xml(4 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java(3 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java(2 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/UsageCollectorConstants.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java
📚 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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java
🧬 Code graph analysis (4)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (6)
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/UsageDataUtil.java (1)
UsageDataUtil(35-155)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (1)
AppCredentialsUtil(36-181)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/UsageCollectorConstants.java (1)
UsageCollectorConstants(24-32)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (5)
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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorDataHolder.java (1)
UsageDataCollectorDataHolder(28-71)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/model/SystemUsage.java (1)
SystemUsage(24-69)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
HTTPClient(49-132)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(34-86)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(34-86)
🔇 Additional comments (2)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
129-130: Verify the "Client " authorization scheme.The authorization header uses "Client " prefix, which is non-standard. Standard HTTP authentication schemes include "Basic", "Bearer", "Digest", etc. Verify that the receiving endpoint expects this custom "Client " scheme.
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (1)
189-203: LGTM: Error logging follows project conventions.The error logging at debug level aligns with project conventions for customer-facing scenarios, as per learnings from this repository.
...g/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java
Show resolved
Hide resolved
...entity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java
Show resolved
Hide resolved
...entity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java
Show resolved
Hide resolved
...tity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java
Show resolved
Hide resolved
...identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java
Show resolved
Hide resolved
...identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java
Show resolved
Hide resolved
...ity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java
Show resolved
Hide resolved
...rc/main/java/org/wso2/carbon/usage/data/collector/identity/util/UsageCollectorConstants.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (2)
88-104: Remove incorrect@throws PublisherExceptionfrom Javadoc.
createUsageDataRequestdoes not declare or throwPublisherException, so the@throws PublisherExceptiontag in the Javadoc is misleading and should be removed (Line 94).Apply this diff:
/** * Creates an ApiRequest for usage data with full customization. * * @param count The count value * @param type The type of usage data * @return ApiRequest object - * @throws PublisherException if creation fails */ public static ApiRequest createUsageDataRequest(int count, String type) {
116-133: Fix charset usage when building the authorization header.The header currently depends on the platform default charset via
toEncode.getBytes()andnew String(encoding, Charset.defaultCharset()), which can lead to environment‑dependent behavior for non‑ASCII credentials (Lines 128–130). Use an explicit charset (e.g., UTF‑8) and simplify viaencodeBase64String.Apply this diff:
private void setAuthorizationHeader(HttpUriRequestBase httpMethod) { AppCredentialsUtil credentialsUtil = AppCredentialsUtil.getInstance(); if (!credentialsUtil.hasCredentials()) { if (LOG.isDebugEnabled()) { LOG.debug("No authorization credentials configured, skipping auth header"); } return; } - String appName = credentialsUtil.getAppName(); - String appPassword = String.valueOf(credentialsUtil.getAppPassword()); - String toEncode = appName + ":" + appPassword; - byte[] encoding = Base64.encodeBase64(toEncode.getBytes()); - String authHeader = new String(encoding, Charset.defaultCharset()); + String appName = credentialsUtil.getAppName(); + String appPassword = String.valueOf(credentialsUtil.getAppPassword()); + String toEncode = appName + ":" + appPassword; + String authHeader = + Base64.encodeBase64String(toEncode.getBytes(StandardCharsets.UTF_8)); String CLIENT = "Client "; httpMethod.addHeader(HTTPConstants.HEADER_AUTHORIZATION, CLIENT + authHeader); }You may also want to review whether using
char[]for the password still provides value here, since it is immediately converted to aString; if you intend to rely onchar[]for security, consider zeroing it (and any intermediate byte arrays) after use. Please double‑check against the Apache Commons Codec and Java 11+ docs to confirm behavior and available helpers.Confirm that `org.apache.commons.codec.binary.Base64.encodeBase64String(byte[])` is appropriate for creating a Base64 text representation for HTTP headers and that using `StandardCharsets.UTF_8` is recommended for encoding credential strings in Java 11+.collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (1)
63-67: Avoid re-initializing credentials on everygetInstance()call.
getInstance()currently callsinstance.init()on every invocation, which repeatedly performs file IO and secret resolution and may become a hot path once used by HTTPClient (Lines 63–67). Initialize once instead.Apply this diff:
+import java.util.concurrent.atomic.AtomicBoolean; @@ - private static final AppCredentialsUtil instance = new AppCredentialsUtil(); + private static final AppCredentialsUtil instance = new AppCredentialsUtil(); + private static final AtomicBoolean initialized = new AtomicBoolean(false); @@ public static AppCredentialsUtil getInstance() { - instance.init(); + if (initialized.compareAndSet(false, true)) { + instance.init(); + } return instance; }This keeps initialization lazy, avoids repeated expensive work, and is safe under concurrent access.
🧹 Nitpick comments (3)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
59-86: HTTP execution flow and error handling look solid; consider exposing error body.The overall flow (timeout selection, JSON POST, try-with-resources, and 2xx mapping) looks good. You currently discard the
responseBodyon non‑2xx responses and only return"HTTP error: " + statusCode; consider including the response body (or part of it) in the failure message to aid debugging, while still avoiding sensitive data leakage.collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (2)
72-126: Initialization logic is reasonable; consider log level for configuration failures.The
init()method correctly handles missing files, resolves secrets, and closes theInputStreamin afinallyblock. One minor consideration: configuration load failures (Line 110) are only logged at DEBUG; you might want INFO/WARN there so misconfiguration is visible without debug logging.
164-188: Accessors are fine; optional: return a copy of the password array.The accessors and
hasCredentials()check are straightforward and correct. If you want to prevent accidental external mutation of the internal password array, consider returning a defensive copy fromgetAppPassword().public char[] getAppPassword() { - return appPassword; + return appPassword != null ? appPassword.clone() : null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java
🔇 Additional comments (2)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
106-114: RequestConfig construction is correct and consistent.Using a single
Timeoutinstance for connect, connection-request, and response is a clean, consistent setup and aligns with the timeout derived from the request.collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (1)
133-157: Secure vault integration is defensive and aligns with expected patterns.
resolveSecretsfollows the typical Secure Vault pattern: defaultingsecretProvider, checkingisInitialized(), iterating protected tokens, and updating properties. Swallowing exceptions with DEBUG logging is acceptable here since missing vault configuration should not be fatal to the collector.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
67-92: Publisher service lifecycle handling looks correct; minor idempotency tweak possible.Storing the
ServiceRegistration<?>and unregistering it incleanup()fixes the original leak and keeps the Publisher service lifecycle aligned with the component.If you expect
cleanup()to be called more than once (e.g., activation failure + deactivate), you could avoid relying onIllegalStateExceptionby nulling out the field after unregistering:if (publisherServiceRegistration != null) { try { publisherServiceRegistration.unregister(); - } catch (IllegalStateException e) { - // Service already unregistered - } + } catch (IllegalStateException e) { + // Service already unregistered + } finally { + publisherServiceRegistration = null; + } }This is optional, the current code is functionally fine.
Also applies to: 129-135
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (1)
24-33: HTTP-based metric publishing flow is sound; consider decoupling from the concrete Publisher implementation.The new flow to publish
TOTAL_USERS,TOTAL_B2B_ORGS, andTOTAL_ROOT_ORGSvia:
HTTPClient.createUsageDataRequest(count, type), andpublisher.callReceiverApi(request)withApiResponse-based success/failure loggingis consistent and keeps failures gated behind debug logging, which matches the project’s preference not to surface internal errors at higher log levels. Based on learnings, this logging pattern is appropriate.
For future iterations, you might want to:
- Declare the field as the interface type and avoid hard-coding the implementation:
// instead of private final PublisherImp publisher; // prefer private final Publisher publisher;
- And obtain the
Publisherfrom the OSGi service (registered inUsageDataCollectorServiceComponent) or a holder, rather than instantiatingnew PublisherImp()here. That would improve testability and keep the collector agnostic of the concrete publisher.The current implementation is still acceptable as-is.
Also applies to: 48-65, 67-75, 183-208
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
62-75: Handle receiver endpoint build failures more explicitly instead of falling through.When
ServiceURLBuilderfails,getReceiverEndpoint()returns an empty string. In that case,getEndpoint()still appends a suffix (e.g.,"/usage-counts") and the HTTP client is called with a malformed/host-less URL, which then fails generically insideHTTPClient.To make failures easier to diagnose and avoid relying on downstream URI parsing, consider short-circuiting when the base endpoint is empty, for example:
private static String getEndpoint(ApiRequest request) { - String endpoint = getReceiverEndpoint(); + String endpoint = getReceiverEndpoint(); + if (endpoint == null || endpoint.isEmpty()) { + return ""; + } if (request.getData() instanceof UsageCount) { endpoint += "/usage-counts"; } else if (request.getData() instanceof DeploymentInformation) { endpoint += "/deployment-information"; } else if (request.getData() instanceof MetaInformation) { endpoint += "/meta-information"; } else if (request.getEndpoint() != null && !request.getEndpoint().isEmpty()) { endpoint += "/" + request.getEndpoint(); } return endpoint; }and then in
callReceiverApidetect the empty endpoint and either:
- throw a
PublisherExceptionwith a clear message, or- return an
ApiResponse.failure(...)indicating the receiver endpoint could not be resolved.This isn’t strictly required for correctness (HTTPClient already converts exceptions into a failure
ApiResponse), but it would produce clearer failure semantics.Also applies to: 77-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java(3 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java(5 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java
- collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java
🧬 Code graph analysis (3)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(36-101)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (6)
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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorDataHolder.java (1)
UsageDataCollectorDataHolder(28-71)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/model/SystemUsage.java (1)
SystemUsage(24-69)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
HTTPClient(47-131)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(36-101)
🔇 Additional comments (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
36-60: Thin PublisherImp wrapper over HTTPClient looks good.Using
IdentityDatabaseUtil.getDataSource()and delegating bothcallReceiverApiandcallWso2Apidirectly toHTTPClient.executeApiRequest(...)keeps this implementation small and focused. The endpoint selection based onApiRequestand data type is clear and aligns with the shared publisher API model.
inthirakumaaran
force-pushed
the
is_usage_collector
branch
from
cfe75c4 to
975c290
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (1)
36-188: Singleton + credential loading/secret resolution look correct; only small optional refinements.
- Constructor‑triggered
init()with a static final instance gives one‑time initialization and avoids the earlier repeated‑init issue.init()handles missing config and IO errors gracefully and keepshasCredentials()safe (just returnsfalsewhen nothing is loaded).- Secret resolution via
SecretResolverFactory.create(properties)and updating resolved values back intopropertiesis consistent with typical Secure Vault usage.Two minor, optional suggestions:
- Consider logging only whether an app name is configured instead of the actual
appNamevalue to avoid leaking identifiers in DEBUG logs.- If this class might ever be re‑initialized or used in tests, you may want to document that
init()is not thread‑synchronized (or make it idempotent/thread‑safe) to avoid surprises under concurrent calls, even though current usage as a one‑time init is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java(3 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java(5 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java
- collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java
- collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java
🧬 Code graph analysis (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(36-101)
🔇 Additional comments (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
24-25: Properly tracking and cleaning up the Publisher service registration.Storing the
ServiceRegistration<?>forPublisherImpand unregistering it incleanup()fixes the earlier lifecycle gap and aligns the component with OSGi best practices for manually registered services. The null check andIllegalStateExceptionhandling make deactivation and error‑path cleanup robust across repeated activate/deactivate cycles.Also applies to: 36-37, 67-67, 88-92, 129-135
Fix Fix
inthirakumaaran
force-pushed
the
is_usage_collector
branch
from
975c290 to
c16a766
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
24-24: ServiceRegistration lifecycle handled; consider clearing reference after unregisterStoring the
ServiceRegistration<?>and unregistering it incleanup()correctly avoids a dangling OSGi service. As a minor follow-up, you could setpublisherServiceRegistration = nullafter a successfulunregister()to make reuse/re-activation semantics clearer and guard against accidental double use.Also applies to: 67-67, 129-135
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (1)
48-50: Prefer depending on thePublisherinterface instead ofPublisherImpRight now
UsageDataCollectorholds a concretePublisherImpfield. To keep this aligned with the common publisher abstraction and ease future swaps/mocking, consider declaring the field as thePublisherinterface and instantiating the concrete class only at construction time:-import org.wso2.carbon.usage.data.collector.identity.publisher.PublisherImp; +import org.wso2.carbon.usage.data.collector.common.publisher.api.Publisher; +import org.wso2.carbon.usage.data.collector.identity.publisher.PublisherImp; @@ - private final PublisherImp publisher; + private final Publisher publisher; @@ - this.orgCountCalculator = new OrganizationCounter(organizationManager); - this.publisher = new PublisherImp(); + this.orgCountCalculator = new OrganizationCounter(organizationManager); + this.publisher = new PublisherImp();This keeps the public surface tied to the shared API rather than a specific implementation.
Also applies to: 56-57, 64-64
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
93-101: Use UTF‑8 explicitly when encoding HTTP Basic Authentication credentialsThe code uses
toEncode.getBytes()without specifying a charset, which defaults to the platform's character encoding. This can vary across operating systems and JVM configurations, potentially causing authentication failures or interoperability issues. RFC 7617 recommends UTF‑8 encoding for HTTP Basic Authentication to ensure consistent behavior across platforms.- char[] appPasswordChars = credentialsUtil.getAppPassword(); - String toEncode = appName + ":" + new String(appPasswordChars); - byte[] encoding = Base64.encodeBase64(toEncode.getBytes()); + char[] appPasswordChars = credentialsUtil.getAppPassword(); + String toEncode = appName + ":" + new String(appPasswordChars); + byte[] encoding = Base64.encodeBase64(toEncode.getBytes(StandardCharsets.UTF_8)); String authHeader = new String(encoding, StandardCharsets.UTF_8);This ensures all credential handling uses a well-defined charset and avoids environment-dependent behavior.
Also applies to: 113-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java(3 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java(5 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java(1 hunks)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.javacollectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java
🧬 Code graph analysis (2)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(36-101)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (5)
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.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/model/SystemUsage.java (1)
SystemUsage(24-69)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
HTTPClient(47-131)collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/PublisherImp.java (1)
PublisherImp(36-101)
🔇 Additional comments (4)
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/internal/UsageDataCollectorServiceComponent.java (1)
36-36: Publisher service registration wiring looks correctUsing
PublisherImpas the implementation and registering it under the commonPublisherinterface inactivate()is consistent with the shared publisher API and keeps usage isolated to this component. No functional issues spotted here.Also applies to: 88-92
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/UsageDataCollector.java (1)
67-75: Metric publishing flow and response handling look solid
collectAndPublish()now pushes three aggregate metrics viapublishUsageMetrics()and still logs the human-readable report, andpublishMetric():
- Builds the request via
HTTPClient.createUsageDataRequest.- Checks
ApiResponse.isSuccess()before logging success/failure.- Treats network/serialization errors as non-fatal, logging them only at debug level, which matches the logging guidance for this module.
No blocking issues here; the behavior is consistent with the non-intrusive metrics collection approach.
Also applies to: 183-188, 190-208
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/util/AppCredentialsUtil.java (1)
38-57: Credential loading and secret resolution are structured appropriatelyThe singleton pattern with one-time
init()in the constructor, filesystem load ofRecoveryEndpointConfig.properties, and optional secure-vault resolution viaSecretResolverall look consistent and low-risk.hasCredentials()gives a clear guard for the HTTP client, and failures are only logged at debug, which aligns with the module’s logging guidance.No changes needed from my side here.
Also applies to: 69-127, 134-189
collectors/org.wso2.carbon.usage.data.collector.identity/src/main/java/org/wso2/carbon/usage/data/collector/identity/publisher/HTTPClient.java (1)
57-77: ApiRequest fields beyond payload/timeout are currently ignored
executeApiRequestalways issues a JSONPOSTand doesn't applyApiRequest'shttpMethod, headers, or query parameters. That matches current usage for usage-count posts, but callers cannot yet rely on those fields.If this client is intended to support broader use cases with multiple HTTP methods and custom headers, consider:
- Honoring
request.getHttpMethod()instead of hardcodingHttpPost.- Applying headers and query parameters from
ApiRequestbefore execution.This would make the HTTP client more aligned with a general-purpose publisher API.
|
There are a few hardcoded entries that need to be updated. As discussed with @inthirakumaaran he will update those places after discussing with the other stakeholders too. Approving the PR for now. |
2 participants
Purpose
Summary by CodeRabbit
New Features
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.