Removes some logs which are killing our sumo usage #2911
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple classes in the codebase, primarily focusing on logging behavior. In the In contrast, the Sequence Diagram(s)sequenceDiagram
participant User
participant DataPullTask
participant Server
User->>DataPullTask: Start Sync
DataPullTask->>DataPullTask: Log "Starting Sync"
DataPullTask->>DataPullTask: Check Server URL
alt URL is empty
DataPullTask-->>User: Return Error
else URL is valid
DataPullTask->>Server: Initiate Sync
Server-->>DataPullTask: Sync Response
DataPullTask-->>User: Return Sync Result
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/src/org/commcare/utils/FormUploadUtil.java (2)
Line range hint
42-43: Consider making MAX_BYTES configurableThe 15MB size limit is currently hardcoded. Consider making this configurable through build configuration or system properties to allow for environment-specific adjustments.
Line range hint
248-253: Address TODO comment about validation responsibilityThe TODO comment indicates that form record validation should be moved out of the submission process. This is a valid architectural concern as it violates the Single Responsibility Principle.
Consider:
- Moving the validation logic to a dedicated
FormValidatorclass- Making the validation a prerequisite step before submission
Would you like me to help create a separate
FormValidatorclass to handle this responsibility?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/src/org/commcare/android/javarosa/IntentCallout.java(0 hunks)app/src/org/commcare/appupdate/CommcareFlexibleAppUpdateManager.java(0 hunks)app/src/org/commcare/preferences/HiddenPreferences.java(0 hunks)app/src/org/commcare/tasks/DataPullTask.java(0 hunks)app/src/org/commcare/utils/FormUploadUtil.java(1 hunks)
💤 Files with no reviewable changes (4)
- app/src/org/commcare/android/javarosa/IntentCallout.java
- app/src/org/commcare/appupdate/CommcareFlexibleAppUpdateManager.java
- app/src/org/commcare/preferences/HiddenPreferences.java
- app/src/org/commcare/tasks/DataPullTask.java
🔇 Additional comments (1)
app/src/org/commcare/utils/FormUploadUtil.java (1)
364-371: LGTM! Conditional logging improves efficiency
The change to only log attachment counts when there are actual attachments is a good optimization that aligns with the PR's goal of reducing Sumo Logic usage while preserving important debugging information.
Let's verify that we're not losing any critical debugging information by checking other logging points in the upload flow:
✅ Verification successful
Let me check the context of these logging statements to ensure we have a complete picture of the form submission logging flow.
Conditional logging change is safe and maintains debugging capabilities
The verification shows that all critical form submission scenarios are still being logged:
- Network response issues and error details
- Unsupported file type warnings
- File size limitation warnings
- Form record removal notifications
- Form submission attempts and success tracking
The change to make attachment count logging conditional only eliminates noise when there are no attachments, while preserving all essential debugging information in the form upload flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other logging statements in the form upload flow to ensure critical debugging information is preserved
# Search for other logging statements in the form upload process
rg "Logger.log.*TYPE_FORM_SUBMISSION" --type java
Length of output: 1111
Script:
#!/bin/bash
# Get context around the logging statements to understand the complete logging flow
rg "Logger.log.*TYPE_FORM_SUBMISSION" --type java -B 2 -A 2
Length of output: 4419
| Logger.log(LogTypes.TYPE_CC_UPDATE, "Publishing status update to : " + newState.name() + ", from : " | ||
| + (mAppUpdateState != null ? mAppUpdateState.name() : "null")); |
There was a problem hiding this comment.
@shubham1g5 Are we 100% sure that this is unnecessary? We can consider logging when the state changes.
There was a problem hiding this comment.
This already logs only when state changes (look at the if cond above this). I don't think this gives us any useful debugging info as such as most functionality there is managed by Play services API. Also there is nothing like 100% certainity with what you might and might not need with logs and think we should avoid this framing in deciding what to log. Instead a better framing for retroactively deleting logs is have you found a particular line of useful while solving any debugging issues in past ? Happy to re-consider if so.
There was a problem hiding this comment.
Instead a better framing for retroactively deleting logs is have you found a particular line of useful while solving any debugging issues in past ?
Agree, my framing was exactly in this sense, I remember troubleshooting the app update issue and looking at these, but don't remember how useful they actually were.
Summary
Cleaning some not so useful logs which makes up for almost half of the log entries in sumologic in hopes to reduce our sumo usage by a good factor.
PR Checklist
Automated test coverage
NA
Safety story
Logs only