Logs all network requests to Analytics #3352
Conversation
shubham1g5
requested review from
Jignesh-dimagi,
OrangeAndGreen and
avazirna
| import retrofit2.Retrofit | ||
| import java.util.concurrent.TimeUnit | ||
|
|
||
| object ConnectNetworkServiceFactory { |
There was a problem hiding this comment.
this was unused.
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Code
participant OkHttp as OkHttpClient
participant NAI as NetworkAnalyticsInterceptor
participant Svc as Remote Service
participant FA as FirebaseAnalyticsUtil
participant FBA as Firebase Analytics
Note over App,OkHttp: High-level request flow with analytics logging
App->>OkHttp: execute(request)
OkHttp->>NAI: intercept(request)
activate NAI
Note over NAI: start timer
NAI->>Svc: proceed(request)
Svc-->>NAI: response (code)
Note over NAI: compute duration
NAI->>FA: reportNetworkRequest(url, code, duration)
activate FA
FA->>FBA: logEvent(NETWORK_REQUEST, {url, code, durationMs})
deactivate FA
NAI-->>OkHttp: response
deactivate NAI
OkHttp-->>App: response
alt Error during request
NAI->>FA: reportNetworkRequest(url, errorCode, duration)
FA->>FBA: logEvent(...)
NAI-->>OkHttp: propagate exception
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (2)
app/src/org/commcare/network/NetworkAnalyticsInterceptor.kt (1)
11-27: Consider simplifying response type.The implementation correctly handles request failures by logging
responseCode = -1in thefinallyblock. However,responseis declared asResponse?but is never actually null when returned (line 22). Ifchain.proceed()throws an exception, execution jumps directly to thefinallyblock without reaching the return statement.Consider this simpler approach:
override fun intercept(chain: Interceptor.Chain): Response { val request = chain.request() val startTime = System.currentTimeMillis() - var response: Response? var responseCode = -1 try { - response = chain.proceed(request) + val response = chain.proceed(request) responseCode = response.code return response } finally { val duration = System.currentTimeMillis() - startTime logNetworkAnalytics(request.url.toString(), responseCode, duration) } }app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
587-593: Consider rate limiting to manage event volume and costs.Logging every network request without rate limiting could generate very high event volumes, impacting:
- Firebase Analytics quotas and costs
- Analytics dashboard performance
- Data retention and query performance
Apply rate limiting similar to other high-frequency events:
public static void reportNetworkRequest(String url, int responseCode, long durationMs) { + if (!rateLimitReporting(0.1)) { // Log 10% of requests + return; + } Bundle bundle = new Bundle(); bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode); bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs); bundle.putString(CCAnalyticsParam.URL, url); reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle); }Alternatively, consider logging only failed requests (non-2xx status codes) or sampling based on response code ranges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/org/commcare/connect/network/ConnectNetworkServiceFactory.kt(0 hunks)app/src/org/commcare/connect/network/base/BaseApiClient.kt(3 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java(1 hunks)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(1 hunks)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java(2 hunks)app/src/org/commcare/network/NetworkAnalyticsInterceptor.kt(1 hunks)app/src/org/commcare/network/OkHttpBuilderCustomConfig.java(2 hunks)
💤 Files with no reviewable changes (1)
- app/src/org/commcare/connect/network/ConnectNetworkServiceFactory.kt
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (3)
app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java (1)
AnalyticsParamValue(7-187)app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
CCAnalyticsParam(7-47)app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
CCAnalyticsEvent(7-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (8)
app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java (1)
58-58: LGTM!The new
NETWORK_REQUESTevent constant follows the established naming convention and is properly positioned with other analytics events.app/src/org/commcare/network/OkHttpBuilderCustomConfig.java (2)
11-12: LGTM!Javadoc accurately reflects the updated functionality.
25-25: LGTM!The
NetworkAnalyticsInterceptoris properly instantiated and added to the OkHttp client. The placement after CT and certificate configuration is appropriate.app/src/org/commcare/connect/network/base/BaseApiClient.kt (3)
8-8: LGTM!Import is properly added.
49-49: Minor formatting change.Extra newline added for consistency.
26-26: Verify custom config usage
I didn’t find any references toOkHttpBuilderCustomConfigorperformCustomConfiginBaseApiClient.kt; please confirm if a shared custom OkHttp configuration is applied when constructing the client—if not, interceptor duplication isn’t an issue.app/src/org/commcare/network/NetworkAnalyticsInterceptor.kt (1)
29-39: LGTM! Excellent error handling.The try-catch block properly isolates analytics logging failures from the request flow, ensuring that analytics issues don't break network requests. Logging failures are appropriately reported via
Logger.exception().app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
3-12: LGTM! Explicit imports improve clarity.The refactor from wildcard imports to explicit static imports for
AnalyticsParamValueconstants improves code clarity and maintainability.
| static final String RESPONSE_CODE = "response_code"; | ||
| static final String REQUEST_DURATION_MS = "request_duration_ms"; | ||
| static final String URL = "url"; |
There was a problem hiding this comment.
🧩 Analysis chain
Consider URL sanitization for privacy.
The URL parameter will log complete request URLs, which may contain sensitive information such as authentication tokens, API keys, user IDs, or PII in query parameters or path segments. Consider sanitizing URLs before logging them to analytics.
Example sanitization approaches:
- Strip query parameters entirely
- Redact path segments that contain IDs or sensitive data
- Use allowlist of safe parameters/paths
static final String RESPONSE_CODE = "response_code";
static final String REQUEST_DURATION_MS = "request_duration_ms";
-static final String URL = "url";
+static final String URL = "url"; // Note: URLs should be sanitized before logging to remove sensitive dataYou may want to implement sanitization in NetworkAnalyticsInterceptor.logNetworkAnalytics() or FirebaseAnalyticsUtil.reportNetworkRequest().
🏁 Script executed:
#!/bin/bash
# Locate references to the URL analytics parameter
rg -n "CCAnalyticsParam\.URL" -C3
# Find and inspect the logNetworkAnalytics method
rg -n "logNetworkAnalytics" -C3
# Find and inspect the reportNetworkRequest method
rg -n "reportNetworkRequest" -C3Length of output: 3419
Sanitize URLs before logging
Both NetworkAnalyticsInterceptor.logNetworkAnalytics(request.url.toString(),…) and FirebaseAnalyticsUtil.reportNetworkRequest(url,…) send raw URLs to analytics, risking exposure of tokens, PII, or sensitive query parameters. Strip or redact sensitive data—e.g., remove query strings entirely, mask path segments containing IDs, or whitelist safe parameters—before logging.
There was a problem hiding this comment.
+1, might need to remove PII or token
There was a problem hiding this comment.
@Jignesh-dimagi Thank you for confirming! URL sanitization is indeed important to prevent exposing sensitive data in analytics.
If you'd like, I can help design the sanitization logic. Common approaches include:
- Strip all query parameters — remove everything after
? - Redact path segments — replace UUIDs, IDs, tokens with placeholders like
[REDACTED] - Domain + path pattern only — log just the domain and endpoint structure (e.g.,
/api/v1/users/[ID]/profile)
Would you like me to suggest specific implementation code for the NetworkAnalyticsInterceptor or FirebaseAnalyticsUtil to handle this sanitization?
| public static void reportNetworkRequest(String url, int responseCode, long durationMs) { | ||
| Bundle bundle = new Bundle(); | ||
| bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode); | ||
| bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs); | ||
| bundle.putString(CCAnalyticsParam.URL, url); | ||
| reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle); | ||
| } |
There was a problem hiding this comment.
Critical: URLs may contain sensitive data (auth tokens, PII, PHI).
Logging full URLs to analytics can expose:
- Authentication tokens in query parameters or headers
- User identifiers, session IDs, or personally identifiable information
- Protected health information (PHI) in paths or parameters
- Internal server paths or API structure
Additionally, Firebase Analytics truncates parameter values to 100 characters (line 61-62), which may:
- Still leak partial sensitive data
- Render URLs incomplete and less useful for debugging
- Create compliance risks (GDPR, CCPA, HIPAA)
Recommendations:
- Sanitize URLs before logging - remove query parameters, hash sensitive path segments, or use URL patterns:
public static void reportNetworkRequest(String url, int responseCode, long durationMs) {
Bundle bundle = new Bundle();
bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode);
bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs);
- bundle.putString(CCAnalyticsParam.URL, url);
+ bundle.putString(CCAnalyticsParam.URL, sanitizeUrl(url));
reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle);
}
+
+private static String sanitizeUrl(String url) {
+ try {
+ java.net.URL parsed = new java.net.URL(url);
+ // Return only protocol, host, and path (no query params)
+ return parsed.getProtocol() + "://" + parsed.getHost() + parsed.getPath();
+ } catch (Exception e) {
+ return "invalid_url";
+ }
+}-
Consider logging only the endpoint pattern (e.g.,
/api/v1/syncinstead of full URL). -
Apply rate limiting similar to other high-frequency events (see
reportFormNavat line 222).
📝 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 static void reportNetworkRequest(String url, int responseCode, long durationMs) { | |
| Bundle bundle = new Bundle(); | |
| bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode); | |
| bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs); | |
| bundle.putString(CCAnalyticsParam.URL, url); | |
| reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle); | |
| } | |
| public static void reportNetworkRequest(String url, int responseCode, long durationMs) { | |
| Bundle bundle = new Bundle(); | |
| bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode); | |
| bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs); | |
| bundle.putString(CCAnalyticsParam.URL, sanitizeUrl(url)); | |
| reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle); | |
| } | |
| private static String sanitizeUrl(String url) { | |
| try { | |
| java.net.URL parsed = new java.net.URL(url); | |
| // Return only protocol, host, and path (no query params) | |
| return parsed.getProtocol() + "://" + parsed.getHost() + parsed.getPath(); | |
| } catch (Exception e) { | |
| return "invalid_url"; | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
around lines 587 to 593, do not log full raw URLs (they may contain auth tokens,
PII/PHI and Firebase truncation can still leak data); instead sanitize the URL
before building the Bundle by stripping query parameters and fragments,
replacing sensitive path segments with generic placeholders or a deterministic
hash, and/or mapping the URL to an endpoint pattern (e.g., "/api/v1/sync"); then
log only the sanitized pattern or hashed segment plus the responseCode and
durationMs; also apply the existing rate-limiting approach used by reportFormNav
to limit frequency of NETWORK_REQUEST events.
There was a problem hiding this comment.
@Jignesh-dimagi Thank you for acknowledging this security concern!
Given the critical nature of potentially exposing sensitive data (auth tokens, PII, PHI) through URL logging, I wanted to check: are you planning to address the URL sanitization in this PR, or would you like me to create a follow-up issue to track this separately?
If you'd like an issue created, I can include a reference to this discussion and the suggested sanitization approach.
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| val request = chain.request() | ||
| val startTime = System.currentTimeMillis() | ||
| var response: Response? | ||
| var responseCode = -1 | ||
|
|
||
| try { | ||
| response = chain.proceed(request) | ||
| responseCode = response.code | ||
| return response | ||
| } finally { | ||
| val duration = System.currentTimeMillis() - startTime | ||
| logNetworkAnalytics(request.url.toString(), responseCode, duration) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify FirebaseAnalyticsUtil.reportNetworkRequest implementation.
The interceptor will invoke FirebaseAnalyticsUtil.reportNetworkRequest() for every HTTP request in the app. Ensure this method:
- Sanitizes URLs to remove sensitive data (tokens, PII, etc.)
- Handles high request volumes efficiently (async/batched)
- Respects analytics opt-out preferences if applicable
🏁 Script executed:
#!/bin/bash
# Find and examine the FirebaseAnalyticsUtil.reportNetworkRequest implementation
# Search for reportNetworkRequest method definition
ast-grep --pattern $'class FirebaseAnalyticsUtil {
$$$
reportNetworkRequest($$$) {
$$$
}
$$$
}'
# Also search with ripgrep as fallback
rg -n -A 10 'fun reportNetworkRequest|void reportNetworkRequest|public.*reportNetworkRequest' --type=java --type=kotlinLength of output: 1100
Enhance reportNetworkRequest to sanitize URLs, use async/batching, and respect opt-out
FirebaseAnalyticsUtil.java:587 currently logs full URLs, fires each event synchronously, and ignores analytics settings. Update it to:
- Mask or remove sensitive query parameters before logging
- Dispatch events off the calling thread or batch them to avoid overhead
- Check and honor user analytics opt-out before reporting
🤖 Prompt for AI Agents
In app/src/org/commcare/network/NetworkAnalyticsInterceptor.kt around lines 13
to 27, the interceptor currently logs full URLs synchronously and ignores user
analytics settings; update it to (1) sanitize the request URL before logging by
removing or masking sensitive query parameters (e.g., password, token, auth,
ssn, session, access_token) or stripping the query string entirely when present,
(2) dispatch the analytics call off the network thread—use a background
coroutine, executor, or a batching queue to send events asynchronously or
aggregate events and flush periodically to avoid per-request overhead, and (3)
check the application’s analytics/opt-out setting before sending (call the
existing analytics opt-out API or a central AnalyticsUtil.isEnabled method) and
skip reporting if the user has disabled analytics.
| /** | ||
| * OkHttp interceptor that logs analytics events for all HTTP requests/responses | ||
| */ | ||
| class NetworkAnalyticsInterceptor : Interceptor { |
There was a problem hiding this comment.
@shubham1g5 We are trying to log all network calls, which might be available in AWS EC2 instance with corresponding delays. These might be too much data to handle and might not be useful at the end?
There was a problem hiding this comment.
I think we are making a shift to log more and more data to analytics (now that we don't have sumologic) , so I would not be worried about this data size untill it starts creating practical issues for us.
I am also not sure how will we filter what requests we want to log and what not.
| static final String RESPONSE_CODE = "response_code"; | ||
| static final String REQUEST_DURATION_MS = "request_duration_ms"; | ||
| static final String URL = "url"; |
There was a problem hiding this comment.
+1, might need to remove PII or token
| public static void reportNetworkRequest(String url, int responseCode, long durationMs) { | ||
| Bundle bundle = new Bundle(); | ||
| bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode); | ||
| bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs); | ||
| bundle.putString(CCAnalyticsParam.URL, url); | ||
| reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle); | ||
| } |
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Left a few ideas but happy to save any/all for future work so we can get this in to 2.60
| static final String PERSONAL_ID_CONFIGURATION_FAILURE = "personal_id_configuration_failure"; | ||
| static final String NAV_DRAWER_OPEN = "nav_drawer_open"; | ||
| static final String NAV_DRAWER_ITEM_SELECTED = "nav_drawer_item_selected"; | ||
| static final String NETWORK_REQUEST = "network_request"; |
| @@ -40,4 +40,8 @@ public class CCAnalyticsParam { | |||
| static final String PARAM_API_SUCCESS = "ccc_api_success"; | |||
| static final String PARAM_API_TOTAL_JOBS = "ccc_api_total_jobs"; | |||
| static final String PARAM_API_NEW_JOBS = "ccc_api_new_jobs"; | |||
There was a problem hiding this comment.
Noting the overlap with the ccc_api_* events. These can log additional info in some cases though, such as number of new jobs received. I think fine to leave them all in for now, but I wonder if there's a future world we could get to where network_request events could be the single event and optionally include additional analytics info depending on the call? Recognizing this may be difficult with the more-generic NetworkAnalyticsInterceptor...
There was a problem hiding this comment.
agree, I think based on the overall feedback , I am going to move away from interceptor approach and instead switch to have this logic as part of generalised network callbacks.
| return response | ||
| } finally { | ||
| val duration = System.currentTimeMillis() - startTime | ||
| logNetworkAnalytics(request.url.toString(), responseCode, duration) |
There was a problem hiding this comment.
Noting that any exceptions sending the request will result in a -1 responseCode with no additional exception info. I wonder if we should attempt to distinguish the various possible exceptions with different negative error codes?
| ) { | ||
| try { | ||
| FirebaseAnalyticsUtil.reportNetworkRequest(url, responseCode, duration) | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
+1 to adding safety to our analytics calls and silently reporting exceptions, I think we've seen a crash or two come from such calls in the past. But I think we should make it comprehensive, rather than the caller's responsibility as done here.
Looking in FirebaseAnalyticsUtil it seems we may be close to accomplishing this already.
- reportEvent(String,String[],String[]) is wrapped in a try-catch
- reportEvent(String,Bundle) is not
The first calls the second, but there are 16 additional usages of the second. Seems a little cleanup here would globally guarantee safety for analytics calls.
There was a problem hiding this comment.
thanks for flagging, I was under assumption that all calls are fail safe but I can get that in as part of this work.
|
@Jignesh-dimagi @OrangeAndGreen Actually realised that we are already logging failed responses to the Firebase as soft exceptions and my primary use case for this PR was for debugging. As such, I no longer think it's useful to log these in Analytics. Thoughts ? |
|
Changed my approach here to instead log better failures in crashlytics, closing this one out for now and can revisit if we need it later. |
Product Description
Logging request urls and corresponding response codes for all requests mobile is making
Labels and Review