-
Notifications
You must be signed in to change notification settings - Fork 48
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
Modify error reporting for API >= 26 #472
Conversation
Codecov Report
@@ Coverage Diff @@
## master #472 +/- ##
============================================
- Coverage 68.41% 68.26% -0.16%
- Complexity 394 397 +3
============================================
Files 69 70 +1
Lines 2159 2174 +15
Branches 172 172
============================================
+ Hits 1477 1484 +7
- Misses 584 591 +7
- Partials 98 99 +1 |
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.
Sorry about the renames, but we will be exposing the ErrorReporter interface to other SDKs and 'Crash' just sounds so scary
libtelemetry/src/full/java/com/mapbox/android/telemetry/MapboxTelemetry.java
Show resolved
Hide resolved
libtelemetry/src/full/java/com/mapbox/android/telemetry/crash/CrashReporter.java
Outdated
Show resolved
Hide resolved
libtelemetry/src/full/java/com/mapbox/android/telemetry/crash/CrashReporter.java
Outdated
Show resolved
Hide resolved
crashReporter.sendErrorReports(); | ||
} | ||
}); | ||
} catch (Throwable throwable) { |
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.
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.
Reviewed together with @alfwatt .
@Guardiola31337 @pengdev I'd love your reviews from the Nav and Maps SDK sides, respectively. |
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.
LGTM 🚀
It would be great to have some more details in the description about the bug being solved.
And the ultimate solution would be using WorkManager
, right? (regardless of the binary size impact).
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.
A couple of post-commit comments @nkukday
* This is a background job that sends crash events to the telemetry endpoint | ||
* at startup. | ||
*/ | ||
public final class CrashReporterJobIntentService extends JobIntentService { |
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.
Noting that this is technically breaking SemVer as it's a public
class which is not under internal
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.
@nkukday though they arrived after merging the PR - did you have a chance to address @Guardiola31337's comments, maybe as tail work?
@@ -1,4 +1,4 @@ | |||
package com.mapbox.android.telemetry.crash; |
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.
Noting that this is technically breaking Semver as packages of a public
class changed
# Resolve Conflicts: # libtelemetry/src/androidTestFull/java/com/mapbox/android/telemetry/errors/ErrorReporterEngineInstrumentedTest.java # libtelemetry/src/full/java/com/mapbox/android/telemetry/MapboxTelemetry.java # libtelemetry/src/full/java/com/mapbox/android/telemetry/crash/CrashReporterJobIntentService.java
* Handle null input while creating LocationEngineResult (#469) # Resolve Conflicts: # liblocation/src/main/java/com/mapbox/android/core/location/LocationEngineResult.java * Modify error reporting for API >= 26 (#472) Co-authored-by: Neeraja Kukday <[email protected]>
We are experiencing crashes within
JobIntentService
on and above API 26. Linking the Google issue here. As a workaround, we will continue to useJobIntentService
below API 26 (Android O) and use a network request to report errors on or above API 26.