Fix: Breadcrumbs added on forked context are now captured #629

lucas-zimerman · 2024-04-17T19:12:57Z

This PR brings more native context data to be synced with captured events and also allows hint breadcrumbs to be properly registered, for example:

    Sentry.captureException(new Error(`${Date.now()}: a test error occurred`), (context) => {
      return context.addBreadcrumb({ message: 'test' });
    });

will now be registered on the event, including the native events:

Additional notes on the PR:

No Required changes were needed to support it on iOS.
Android had an required refactor in order to capture Envelopes no longer by files.
some iOS specific properties are now working with both Android and iOS
Improved the device context sync on both Android and iOS.

🛑 Blocked by

Closes #263

…version. Added tests. used a better name for the new function that joins two groups of breadcrumbs. Removed comment from java layer. Removed duplicated tests on the wrapper test file

src/integrations/devicecontext.ts

test/integrations/devicecontext.test.ts

krystofwoldrich · 2024-04-19T14:40:57Z

test/integrations/devicecontext.test.ts

+  }
+});
+
+describe('Device Context Breadcrumb filter', () => {


We should add test for user set ~~timeout~~ timestamp. Something like this:

const jsList = [ { timestamp: 2, message: 'new js' }, { timestamp: 1, message: 'new js' }, ] as Breadcrumb[]; const nativeList = [ { timestamp: 2, message: 'new native' }, { timestamp: 2, message: 'new js' }, { timestamp: 1, message: 'new js' }, { timestamp: 3, message: 'new native' }, ] as Breadcrumb[]; const expected = [ { timestamp: 2, message: 'new native' }, { timestamp: 2, message: 'new js' }, { timestamp: 1, message: 'new js' }, { timestamp: 3, message: 'new native' }, ] as Breadcrumb[];

src/wrapper.ts

github-actions · 2024-05-11T00:11:43Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

…ue; requested changes

android/src/main/java/io/sentry/capacitor/SentryCapacitor.java

krystofwoldrich · 2024-05-27T14:07:41Z

android/src/main/java/io/sentry/capacitor/SentryCapacitor.java

+          catch (Throwable e) {
+            final String errorMessage ="Error while capturing envelope";
+            logger.log(Level.WARNING, errorMessage);
+            call.reject(errorMessage);


I see previously this was call.reject(String.valueOf(e));? Does this change the error in any way?

krystofwoldrich · 2024-05-27T14:11:32Z

android/src/main/java/io/sentry/capacitor/CapSentryMapConverter.java

+
+  private static final ILogger logger = new AndroidLogger(NAME);
+
+  public static Object convertToWritable(Object serialized) {


Can this return JSObject directly?

krystofwoldrich · 2024-05-27T14:12:54Z

android/src/main/java/io/sentry/capacitor/CapSentryMapConverter.java

+import io.sentry.SentryLevel;
+import io.sentry.android.core.AndroidLogger;
+
+public class CapSentryMapConverter {


Can we add unit tests for this class?

krystofwoldrich · 2024-05-27T15:07:29Z

src/integrations/devicecontext.ts

+  private _mergeUniqueBreadcrumbs(jsList: Breadcrumb[], nativeList: Breadcrumb[], maxBreadcrumbs: number): Breadcrumb[] {
+    // Ensure both lists are ordered by timestamp.
+    const orderedNativeList = [...nativeList].sort((a, b) => (a.timestamp ?? 0) - (b.timestamp ?? 0));
+    const orderedJsList = [...jsList].sort((a, b) => (a.timestamp ?? 0) - (b.timestamp ?? 0));
+
+    const combinedList: Breadcrumb[] = [];
+    let jsIndex = 0;
+    let natIndex = 0;
+
+    while (jsIndex < orderedJsList.length && natIndex < orderedNativeList.length && combinedList.length < maxBreadcrumbs)
+    {
+      const jsBreadcrumb = orderedJsList[jsIndex];
+      const natBreadcrumb = orderedNativeList[natIndex];
+
+      if (jsBreadcrumb.timestamp === natBreadcrumb.timestamp &&
+        jsBreadcrumb.message === natBreadcrumb.message) {
+        combinedList.push(jsBreadcrumb);
+        jsIndex++;
+        natIndex++;
+      }
+      else if (jsBreadcrumb.timestamp && natBreadcrumb.timestamp &&
+        jsBreadcrumb.timestamp < natBreadcrumb.timestamp)
+      {
+        combinedList.push(jsBreadcrumb);
+        jsIndex++;
+      }
+      else {
+        combinedList.push(natBreadcrumb);
+        natIndex++;
+      }
+    }
+
+    // Add remaining breadcrumbs from the JavaScript and Native list if space allows.
+    while (jsIndex < orderedJsList.length  && combinedList.length < maxBreadcrumbs) {
+      combinedList.push(orderedJsList[jsIndex++]);
+    }
+
+    while (natIndex < orderedNativeList.length && combinedList.length < maxBreadcrumbs) {
+      combinedList.push(orderedNativeList[natIndex++]);
+    }
+    return combinedList;
+  }


The function works, but I think we should consider marking the JS crumbs during the creation on native layers, as then we can also filter then on the native layers and only return the native crumbs.

The native solution would be faster and required less memory.

We could do JSBreadcrumb extends Breadcrumb and then filter breadcrumb instanceof JSBreadcrumb.

Because Breadcrumb is final (https://github.com/getsentry/sentry-java/blob/9596bb93199a1032cb255a62bea32020bd04c9ac/sentry/src/main/java/io/sentry/Breadcrumb.java#L20), thank @lucas-zimerman for pointing that out, we can extend it.

But the class has property called unknown marked for internal use. We could add a flag to it.

Breadcrumb crumb = new Breadcrumb(); crumb.getUnknown().put("hybrid", true);

@romtsn @markushi I don't know current usage of the unknown field, could this cause any issue/would it work?

unknown is used across our protocol classes to store attributes which are not defined within sentry-native. This avoids any data loss, achieving ~serialize(deserialize(breadcrumbJson)) == breadcrumbJson.

So yes it would work, but all unknown fields end up in the network requests as well - which is probably something we want to have anyway at some point?
To be honest I'd prefer some explicit field, e.g. breadcrumb.origin.

Got it thanks, breadcrumb.origin at first I think it could be runtime only, not included in the serialization, not transferred over network. I can see it being used later in the product, but currently there are no plans and there might never be.

We only want to solve a technical problem of efficiently filter js breadcrumbs from all the crumbs currently present on the scope.

yeah, let's please not use unknown - it exists to work-around JVM's static type system exclusively. A new field is the cleanest way imo, but we could also open the Breadcrumb class for extension, should not be a problem

I also like the idea of breadcrumb.origin, it could also help in the future to know if a breadcrumb was originated from the JavaScript layer or native

I also like the idea of breadcrumb.origin, it could also help in the future to know if a breadcrumb was originated from the JavaScript layer or native

Hey @lucas-zimerman 👋
I've drafted an iOS PR and was wondering if you have any early feedback on it. Does the approach chosen unblock this PR?

Co-authored-by: Krystof Woldrich <[email protected]>

github-actions · 2024-06-25T00:12:09Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

github-actions · 2024-07-18T00:12:29Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

github-actions · 2024-09-07T00:13:21Z

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

lucas-zimerman added 2 commits April 17, 2024 14:03

initial implementation - android ok

90c8093

missing files

90319a8

lucas-zimerman changed the title ~~Fix: Merge breadcrumbs from envelope with native breadcrumbs.~~ Fix: Breadcrumbs added on hints are now captured Apr 17, 2024

add changelog

259d147

lucas-zimerman requested a review from krystofwoldrich April 18, 2024 19:12

lucas-zimerman added 2 commits April 18, 2024 18:15

removed extra lines from the java project. rollback original release …

b79460f

…version. Added tests. used a better name for the new function that joins two groups of breadcrumbs. Removed comment from java layer. Removed duplicated tests on the wrapper test file

forgot git add -A

4f4c0ce

lucas-zimerman marked this pull request as ready for review April 18, 2024 21:16

lucas-zimerman changed the title ~~Fix: Breadcrumbs added on hints are now captured~~ Fix: Breadcrumbs added on forked context are now captured Apr 18, 2024

lucas-zimerman added 6 commits April 18, 2024 18:25

test

79a3bea

space

98df4e3

native test fix?

6863c45

fix flaky tests

dd2a29a

fix sorting

9bdd97d

rollback buildandtest yml

70dd584

krystofwoldrich reviewed Apr 19, 2024

View reviewed changes

src/integrations/devicecontext.ts Outdated Show resolved Hide resolved

krystofwoldrich reviewed Apr 19, 2024

View reviewed changes

test/integrations/devicecontext.test.ts Show resolved Hide resolved

krystofwoldrich reviewed Apr 19, 2024

View reviewed changes

src/wrapper.ts Outdated Show resolved Hide resolved

krystofwoldrich reviewed Apr 19, 2024

View reviewed changes

src/wrapper.ts Show resolved Hide resolved

github-actions bot added the Status: Stale label May 11, 2024

github-actions bot closed this May 19, 2024

lucas-zimerman reopened this May 23, 2024

lucas-zimerman added 2 commits May 23, 2024 11:02

merge main

f92a6ad

double check/ order native/js breadcrumbs ; respect max breacrumb val…

9727c02

…ue; requested changes

github-actions bot removed the Status: Stale label May 24, 2024

krystofwoldrich reviewed May 27, 2024

View reviewed changes

android/src/main/java/io/sentry/capacitor/SentryCapacitor.java Outdated Show resolved Hide resolved

krystofwoldrich reviewed May 27, 2024

View reviewed changes

Update android/src/main/java/io/sentry/capacitor/SentryCapacitor.java

f7ec2ba

Co-authored-by: Krystof Woldrich <[email protected]>

This was referenced Jun 10, 2024

Add breadrumb.origin getsentry/sentry-java#3470

Open

Add breadrumb.origin getsentry/sentry-cocoa#4043

Open

github-actions bot added the Status: Stale label Jun 25, 2024

lucas-zimerman added Blocked and removed Status: Stale labels Jun 26, 2024

github-actions bot added the Status: Stale label Jul 18, 2024

github-actions bot closed this Jul 25, 2024

lucas-zimerman reopened this Aug 16, 2024

lucas-zimerman removed the Status: Stale label Aug 16, 2024

github-actions bot added the Status: Stale label Sep 7, 2024

lucas-zimerman removed the Status: Stale label Sep 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Breadcrumbs added on forked context are now captured #629

Fix: Breadcrumbs added on forked context are now captured #629

lucas-zimerman commented Apr 17, 2024 •

edited

Loading

krystofwoldrich Apr 19, 2024 •

edited

Loading

github-actions bot commented May 11, 2024

krystofwoldrich May 27, 2024

krystofwoldrich May 27, 2024

krystofwoldrich May 27, 2024

krystofwoldrich May 27, 2024

krystofwoldrich May 28, 2024

markushi May 28, 2024

markushi May 28, 2024

krystofwoldrich May 28, 2024

romtsn May 28, 2024 •

edited

Loading

lucas-zimerman Jun 3, 2024

antonis Sep 20, 2024

github-actions bot commented Jun 25, 2024

github-actions bot commented Jul 18, 2024

github-actions bot commented Sep 7, 2024


		private static final ILogger logger = new AndroidLogger(NAME);

		public static Object convertToWritable(Object serialized) {

Fix: Breadcrumbs added on forked context are now captured #629

Are you sure you want to change the base?

Fix: Breadcrumbs added on forked context are now captured #629

Conversation

lucas-zimerman commented Apr 17, 2024 • edited Loading

🛑 Blocked by

krystofwoldrich Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romtsn May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 25, 2024

github-actions bot commented Jul 18, 2024

github-actions bot commented Sep 7, 2024

lucas-zimerman commented Apr 17, 2024 •

edited

Loading

krystofwoldrich Apr 19, 2024 •

edited

Loading

romtsn May 28, 2024 •

edited

Loading