-
Notifications
You must be signed in to change notification settings - Fork 35
Version 3.4.0 - attachment and background thread support, prevent false-positive anrs #69
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
Conversation
* Background thread support * Fixed exception flow * Removed renamed file
…ng them (#66) * Detect false-positive ANRs report and prevent ANR watchdog from sending them + anr thread improvements * Fixed typos + better documentation - added Samy's advices
* Attachment support on Android - iOS shouldn't compile * Updated the latest version of backtrace-android libraries and also adjust iOS integration code - code should now compile on iOS * Native attachment support * Attachment improvements * Updated label * Native client updates * Android libraries
| @@ -1,5 +1,12 @@ | |||
| # Backtrace Unity Release Notes | |||
|
|
|||
| ## Version 3.4.0 | |||
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.
3.4.0-rc1 ?
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-catch I will update a version.
jasoncdavis0
left a comment
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.
suggest we clarify 3.4.0-rc1 as the version.
|
Tested on iOS
|
CHANGELOG.md
Outdated
| # Backtrace Unity Release Notes | ||
|
|
||
| ## Version 3.4.0-rc1 | ||
| - native/managed attachment support - user can add path to attachments via BacktraceClient Initialize method or via Unity Editor Backtrace Window. Once user add them on the initialization method/game object - it will be available in every report that BacktraceClient will send to Backtrace. |
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 to be too pedantic here, but you should change "Unity Editor Backtrace Window" to "Unity Editor in the BacktraceClient's inspector. In Unity: Inspector <> Window.
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.
thanks, Will make a change.
| foreach (var path in AttachmentPaths) | ||
| { | ||
| result.Add(ClientPathHelper.GetFulLPath(path)); | ||
| } |
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.
@konraddysput Corner case you should handle (sorry if I missed it elsewhere: Check for null / empty items in the AttachmentPaths list. Given the user can create that list + amend it via the inspector, they could accidentally leave one of the items empty. I dont think this will throw a null reference exception in that case because I am pretty sure the list would just contain an empty string, but might be worth handling (or at least testing). Let me know what you think
Might make sense to put this in the Initialize method when you assign the value in the config to the client's AttachmentPath property instead. Ill leave that up to you.
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.
User attachment never will be null. Reference:
- user attachments:
if (AttachmentPaths == null || AttachmentPaths.Length == 0) - report attachments:
AttachmentPaths = attachmentPaths ?? new List<string>();
If the list is empty in both examples - it's fine. If list has empty values it's also fine - we will check if file exists before uploading it to Backtrace. We wont want to check if file exists here in this function because user might want to create them after backtrace client initialization.
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.
Ok great, makes sense. Thanks
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.
@Dautery91 it won't cost us anything to anyway prevents from setting empty strings in editor. I will add anyway a check on the startup. Thanks for pointing this out.
| AndroidJNI.NewStringUTF("error.type"), | ||
| AndroidJNI.NewStringUTF("Hang")); |
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.
[General Note. Making a change not necessary for this pull request] Reading through this made me think: Why are your attribute strings defined in line like this? In your editor code, you have an entire file where you define all of the Labels in one place, and yet these are in line. Seems like following a similar pattern would be a good change both from the perspective of organization AND maintaining documentation. If you have all of the attribute definitions in one place, would be easier for us to ensure any docs are up to date + give users a place in the code to see all of the attributes + quickly link to how they are used. Let's discuss this as an aside
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.
It should be defined in const variables in class definition I agree - but this change might be important/usefull once we will merge implementations for native clients. I think it't an item on my to do list while we will work on native client refactor.
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.
Ok sounds good. Yeah I view this change as primarily a documentation improvement than anything else, so we should just be keeping this in the back of our minds + slot in the work when and where it makes sense.
@vlussenburg
Dautery91
left a comment
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.
See my comments
* Fixed invalid date in log manager when calendar is unsupported * Use single method to generate UTC timestamps + move it from extension code to static class to avoid unnecessary allocation * Fixed line endings * Updated native library * Removed debug logs from native library * Updated library version * Added invariant culture to rest of the code * native library update * Fixed issue in the readme file
* Correct attachment name * Moved constants to const variables

Update 3.4.0-rc summary
thread.mainattribute support - attribute stores an identifier of a main thread. In this situation user can use value available in this attribute to detect main thread._mod_faulting_tidattribute for ANR reports to set default faulting thread.