fix(android): refactor KMLog - simplification and added resilience#16152
fix(android): refactor KMLog - simplification and added resilience#16152mcdurdin wants to merge 2 commits into
KMLog - simplification and added resilience#16152Conversation
* Clean up `KMLog` -- simpler code paths, DRY out common validation, remove redundant re-entrancy checks. * Use Sentry `setTag` API and scopes instead of `setExtra`. * Wrap all potential failure points in exception handlers for extra resilience -- do our best to make sure errors are reported in as many cases as possible. Fixes: #16122 Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
| // trigger errors that can also trigger the same logging. We must not get | ||
| // caught in an infinite loop / stack-overflow; this field helps us avoid states | ||
| // that would otherwise cause error-looping, etc. | ||
| private static boolean isLogging = false; |
There was a problem hiding this comment.
AFAICT, there is no re-entrancy in this module so this can not arise (the only nested call was from LogExceptionWithData to LogException, and the pattern was actually wrong in this case, as noted in #16122!)
| @@ -169,28 +174,7 @@ public static void LogError(String tag, String msg) { | |||
| * @param e Throwable exception | |||
| */ | |||
| public static void LogException(String tag, String msg, Throwable e) { | |||
There was a problem hiding this comment.
I swapped LogException and LogExceptionWithData implementations -- simpler code path.
| } | ||
| isLogging = false; | ||
|
|
||
| Sentry.withScope(scope -> { |
ermshiperete
left a comment
ermshiperete
left a comment
There was a problem hiding this comment.
Great to see the KMLog improved and the problems fixed!
| String errorMsg = ""; | ||
| try { | ||
| if (msg != null && !msg.isEmpty()) { | ||
| errorMsg = msg + "\n" + e.toString(); |
There was a problem hiding this comment.
This will throw if e == null.
| errorMsg = msg + "\n" + e.toString(); | |
| errorMsg = msg + "\n" + e;; |
| if(obj != null && objName != null) { | ||
| Sentry.setTag(objName, obj.toString()); |
There was a problem hiding this comment.
From devin.ai:
Sentry tag values have a 200-character limit which may truncate data previously stored as extras
The PR changes Sentry.setExtra(...) to Sentry.setTag(...). Sentry tags are indexed and searchable but have a maximum value length of 200 characters, while extras have no such limit. This matters most for LogExceptionWithData at line 218 where obj.toString() is set as a tag — for example, KeyboardController.java:92-93 passes a JSON list as the obj parameter, which could easily exceed 200 characters and be silently truncated by Sentry. The old code used setExtra which would have preserved the full value.
There was a problem hiding this comment.
Yes, I was aware of this. I was trying to remove the need to setExtra and then removeExtra. The use cases for passing large chunks of extra data are pretty narrow. For now, I will restore just this one tag to extra data.
Also add logging if unit test fails
2 participants
KMLog-- simpler code paths, DRY out common validation, remove redundant re-entrancy checks.setTagAPI and scopes instead ofsetExtra.Fixes: #16122
Test-bot: skip