Add API to retrieve Android scope data #2814

markushi · 2023-07-03T11:35:53Z

📜 Description

More discussion on this one: getsentry/team-mobile#11

💡 Motivation and Context

Enable pulling scope data via InternalSentrySdk.serializeScope(scope: Scope) : Map<String, Any>

In order to not re-implement all our protocol serialization logic again, I extracted some interfaces to abstract away the underlying mechanism (Classic JsonObjectWriter + new MapObjectWriter).

I tried to keep the changes to a minimum, but there are still a few rough edges:

The ObjectWriter interface methods all throw IOExceptions because the underlying Gson impl does it
A few @SuppressWarnings("unchecked") in MapObjectWriter due to the dynamic nature of Map<String, Any>
Some *ObjectWriter.serialize Date/TimeZone/Collection code is duplicate

💚 How did you test it?

📝 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

#skip-changelog

github-actions · 2023-07-03T11:36:59Z

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

Generated by 🚫 dangerJS against 5f44d96

sentry/src/main/java/io/sentry/ScopeUtil.java

To better reflect how cocoa works and have a more central API for hybrid SDKs

github-actions · 2023-07-05T08:09:54Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	278.74 ms	370.67 ms	91.93 ms
Size	1.72 MiB	2.29 MiB	574.43 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`9246ed4`	275.63 ms	321.31 ms	45.69 ms
`f274c79`	334.86 ms	348.54 ms	13.68 ms
`c03a05e`	390.40 ms	419.35 ms	28.94 ms
`9246ed4`	281.79 ms	352.08 ms	70.29 ms
`8820c5c`	330.60 ms	416.86 ms	86.26 ms
`adf8fe3`	300.49 ms	357.36 ms	56.87 ms
`0310da5`	381.20 ms	404.50 ms	23.30 ms
`dc67004`	273.86 ms	346.37 ms	72.51 ms
`496bdfd`	272.86 ms	407.33 ms	134.48 ms
`caf50ec`	302.36 ms	325.25 ms	22.89 ms

App size

Revision	Plain	With Sentry	Diff
`9246ed4`	1.72 MiB	2.28 MiB	572.22 KiB
`f274c79`	1.72 MiB	2.29 MiB	575.01 KiB
`c03a05e`	1.72 MiB	2.29 MiB	574.43 KiB
`9246ed4`	1.72 MiB	2.28 MiB	572.22 KiB
`8820c5c`	1.72 MiB	2.28 MiB	571.82 KiB
`adf8fe3`	1.72 MiB	2.29 MiB	575.24 KiB
`0310da5`	1.72 MiB	2.28 MiB	573.45 KiB
`dc67004`	1.72 MiB	2.28 MiB	573.45 KiB
`496bdfd`	1.72 MiB	2.28 MiB	571.82 KiB
`caf50ec`	1.72 MiB	2.29 MiB	575.24 KiB

Previous results on branch: feat/hybrid-scope-sync

Startup times

Revision	Plain	With Sentry	Diff
`7954f77`	292.20 ms	362.88 ms	70.68 ms
`cce67e0`	282.84 ms	331.51 ms	48.67 ms
`6524670`	298.71 ms	317.40 ms	18.69 ms
`d307d12`	264.82 ms	340.51 ms	75.69 ms
`59444c6`	294.98 ms	360.68 ms	65.70 ms
`b6de74f`	351.85 ms	364.75 ms	12.90 ms

App size

Revision	Plain	With Sentry	Diff
`7954f77`	1.72 MiB	2.29 MiB	574.99 KiB
`cce67e0`	1.72 MiB	2.29 MiB	574.30 KiB
`6524670`	1.72 MiB	2.29 MiB	574.59 KiB
`d307d12`	1.72 MiB	2.29 MiB	574.99 KiB
`59444c6`	1.72 MiB	2.29 MiB	574.59 KiB
`b6de74f`	1.72 MiB	2.29 MiB	574.59 KiB

codecov · 2023-07-05T10:15:47Z

Codecov Report

Patch coverage: 84.02% and project coverage change: +0.15 🎉

Comparison is base (dc67004) 81.15% compared to head (2860fa1) 81.30%.

❗ Current head 2860fa1 differs from pull request most recent head 5f44d96. Consider uploading reports for the commit 5f44d96 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2814      +/-   ##
============================================
+ Coverage     81.15%   81.30%   +0.15%     
- Complexity     4501     4615     +114     
============================================
  Files           348      352       +4     
  Lines         16685    17013     +328     
  Branches       2268     2298      +30     
============================================
+ Hits          13540    13832     +292     
- Misses         2198     2233      +35     
- Partials        947      948       +1

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/Breadcrumb.java	`85.04% <ø> (ø)`
.../src/main/java/io/sentry/JsonObjectSerializer.java	`84.21% <ø> (ø)`
...ry/src/main/java/io/sentry/ProfilingTraceData.java	`78.51% <ø> (ø)`
.../main/java/io/sentry/ProfilingTransactionData.java	`52.52% <ø> (ø)`
...entry/src/main/java/io/sentry/SentryBaseEvent.java	`90.90% <ø> (ø)`
.../src/main/java/io/sentry/SentryEnvelopeHeader.java	`96.92% <ø> (ø)`
.../main/java/io/sentry/SentryEnvelopeItemHeader.java	`87.95% <ø> (ø)`
sentry/src/main/java/io/sentry/SentryEvent.java	`89.36% <ø> (ø)`
sentry/src/main/java/io/sentry/SentryItemType.java	`87.50% <ø> (ø)`
sentry/src/main/java/io/sentry/SentryLevel.java	`100.00% <ø> (ø)`
... and 43 more

... and 29 files with indirect coverage changes

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

krystofwoldrich · 2023-07-10T07:37:30Z

RN implementation using the serializer is here getsentry/sentry-react-native#3170.

I've noticed there is very little data returned from the Android scope, only Breadcrumbs and items synced from RN to Android are present (redux state and routing navigation tag).

{
  "breadcrumbs": [
    {
      "category": "app.lifecycle",
      "level": "info",
      "timestamp": "2023-07-10T07:11:12.875Z",
      "type": "session",
      "data": {
        "state": "start"
      }
    }
  ],
  "contexts": {
    "state": {
      "state": {
        "value": {
          "counter": 0
        },
        "type": "redux"
      }
    }
  },
  "fingerprint": [],
  "user": null,
  "extras": {},
  "tags": {
    "routing.route.name": "Home"
  },
  "level": null
}

marandaneto · 2023-07-10T08:27:36Z

@krystofwoldrich there's a lot of data missing, InternalSentrySdk.serializeScope is just one part of the data you need.
For example, all the data set on DefaultAndroidEventProcessor, MainEventProcessor, etc, and every data that is set when captureEvent is captured should be returned as well.
I don't think that InternalSentrySdk.serializeScope is a good naming as well since we need more data than just the scope's data.

krystofwoldrich · 2023-07-10T10:23:49Z

@marandaneto Yes, I agree,

I think something like InternalSentrySdk.getEventScope, InternalSentrySdk.getFullScope, or InternalSentrySdk.getAndroidScope would make sense.

stefanosiano · 2023-07-12T11:15:09Z

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

-        }
-
-        return sideLoadedInfo;
+        return new SideLoadedInfo(installerPackageName == null, installerPackageName);


You can put the check inside the constructor, so that you can pass only one parameter

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

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

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

krystofwoldrich · 2023-07-13T12:05:07Z

I tested the latest version with RN and I'm getting all data as before the change. 👍🚀

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

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

sentry/src/main/java/io/sentry/ObjectWriter.java

sentry/src/main/java/io/sentry/protocol/Geo.java

sentry/src/main/java/io/sentry/util/MapObjectWriter.java

stefanosiano · 2023-07-13T15:46:28Z

just few comments, but looks good overall.
I'll have another look to confirm.
As a side note, for the next time, may you try to split all your changes across separate PRs? That would make it easier and faster to review.

stefanosiano

Looks good!

marandaneto · 2023-07-18T07:40:34Z

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

+  @Nullable
+  public static Scope getCurrentScope() {
+    final @NotNull AtomicReference<Scope> scopeRef = new AtomicReference<>();
+    HubAdapter.getInstance().withScope(scopeRef::set);


@markushi See

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

Lines 46 to 47 in beba014

// don't ref. to method reference, theres a bug on it

// noinspection Convert2MethodRef

This has an issue with some specific use cases, please avoid method references.

Also check if the source code has any more references to ::.

Are you sure this is a problem? I see more than 50 occurrences in our code. Most of them are backend-related, but there are also some which do get executed on Android devices (e.g. JsonObjectDeserializer)

yes, I've written that comment myself, its a problem on Android only during desugaring, mainly because of older AGP versions (Hi RN and Flutter).
If there are more on Android code, they should be removed, that's why this comment #2814 (comment)

@markushi can you raise a new issue for replacing all :: references that run on Android code?

👍 created an issue here, #2848

Following this logic, we should also get rid of all default methods in interfaces, because this is also part of the desugaring. We use those quite heavily. The fact, that nobody has reported any problems so far kinda confirms that there's noone of our customers on AGP lower than 3.0. Is still worth considering this limitation?

@romtsn no, we decided and use Java 8 features, the min. version of AGP isn't a problem here, the method reference was a bug on the AGP desugaring and not "not supported" if not version >= X.
If default has a bug in one of the AGP versions that are above the min. required, then yes, we'd get rid of default methods for compatibility, which is not the case.

Ok, what was the version where the bug has been fixed? If it's 3.0.1 I'd say we are still good to not consider it

Don't recall, but why do we even bother if we can keep 100% compatibility by having just a few chars more? () -> {}

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

marandaneto · 2023-07-18T07:47:24Z

sentry/src/main/java/io/sentry/JsonObjectWriter.java

@@ -6,18 +6,85 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

-public final class JsonObjectWriter extends JsonWriter {
+public final class JsonObjectWriter implements ObjectWriter {


This class is public and not annotated as internal, technically a breaking change, was that intentional?
What's the motivation for the renaming?

This is a required change in order to serialize protocol classes to either Json or Map<>. IMHO it's an oversight that this class is not marked as apistatus.internal. If we consider this a breaking change we should talk to Karl/Krystof as this PR is blocking the RN profiling feature.

I agree its an oversight, but the class/interface was used in pretty much every class of the protocol.

Broke the C# binding somehow: https://github.com/getsentry/sentry-dotnet/pull/2498/files
cc @bitsandfoxes

marandaneto · 2023-07-18T07:50:18Z

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

+  @Nullable
+  public static Scope getCurrentScope() {
+    final @NotNull AtomicReference<Scope> scopeRef = new AtomicReference<>();
+    HubAdapter.getInstance().withScope(scopeRef::set);


Also, withScope has side effects and isn't intended to be used on Android, since it's a single hub/scope.
It's being even removed from a few SDKs in favor of the scope lambda -> captureException(scope {})
This might have side effects, I know @romtsn dislikes this API and already had issues with it as well.

This is intentional, as it returns a safe copy of the current scope. I already talked to Roman about this, this is probably the only good use-case for using withScope 😅

H: withScope isn't thread-safe because it's meant to run in a single thread.
Android is single hub/scope.
If another thread set something on the scope while the push has been executed but not the pop yet, the data is set in the wrong scope, IMO this is still a bug, and withScope should never be used in Android.
This API was a mistake and is already deprecated in a few SDKs as mentioned, in favor of the scope lambda.

@markushi this is marked as resolved but it's not AFAIK.
the usage of withScope is still there, mind sharing a commit link or PR?

I thought this PR solved that, but looking closer at the code the withScope is used.

Fix ensure session is updated using configureScope #2846

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

Add MapObjectWriter to serialize protocol data to Java Map<>

254554a

markushi requested review from marandaneto and krystofwoldrich July 3, 2023 11:35

markushi commented Jul 3, 2023

View reviewed changes

sentry/src/main/java/io/sentry/ScopeUtil.java Outdated Show resolved Hide resolved

Move from ScopeUtil to InternalSentrySdk

8b1b62b

To better reflect how cocoa works and have a more central API for hybrid SDKs

krystofwoldrich mentioned this pull request Jul 5, 2023

Align how we enrich the event with device context on Hybrid SDKs getsentry/team-mobile#11

Open

Fix test

6e3df8a

krystofwoldrich mentioned this pull request Jul 10, 2023

Add SentrySerialized to ReactNativeWritable convertor, load device context from sentry-java scope getsentry/sentry-react-native#3170

Merged

10 tasks

Extract device information from DefaultAndroidEventProcessor

a461f7e

markushi marked this pull request as ready for review July 11, 2023 06:07

markushi requested review from adinauer, romtsn and stefanosiano as code owners July 11, 2023 06:07

markushi added 3 commits July 12, 2023 09:03

Fix back-fill user if not manually set on scope

26ab966

Add missing os context

74cbd44

Enrich os, user and app context

e856a09

stefanosiano reviewed Jul 12, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ContextUtils.java Outdated Show resolved Hide resolved

stefanosiano reviewed Jul 12, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/Installation.java Outdated Show resolved Hide resolved

stefanosiano reviewed Jul 12, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java Show resolved Hide resolved

markushi added 2 commits July 13, 2023 09:11

Add InternalSentrySdk.captureEnvelope

09953d7

Address PR comments

2860fa1

stefanosiano reviewed Jul 13, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/DeviceInfoUtil.java Show resolved Hide resolved

stefanosiano reviewed Jul 13, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java Outdated Show resolved Hide resolved

stefanosiano reviewed Jul 13, 2023

View reviewed changes

sentry/src/main/java/io/sentry/ObjectWriter.java Outdated Show resolved Hide resolved

stefanosiano reviewed Jul 13, 2023

View reviewed changes

sentry/src/main/java/io/sentry/protocol/Geo.java Outdated Show resolved Hide resolved

stefanosiano reviewed Jul 13, 2023

View reviewed changes

sentry/src/main/java/io/sentry/util/MapObjectWriter.java Show resolved Hide resolved

Improve nullability and java docs

5f44d96

stefanosiano approved these changes Jul 17, 2023

View reviewed changes

markushi merged commit beba014 into main Jul 18, 2023
19 checks passed

markushi deleted the feat/hybrid-scope-sync branch July 18, 2023 04:53

marandaneto reviewed Jul 18, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java Show resolved Hide resolved

marandaneto reviewed Jul 18, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java Show resolved Hide resolved

marandaneto reviewed Jul 18, 2023

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java Show resolved Hide resolved

This was referenced Jul 19, 2023

Replace all usages of method references (aka ::) with lambdas #2848

Closed

Use configureScope instead of withScope for InternalSentrySdk #2863

Merged

chore(deps): update Java SDK to v6.26.0 getsentry/sentry-dotnet#2498

Closed

romtsn mentioned this pull request Feb 20, 2024

[SR] Introduce MapObjectReader #3215

Merged

	// don't ref. to method reference, theres a bug on it
	// noinspection Convert2MethodRef

Add API to retrieve Android scope data #2814

Add API to retrieve Android scope data #2814

Conversation

markushi commented Jul 3, 2023 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jul 3, 2023 • edited

github-actions bot commented Jul 5, 2023 • edited

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/hybrid-scope-sync

Startup times

App size

codecov bot commented Jul 5, 2023 • edited

Codecov Report

krystofwoldrich commented Jul 10, 2023

marandaneto commented Jul 10, 2023

krystofwoldrich commented Jul 10, 2023

Choose a reason for hiding this comment

krystofwoldrich commented Jul 13, 2023

stefanosiano commented Jul 13, 2023

stefanosiano left a 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

Choose a reason for hiding this comment

marandaneto Jul 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto Jul 18, 2023 • edited

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

marandaneto Jul 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markushi commented Jul 3, 2023 •

edited

github-actions bot commented Jul 3, 2023 •

edited

github-actions bot commented Jul 5, 2023 •

edited

codecov bot commented Jul 5, 2023 •

edited

marandaneto Jul 19, 2023 •

edited

marandaneto Jul 18, 2023 •

edited

marandaneto Jul 18, 2023 •

edited

marandaneto Jul 24, 2023 •

edited