Initial implementation of ANRv2 #2549

romtsn · 2023-02-16T12:40:51Z

#skip-changelog

📜 Description

Introduces new pipeline for reporting historical events that should be backfilled with scope/options data that has been serialized to disk in the previous application run (Backfillable hint and BackfillingEventProcessor)
Introduces PersistingScopeObserver/PersistingOptionsObserver that persist scope and option values to disk and get notified whenever any of the observed data changes
New AnrV2Integration which wroks for API 30 and above and uses getHistoricalProcessExitReasons to report ANRs
- Only the latest ANR being reported enriched with the serialized scope/options data from the previous run
- All other historical ANRs are still reported, but without enriching (only static data, like device params)
New AnrV2EventProcessor which takes care of backfilling/filling contexts/scope data for the ANRv2
As the historical exits list stores values in a ring buffer and does not remove them upon reading, we have to keep track of the last reported ANR timestamp - this is done in AndroidEnvelopeCache, where we store this timestamp in a marker file under cacheDir
Extracted common things to ContextUtils that are used both in AnrV2EventProcessor and DefaultAndroidEventProcessor
Added a bunch of equals/hashCode for the protocol classes to be able to assert against them in tests
Made UncaughtExceptionHint public, because we were using DiskFlushNotification as a proxy for handling uncaught exceptions logic, now it's more explicit (and we also use DiskFlushNotification for ANR events)

💡 Motivation and Context

#1796

💚 How did you test it?

Automated and manually

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

The PR is targeting integration branch to simplify PR review process. Other PRs to follow:

Make sure last known TX is correctly associated with the latest ANR
Mark last known session as abnormal
Parse the thread dump and transform it into sentry threads payload

github-actions · 2023-03-09T22:58:36Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 6c51875

github-actions · 2023-03-09T23:13:08Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	347.55 ms	362.60 ms	15.05 ms
Size	1.73 MiB	2.35 MiB	633.74 KiB

Previous results on branch: feat/new-anr-impl

Startup times

Revision	Plain	With Sentry	Diff
`7a4e12e`	269.74 ms	308.46 ms	38.71 ms
`73f0b5d`	323.22 ms	377.60 ms	54.38 ms
`d504c21`	363.02 ms	435.73 ms	72.71 ms
`9ca7f05`	368.14 ms	413.57 ms	45.43 ms
`3b85ba6`	363.41 ms	417.52 ms	54.11 ms

App size

Revision	Plain	With Sentry	Diff
`7a4e12e`	1.73 MiB	2.35 MiB	633.85 KiB
`73f0b5d`	1.73 MiB	2.35 MiB	633.85 KiB
`d504c21`	1.73 MiB	2.35 MiB	633.84 KiB
`9ca7f05`	1.73 MiB	2.35 MiB	633.85 KiB
`3b85ba6`	1.73 MiB	2.35 MiB	633.74 KiB

codecov · 2023-03-10T09:21:30Z

Codecov Report

Patch coverage: 69.67% and project coverage change: -0.28 ⚠️

Comparison is base (2078e71) 80.28% compared to head (bea3016) 80.01%.

❗ Current head bea3016 differs from pull request most recent head 73b5508. Consider uploading reports for the commit 73b5508 to get more accurate results

Additional details and impacted files

@@                Coverage Diff                @@
##             feat/anr-v2    #2549      +/-   ##
=================================================
- Coverage          80.28%   80.01%   -0.28%     
- Complexity          3990     4131     +141     
=================================================
  Files                327      339      +12     
  Lines              15017    15582     +565     
  Branches            1977     2089     +112     
=================================================
+ Hits               12057    12468     +411     
- Misses              2183     2234      +51     
- Partials             777      880     +103

Impacted Files	Coverage Δ
...y/spring/jakarta/CachedBodyHttpServletRequest.java	`80.00% <ø> (ø)`
...y/spring/jakarta/CachedBodyServletInputStream.java	`40.00% <ø> (ø)`
...sentry/spring/jakarta/RequestPayloadExtractor.java	`37.50% <ø> (ø)`
...a/io/sentry/spring/jakarta/SentrySpringFilter.java	`73.68% <ø> (ø)`
...karta/SentrySpringServletContainerInitializer.java	`0.00% <ø> (ø)`
...ava/io/sentry/spring/jakarta/SentryUserFilter.java	`93.33% <ø> (ø)`
...racing/SentrySpanClientHttpRequestInterceptor.java	`0.00% <0.00%> (ø)`
.../spring/jakarta/webflux/SentryRequestResolver.java	`71.42% <0.00%> (ø)`
...ebflux/SentryWebFilterWithThreadLocalAccessor.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/HubAdapter.java	`9.23% <0.00%> (ø)`
... and 69 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

romtsn · 2023-03-10T09:52:20Z

sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java

+      final ApplicationNotResponding error =
+          new ApplicationNotResponding(message, Looper.getMainLooper().getThread());
+      final Mechanism mechanism = new Mechanism();
+      mechanism.setType("ANRv2");


Not sure if we should keep it as ANRv2 or change it to ANR? This way we can query on looker by mechanism I think and see who's sending new ANR events. But also, it'd be possible through the integration list.

The question here is rather: do the users care about this and want to see if the ANR has been reported using new mechanism?

romtsn · 2023-03-10T09:54:50Z

@markushi re. serializing collections - I've checked, and actually all of our collections on scope are concurrent already (event Contexts class, which itself extends ConcurrentHashMap), so I think we're good here.

markushi

Nice one, the earlier walkthrough definitely helped a lot when reviewing! I added a few minor comments about final keywords and one suggestion for the JSON serializer APIs.

A few more thoughts:

I think we need to ensure that the same ANR won't be sent over and over again, ultimately DDoS-ing us in case something goes wrong along the whole process. I can see the marker file gets written in the AndroidEnvelopeCache after super.store(...) is called. Could it make sense to reverse the order here? Like first save the marker file and then store the envelope? If storing the envelope fails we'd loose the event, but we'd be safe from reporting the same ANR on every app start.
The mechanism which ensures an ANR event is enriched with old disk data while no new data is written is A) adding the ANR integration at the beginning and B) having a single threaded executer service right?

sentry/src/main/java/io/sentry/JsonSerializer.java

sentry-android-core/src/main/java/io/sentry/android/core/ContextUtils.java

sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java

markushi · 2023-03-13T06:50:23Z

sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java

+    } catch (Throwable e) {
+      options.getLogger().log(ERROR, "Error reading last ANR marker", e);
+    }
+    return 0L;


I guess that's technically not correct, but safe since this is way before Android was released 😅

Yeah pretty much my thoughts, but I didn't want to deal with nullable values here, so should be fine I guess 🤣

sentry/src/main/java/io/sentry/SentryOptions.java

sentry/src/test/java/io/sentry/SentryTest.kt

romtsn · 2023-03-13T21:26:06Z

The mechanism which ensures an ANR event is enriched with old disk data while no new data is written is A) adding the ANR integration at the beginning and B) having a single threaded executer service right?

Yes, that's right. Even though it's a bit brittle, I've added a super sophisticated integration test in 6c51875, but I think this one is necessary as it tests exactly this scenario end-to-end. Not sure if we can do something better for now without changing even more in the pipeline which I'd really not want to do :D

romtsn added 11 commits November 25, 2022 22:06

Initial implementation

2816902

Still WIP

80895e3

Merge branch 'main' into feat/new-anr-impl

3045476

Merge branch 'main' into feat/new-anr-impl

5e74151

Backfill latest ANRv2 events (WIP)

644ef27

AnrV2EventProcessor

a0d8733

Fix contexts deserialization

6e42417

Fix deserialization of collections

07403be

Add tests for sentry-java core

ff60474

Add tests for sentry-android-core

c4d1972

Merge branch 'main' into feat/new-anr-impl

1aebf0c

romtsn added 2 commits March 9, 2023 23:59

Api dump

0bf87ae

Spotless

38c5da0

romtsn added 2 commits March 10, 2023 00:27

Tweaks

4cbeccc

api dump

6ba882c

romtsn commented Mar 10, 2023

View reviewed changes

romtsn marked this pull request as ready for review March 10, 2023 09:52

romtsn requested review from adinauer, stefanosiano and markushi as code owners March 10, 2023 09:52

markushi approved these changes Mar 13, 2023

View reviewed changes

romtsn added 5 commits March 13, 2023 17:03

Add finals

32833a1

Make observers thread-safe

f11ff65

Add AnrV2 to integrations list

751ce4d

Extrace deserializeCOllection into its own method

bea3016

Add AnrV2 almost e2e integration test

6c51875

Format code

73b5508

romtsn merged commit f346b26 into feat/anr-v2 Mar 14, 2023

romtsn deleted the feat/new-anr-impl branch March 14, 2023 12:40

romtsn mentioned this pull request May 4, 2023

New ANRv2 implementation based on ApplicationExitInfo #2697

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of ANRv2 #2549

Initial implementation of ANRv2 #2549

romtsn commented Feb 16, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023 •

edited

Loading

Previous results on branch: feat/new-anr-impl

Startup times

App size

codecov bot commented Mar 10, 2023 •

edited

Loading

romtsn Mar 10, 2023

romtsn commented Mar 10, 2023

markushi left a comment

markushi Mar 13, 2023

romtsn Mar 13, 2023

romtsn commented Mar 13, 2023

Initial implementation of ANRv2 #2549

Initial implementation of ANRv2 #2549

Conversation

romtsn commented Feb 16, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Mar 9, 2023 • edited Loading

github-actions bot commented Mar 9, 2023 • edited Loading

Performance metrics 🚀

Previous results on branch: feat/new-anr-impl

Startup times

App size

codecov bot commented Mar 10, 2023 • edited Loading

Codecov Report

romtsn Mar 10, 2023

Choose a reason for hiding this comment

romtsn commented Mar 10, 2023

markushi left a comment

Choose a reason for hiding this comment

markushi Mar 13, 2023

Choose a reason for hiding this comment

romtsn Mar 13, 2023

Choose a reason for hiding this comment

romtsn commented Mar 13, 2023

romtsn commented Feb 16, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 10, 2023 •

edited

Loading