Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use android.util.Log for logging #120

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Use android.util.Log for logging #120

merged 3 commits into from
Jan 13, 2025

Conversation

wzieba
Copy link
Collaborator

@wzieba wzieba commented Jan 10, 2025

Description

This way, the SDK has a control over which logs will be used in production or debug variants of SDK users. As on outcome, this change reduces number of logs that the SDK is producing in production apps.

Closes: #83
Closes: #119

The #119 issue is a result of StringBuilder trying to allocate a significant part of available memory. This is probably a result of a case, in which consumer app tries to report many events: either due to device being offline for a long period or a misconfiguration of the SDK. This PR will stop logging whole payloads in production apps, what will likely address the issue.

Testing

Not necessary: both unit and functional tests pass. If still needed, one can open an example app and validate the logs via Logcat.

image

This way, the SDK has a control over which logs will be used in production or debug variants of SDK users. As on outcome, this change reduces number of logs that the SDK is producing in production apps.
Fixes "Method d in android.util.Log not mocked"
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.

Project coverage is 71.32%. Comparing base (47696a4) to head (a44209d).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...m/parsely/parselyandroid/ParselyTrackerInternal.kt 16.66% 5 Missing ⚠️
...m/parsely/parselyandroid/LocalStorageRepository.kt 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
+ Coverage   70.46%   71.32%   +0.86%     
==========================================
  Files          22       22              
  Lines         413      415       +2     
  Branches       51       50       -1     
==========================================
+ Hits          291      296       +5     
+ Misses        107      105       -2     
+ Partials       15       14       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wzieba wzieba requested a review from ParaskP7 January 10, 2025 15:27
@wzieba wzieba marked this pull request as ready for review January 10, 2025 15:27
Copy link
Collaborator

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @wzieba !

I have reviewed this PR as per the instructions (quickly tested it as well), everything works as expected, great job and much better logging indeed, awesome! 🌟

@wzieba wzieba merged commit d1c4b4f into main Jan 13, 2025
3 checks passed
@wzieba wzieba deleted the use_android_logging branch January 13, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fatal Exception: java.lang.OutOfMemoryError Use Android logger instead of system's output stream
2 participants