-
Notifications
You must be signed in to change notification settings - Fork 33
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
Crashes as logs #237
Crashes as logs #237
Conversation
StringWriter sw = new StringWriter(); | ||
PrintWriter pw = new PrintWriter(sw); | ||
|
||
throwable.printStackTrace(pw); | ||
pw.flush(); | ||
|
||
return sw.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason Timber allocates only 256 and flush manually, see https://github.com/JakeWharton/timber/blob/dfc4e70bd1a4b4e1e136647b96fc9c9dbcb41f65/timber/src/main/java/timber/log/Timber.kt#L175-L181
not sure there's any performance gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite interesting, the 256 initial buffer size seems related to performance indeed. I'm usually not keen on doing premature optimizations, though I think, based on how popular Timber is, I guess it's worth following their approach.
Regarding the and flush manually
part I'm a bit confused because it seems like that's the default behavior of PrintWriter
, so I'm not sure why they had to use that constructor if they didn't need to change the defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the 256 initial size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably because you call flush
manually anyway? so makes sense that the autoFlush=false, unless the default is false already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the default is false, that's why I'm a little confused about the code in Timber 😅
StringWriter sw = new StringWriter(); | ||
PrintWriter pw = new PrintWriter(sw); | ||
|
||
throwable.printStackTrace(pw); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the printStackTrace
also stringify all the inner exceptions? if not, we'd need to iterate the cause recursively and append it in a string builder maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it prints the "Caused by: ..." parts too.
Attributes.builder() | ||
.put(SemanticAttributes.EXCEPTION_ESCAPED, true) | ||
.put(SemanticAttributes.THREAD_ID, thread.getId()) | ||
.put(SemanticAttributes.THREAD_NAME, thread.getName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth considering adding a new attribute if it is the main thread or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm actually not sure if the concept of "main thread" is as relevant in every other platform as it is in Android in order to have it as a general thread attr, but I guess we can always try and check what people think about it. If you have some use cases in mind that would benefit from this attr, I think you could mention them in an issue in this repo, or maybe just create a draft adding the new attr in this file, is usually easier to convey ideas with code changes rather than only with a description in an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, will do, just for context, exception triage, IMO main thread errors are most likely more important than background errors (if the thread is a daemon) maybe the app won't even crash, but not sure if in this case the default signal handler would be called anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's the same thing but here there's thread id already.
Related to #212