-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added trace ID to error events #251
Conversation
if (correlation != null) { | ||
|
||
String traceId = getString(correlation, "traceId"); |
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.
nit: formatting
if (correlation != null) { | |
String traceId = getString(correlation, "traceId"); | |
if (correlation != null) { | |
String traceId = getString(correlation, "traceId"); |
long traceIdMostSignificantBits = hexToLong(traceId.substring(0, HEX_LONG_LENGTH)); | ||
long traceIdLeastSignificantBits = hexToLong(traceId.substring(HEX_LONG_LENGTH)); | ||
long spanIdAsLong = hexToLong(spanId); |
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.
we should do some error-catching (NumberFormatException
at minimum) here, if (somehow) these aren't exactly what we expect we would crash on the hot-path... if the strings aren't hex encoded we can safely omit the entire correlation
element
@@ -442,4 +462,9 @@ JSONObject deliverEvent(@Nullable JSONObject eventJson) { | |||
boolean hasString(JSONObject args, String key) { | |||
return getString(args, key) != null; | |||
} | |||
|
|||
static long hexToLong(String hex) { |
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.
nit: this doesn't appear to be used outside of the BugsnagFlutter
class
static long hexToLong(String hex) { | |
private static long hexToLong(String hex) { |
event.setTraceCorrelation(new UUID(traceIdMostSignificantBits, traceIdLeastSignificantBits), spanIdAsLong); | ||
} | ||
} catch(Exception e) { | ||
Log.e("BugsnagFlutter", "correlation parsing", e); |
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 think that this can be reasonably dropped, logging to the device log just adds noise. This should never happen after-all.
Log.e("BugsnagFlutter", "correlation parsing", e); | |
// ignore the error, the error correlation will be missing |
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 would be good to see a CHANGELOG entry, and end-to-end test of the new functionality
78dc616
to
c596f82
Compare
Added an entry to Changelog. The e2e tests were added in bugsnag-flutter-performance |
* Added null checks for client in Android code * Improved the behaviour on Android when Bugsnag is started multiple times from different contexts * Bump bugsnag-android to v6. Changed redactedKeys and discardClasses types to RegExp (#249) * Bump bugsnag-android to v6. Changed redactedKeys and discardClasses types to RegExp * Regular expressions sent to native clients now carry RegExp flags * Update features/fixtures/app/lib/scenarios/start_bugsnag_scenario.dart Co-authored-by: Jason <lemnik@users.noreply.github.com> --------- Co-authored-by: Robert <robert.smartbear@gmail.com> Co-authored-by: Jason <lemnik@users.noreply.github.com> * docs(readme): amended badge URL to scope to main branch only (#241) Co-authored-by: Tom Longridge <tom.longridge@smartbear.com> * chore(build): update package gemfile (#240) Co-authored-by: Tom Longridge <tom.longridge@smartbear.com> * Rebuilt Flutter example and fixture with Flutter 3.10 (#252) * Rebuilt flutter example * Rebuilt e2e tests fixture * Small fixes for example project * Replaced examples with a single example for pub.dev release * Small fixes for the fixture * Updated mazerunner gem to v9 * Updated mazerunner to v9 on CI * Switched Android E2E tests to be run on Browserstack on CI * Added browserstack username and access key to docker-compose.yaml * adjust pipeline for browserstack * adjust pipeline for browserstack --------- Co-authored-by: Robert <robert.smartbear@gmail.com> Co-authored-by: Josh Edney <josh.edney@smartbear.com> * Added trace ID to error events (#251) * Added spanId and traceId from span context to events * Updated iOS integration and implemented Android integration for error correlation * Changes requested in code review * Made the package publishable again * Changes requested in code review --------- Co-authored-by: Robert <robert.smartbear@gmail.com> * Updated UPGRADING.MD for v4 release (#254) * Updated UPGRADING.MD for v4 release * Update UPGRADING.MD Co-authored-by: Tom Longridge <tom@bugsnag.com> --------- Co-authored-by: Robert <robert.smartbear@gmail.com> Co-authored-by: Tom Longridge <tom@bugsnag.com> * Bumped to v4.0.0 --------- Co-authored-by: Robert <robert.smartbear@gmail.com> Co-authored-by: Jason <lemnik@users.noreply.github.com> Co-authored-by: Tom Longridge <tom@bugsnag.com> Co-authored-by: Tom Longridge <tom.longridge@smartbear.com> Co-authored-by: Josh Edney <josh.edney@smartbear.com>
Goal
Include the ID of the current trace and span ID in errors when both bugsnag-flutter and bugsnag-flutter-performance are included in an application
Design
BugsnagContextProvider is used to receive traceId and spanId from Flutter Performance and then the values are included in bugsnag-cocoa and bugsnag-android events
Testing
Existing E2E tests