Merge pull request #493 from bugsnag/robin/add-additional-metadata-to…

…-error-breadcrumbs (feat) Increased detail in handled event breadcrumb
bugsnag · Mar 30, 2020 · 3d5639a · 3d5639a
2 parents 22fc982 + b7a6ea6
commit 3d5639a
Show file tree

Hide file tree

Showing 10 changed files with 163 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -188,6 +188,9 @@ Bugsnag Notifiers on other platforms.
   `locale`) that were missing from the OOM reports.
   [#444](https://github.com/bugsnag/bugsnag-cocoa/pull/444)
 
+* Increased the detail in handled event breadcrumbs
+  [#493](https://github.com/bugsnag/bugsnag-cocoa/pull/493)
+
 ## 5.23.0 (2019-12-10)
 
 This release removes support for reporting 'partial' or 'minimal' crash reports

diff --git a/Source/BugsnagBreadcrumb.m b/Source/BugsnagBreadcrumb.m
@@ -95,10 +95,12 @@ - (NSDictionary *)objectValue {
                 metadata[[key copy]] = [_metadata[key] copy];
             }
             return @{
-                 BSGKeyMessage : [_message copy],
-                 BSGKeyTimestamp : timestamp,
-                 BSGKeyType : BSGBreadcrumbTypeValue(_type),
-                 BSGKeyMetadata : metadata
+                // Note: The Bugsnag Error Reporting API specifies that the breadcrumb "message"
+                // field should be delivered in as a "name" field.  This comment notes that variance.
+                BSGKeyName : [_message copy],
+                BSGKeyTimestamp : timestamp,
+                BSGKeyType : BSGBreadcrumbTypeValue(_type),
+                BSGKeyMetadata : metadata
             };
         }
         return nil;

diff --git a/Source/BugsnagClient.m b/Source/BugsnagClient.m
@@ -275,6 +275,7 @@ @interface BugsnagConfiguration ()
 @interface BugsnagEvent ()
 @property(readonly, copy, nonnull) NSDictionary *overrides;
 @property(readwrite) NSUInteger depth;
+@property(readonly, nonnull) BugsnagHandledState *handledState;
 @end
 
 @interface BugsnagMetadata ()
@@ -636,6 +637,8 @@ - (void)setupConnectivityListener {
     }];
 }
 
+// MARK: - Notify
+
 - (void)notifyError:(NSError *)error
               block:(void (^)(BugsnagEvent *))block {
     BugsnagHandledState *state =
@@ -744,16 +747,18 @@ - (void)notify:(NSException *)exception
         [self.sessionTracker handleHandledErrorEvent];
     }
 
-    BugsnagEvent *report = [[BugsnagEvent alloc]
+    BugsnagEvent *event = [[BugsnagEvent alloc]
         initWithErrorName:exceptionName
              errorMessage:message
             configuration:self.configuration
                  metadata:[self.configuration.metadata toDictionary]
              handledState:handledState
                   session:self.sessionTracker.runningSession];
+
     if (block) {
-        block(report);
+        block(event);
     }
+
     //    We discard 5 stack frames (including this one) by default,
     //    and sum that with the number specified by report.depth:
     //
@@ -764,28 +769,36 @@ - (void)notify:(NSException *)exception
     //    3 -[BugsnagCrashSentry reportUserException:reason:]
     //    4 -[BugsnagClient notify:message:block:]
 
-    int depth = (int)(BSGNotifierStackFrameCount + report.depth);
+    int depth = (int)(BSGNotifierStackFrameCount + event.depth);
 
-    NSString *reportName =
-        report.errorClass ?: NSStringFromClass([NSException class]);
-    NSString *reportMessage = report.errorMessage ?: @"";
+    NSString *eventErrorClass = event.errorClass ?: NSStringFromClass([NSException class]);
+    NSString *eventMessage = event.errorMessage ?: @"";
 
-    [self.crashSentry reportUserException:reportName
-                                   reason:reportMessage
+    [self.crashSentry reportUserException:eventErrorClass
+                                   reason:eventMessage
                         originalException:exception
                              handledState:[handledState toJson]
                                  appState:[self.state toDictionary]
-                        callbackOverrides:report.overrides
-                                 metadata:[report.metadata copy]
+                        callbackOverrides:event.overrides
+                                 metadata:[event.metadata copy]
                                    config:[self.configuration.config toDictionary]
                              discardDepth:depth];
-
+
+    // A basic set of event metadata
+    NSMutableDictionary *metadata = [@{
+        BSGKeyErrorClass : eventErrorClass,
+        BSGKeyUnhandled : [[event handledState] unhandled] ? @YES : @NO,
+        BSGKeySeverity : BSGFormatSeverity(event.severity)
+    } mutableCopy];
+
+    // Only include the eventMessage if it contains something
+    if (eventMessage && [eventMessage length] > 0) {
+        [metadata setValue:eventMessage forKey:BSGKeyName];
+    }
+
     [self addAutoBreadcrumbOfType:BSGBreadcrumbTypeError
-                      withMessage:reportName
-                      andMetadata:@{
-                          BSGKeyMessage : reportMessage,
-                          BSGKeySeverity : BSGFormatSeverity(report.severity)
-                      }];
+                      withMessage:eventErrorClass
+                      andMetadata:metadata];
 
     [self flushPendingReports];
 }

diff --git a/Source/BugsnagEvent.m b/Source/BugsnagEvent.m
@@ -627,7 +627,6 @@ - (void)setOverrideProperty:(NSString *)key value:(id)value {
         }
         _overrides = metadata;
     }
-
 }
 
 - (NSDictionary *)toJson {

diff --git a/Source/BugsnagHandledState.m b/Source/BugsnagHandledState.m
@@ -19,10 +19,10 @@ BSGSeverity BSGParseSeverity(NSString *severity) {
 
 NSString *BSGFormatSeverity(BSGSeverity severity) {
     switch (severity) {
-    case BSGSeverityInfo:
-        return BSGKeyInfo;
     case BSGSeverityError:
         return BSGKeyError;
+    case BSGSeverityInfo:
+        return BSGKeyInfo;
     case BSGSeverityWarning:
         return BSGKeyWarning;
     }

diff --git a/Tests/BugsnagBreadcrumbsTest.m b/Tests/BugsnagBreadcrumbsTest.m
@@ -126,9 +126,9 @@ - (void)testArrayValue {
         XCTAssertTrue([[formatter dateFromString:item[@"timestamp"]]
                        isKindOfClass:[NSDate class]]);
     }
-    XCTAssertEqualObjects(value[0][@"message"], @"Launch app");
-    XCTAssertEqualObjects(value[1][@"message"], @"Tap button");
-    XCTAssertEqualObjects(value[2][@"message"], @"Close tutorial");
+    XCTAssertEqualObjects(value[0][@"name"], @"Launch app");
+    XCTAssertEqualObjects(value[1][@"name"], @"Tap button");
+    XCTAssertEqualObjects(value[2][@"name"], @"Close tutorial");
 }
 
 - (void)testStateType {
@@ -141,7 +141,7 @@ - (void)testStateType {
     awaitBreadcrumbSync(self.crumbs);
     NSArray *value = [crumbs arrayValue];
     XCTAssertEqualObjects(value[0][@"metaData"][@"direction"], @"right");
-    XCTAssertEqualObjects(value[0][@"message"], @"Rotated Menu");
+    XCTAssertEqualObjects(value[0][@"name"], @"Rotated Menu");
     XCTAssertEqualObjects(value[0][@"type"], @"state");
 }
 
@@ -150,13 +150,13 @@ - (void)testPersistentCrumbManual {
     NSArray *value = [NSJSONSerialization JSONObjectWithData:crumbs options:0 error:nil];
     XCTAssertEqual(value.count, 3);
     XCTAssertEqualObjects(value[0][@"type"], @"manual");
-    XCTAssertEqualObjects(value[0][@"message"], @"Launch app");
+    XCTAssertEqualObjects(value[0][@"name"], @"Launch app");
     XCTAssertNotNil(value[0][@"timestamp"]);
     XCTAssertEqualObjects(value[1][@"type"], @"manual");
-    XCTAssertEqualObjects(value[1][@"message"], @"Tap button");
+    XCTAssertEqualObjects(value[1][@"name"], @"Tap button");
     XCTAssertNotNil(value[1][@"timestamp"]);
     XCTAssertEqualObjects(value[2][@"type"], @"manual");
-    XCTAssertEqualObjects(value[2][@"message"], @"Close tutorial");
+    XCTAssertEqualObjects(value[2][@"name"], @"Close tutorial");
     XCTAssertNotNil(value[2][@"timestamp"]);
 }
 
@@ -170,7 +170,7 @@ - (void)testPersistentCrumbCustom {
     NSArray *value = [NSJSONSerialization JSONObjectWithData:crumbs options:0 error:nil];
     XCTAssertEqual(value.count, 4);
     XCTAssertEqualObjects(value[3][@"type"], @"state");
-    XCTAssertEqualObjects(value[3][@"message"], @"Initiate sequence");
+    XCTAssertEqualObjects(value[3][@"name"], @"Initiate sequence");
     XCTAssertEqualObjects(value[3][@"metaData"][@"captain"], @"Bob");
     XCTAssertNotNil(value[3][@"timestamp"]);
 }
@@ -231,7 +231,7 @@ - (void)testAlwaysAllowManual {
     NSArray *value = [self.crumbs arrayValue];
     XCTAssertEqual(1, value.count);
     XCTAssertEqualObjects(value[0][@"type"], @"manual");
-    XCTAssertEqualObjects(value[0][@"message"], @"this is a test");
+    XCTAssertEqualObjects(value[0][@"name"], @"this is a test");
 }
 
 /**
@@ -338,7 +338,7 @@ - (void)testCallbackFreeConstructors2 {
     XCTAssertEqual(Bugsnag.client.configuration.breadcrumbs.count, 8);
 
     XCTAssertEqualObjects(bc0[@"type"], @"state");
-    XCTAssertEqualObjects(bc0[@"message"], @"Bugsnag loaded");
+    XCTAssertEqualObjects(bc0[@"name"], @"Bugsnag loaded");
     XCTAssertEqual([bc0[@"metaData"] count], 0);
 
     XCTAssertEqual([bc1[@"metaData"] count], 1);
@@ -350,25 +350,25 @@ - (void)testCallbackFreeConstructors2 {
     XCTAssertEqual([bc4[@"metaData"] count], 2);
     XCTAssertEqual([bc6[@"metaData"] count], 2);
 
-    XCTAssertEqualObjects(bc1[@"message"], @"manual message");
+    XCTAssertEqualObjects(bc1[@"name"], @"manual message");
     XCTAssertEqualObjects(bc1[@"type"], @"manual");
 
-    XCTAssertEqualObjects(bc2[@"message"], @"log message");
+    XCTAssertEqualObjects(bc2[@"name"], @"log message");
     XCTAssertEqualObjects(bc2[@"type"], @"log");
 
-    XCTAssertEqualObjects(bc3[@"message"], @"navigation message");
+    XCTAssertEqualObjects(bc3[@"name"], @"navigation message");
     XCTAssertEqualObjects(bc3[@"type"], @"navigation");
 
-    XCTAssertEqualObjects(bc4[@"message"], @"process message");
+    XCTAssertEqualObjects(bc4[@"name"], @"process message");
     XCTAssertEqualObjects(bc4[@"type"], @"process");
 
-    XCTAssertEqualObjects(bc5[@"message"], @"request message");
+    XCTAssertEqualObjects(bc5[@"name"], @"request message");
     XCTAssertEqualObjects(bc5[@"type"], @"request");
 
-    XCTAssertEqualObjects(bc6[@"message"], @"state message");
+    XCTAssertEqualObjects(bc6[@"name"], @"state message");
     XCTAssertEqualObjects(bc6[@"type"], @"state");
 
-    XCTAssertEqualObjects(bc7[@"message"], @"user message");
+    XCTAssertEqualObjects(bc7[@"name"], @"user message");
     XCTAssertEqualObjects(bc7[@"type"], @"user");
 }
 
@@ -387,8 +387,8 @@ - (void)testCallbackFreeConstructors3 {
     NSDictionary *bc1 = [Bugsnag.client.configuration.breadcrumbs arrayValue][1];
     NSDictionary *bc2 = [Bugsnag.client.configuration.breadcrumbs arrayValue][2];
 
-    XCTAssertEqualObjects(bc1[@"message"], @"message1");
-    XCTAssertEqualObjects(bc2[@"message"], @"message2");
+    XCTAssertEqualObjects(bc1[@"name"], @"message1");
+    XCTAssertEqualObjects(bc2[@"name"], @"message2");
 
     XCTAssertEqual([bc1[@"metaData"] count], 0);
     XCTAssertEqual([bc2[@"metaData"] count], 0);

diff --git a/Tests/BugsnagClientTests.m b/Tests/BugsnagClientTests.m
@@ -0,0 +1,91 @@
+//
+//  BugsnagClientTests.m
+//  Tests
+//
+//  Created by Robin Macharg on 18/03/2020.
+//  Copyright © 2020 Bugsnag. All rights reserved.
+//
+
+#import "Bugsnag.h"
+#import "BugsnagBreadcrumbs.h"
+#import "BugsnagClient.h"
+#import "BugsnagTestConstants.h"
+#import "BugsnagKeys.h"
+#import <XCTest/XCTest.h>
+
+@interface BugsnagClientTests : XCTestCase
+@end
+
+@interface Bugsnag ()
++ (BugsnagConfiguration *)configuration;
++ (BugsnagClient *)client;
+@end
+
+@interface BugsnagClient ()
+- (void)orientationChanged:(NSNotification *)notif;
+@end
+
+@interface BugsnagBreadcrumb ()
+- (NSDictionary *)objectValue;
+@end
+
+@interface BugsnagConfiguration ()
+@property(readonly, strong, nullable) BugsnagBreadcrumbs *breadcrumbs;
+@end
+
+NSString *BSGFormatSeverity(BSGSeverity severity);
+
+@implementation BugsnagClientTests
+
+/**
+ * A boilerplate helper method to setup Bugsnag
+ */
+-(void)setUpBugsnagWillCallNotify:(bool)willNotify {
+    BugsnagConfiguration *configuration = [[BugsnagConfiguration alloc] initWithApiKey:DUMMY_APIKEY_32CHAR_1];
+    if (willNotify) {
+        [configuration addOnSendBlock:^bool(BugsnagEvent * _Nonnull event) { return false; }];
+    }
+    [Bugsnag startBugsnagWithConfiguration:configuration];
+}
+
+/**
+ * Handled events leave a breadcrumb when notify() is called.  Test that values are inserted
+ * correctly.
+ */
+- (void)testAutomaticNotifyBreadcrumbData {
+
+    [self setUpBugsnagWillCallNotify:false];
+
+    NSException *ex = [[NSException alloc] initWithName:@"myName" reason:@"myReason1" userInfo:nil];
+
+    __block NSString *eventErrorClass;
+    __block NSString *eventErrorMessage;
+    __block BOOL eventUnhandled;
+    __block NSString *eventSeverity;
+
+    // Check that the event is passed the apiKey
+    [Bugsnag notify:ex block:^(BugsnagEvent * _Nonnull event) {
+        XCTAssertEqual(event.apiKey, DUMMY_APIKEY_32CHAR_1);
+
+        // Grab the values that end up in the event for later comparison
+        eventErrorClass = [event errorClass];
+        eventErrorMessage = [event errorMessage];
+        eventUnhandled = [event valueForKeyPath:@"handledState.unhandled"] ? YES : NO;
+        eventSeverity = BSGFormatSeverity([event severity]);
+    }];
+
+    // Check that we can change it
+    [Bugsnag notify:ex];
+
+    NSDictionary *breadcrumb = [[[[Bugsnag client] configuration] breadcrumbs][1] objectValue];
+    NSDictionary *metadata = [breadcrumb valueForKey:@"metaData"];
+
+    XCTAssertEqualObjects([breadcrumb valueForKey:@"type"], @"error");
+    XCTAssertEqualObjects([breadcrumb valueForKey:@"name"], eventErrorClass);
+    XCTAssertEqualObjects([metadata valueForKey:@"errorClass"], eventErrorClass);
+    XCTAssertEqualObjects([metadata valueForKey:@"name"], eventErrorMessage);
+    XCTAssertEqual((bool)[metadata valueForKey:@"unhandled"], eventUnhandled);
+    XCTAssertEqualObjects([metadata valueForKey:@"severity"], eventSeverity);
+}
+
+@end
diff --git a/Tests/BugsnagSinkTests.m b/Tests/BugsnagSinkTests.m
@@ -138,7 +138,7 @@ - (void)testEventBreadcrumbs {
     [self.processedData[@"events"] firstObject][@"breadcrumbs"];
     XCTAssertEqual(2, breadcrumbs.count);
     for (int i = 0; i < breadcrumbs.count; i++) {
-        XCTAssertEqualObjects(expected[i][@"message"], breadcrumbs[i][@"message"]);
+        XCTAssertEqualObjects(expected[i][@"name"], breadcrumbs[i][@"message"]);
         XCTAssertEqualObjects(expected[i][@"type"], breadcrumbs[i][@"type"]);
         XCTAssertEqualObjects(expected[i][@"timestamp"], breadcrumbs[i][@"timestamp"]);
         XCTAssertEqualObjects(expected[i][@"metadata"], breadcrumbs[i][@"metadata"]);

diff --git a/features/steps/ios_steps.rb b/features/steps/ios_steps.rb
@@ -102,7 +102,7 @@
   crumbs = read_key_path(find_request(0)[:body], "events.0.breadcrumbs")
   assert_not_equal(0, crumbs.length, "There are no breadcrumbs on this event")
   match = crumbs.detect do |crumb|
-    crumb["message"] == string && crumb["type"] == type
+    crumb["name"] == string && crumb["type"] == type
   end
   assert_not_nil(match, "No crumb matches the provided message and type")
 end
@@ -111,7 +111,7 @@
   crumbs = read_key_path(find_request(0)[:body], "events.0.breadcrumbs")
   assert_not_equal(0, crumbs.length, "There are no breadcrumbs on this event")
   match = crumbs.detect do |crumb|
-    crumb["message"] == string
+    crumb["name"] == string
   end
   assert_not_nil(match, "No crumb matches the provided message")
 end
-Original file line number
+Diff line change
@@ Expand Up @@
             }
             _overrides = metadata;
         }
     }
     - (NSDictionary *)toJson {
@@ Expand Down @@