Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add current activity name to app context #2999

Merged
merged 14 commits into from
Nov 8, 2023
Merged

Add current activity name to app context #2999

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bd60464
Add current activity name to app context
markushi Oct 19, 2023
09dfd0d
Add missing tests
markushi Oct 19, 2023
7392211
Update Changelog
markushi Oct 19, 2023
3c05fe7
Merge branch 'main' into feat/add-current-screen-to-app-context
markushi Oct 24, 2023
c502dbb
Merge branch 'main' into feat/add-current-screen-to-app-context
markushi Oct 24, 2023
2201017
Extend (de)serialization tests
markushi Oct 24, 2023
7d631f7
Merge branch 'feat/add-current-screen-to-app-context' of github.com:g…
markushi Oct 24, 2023
6cd7c42
Mark screen API as internal
markushi Oct 27, 2023
11559df
Fix tests
markushi Oct 27, 2023
49a0e71
Fix tests
markushi Oct 27, 2023
987e202
Merge branch 'main' into feat/add-current-screen-to-app-context
markushi Oct 31, 2023
c44b4b7
Ensure screen scope change is propagated
markushi Nov 8, 2023
458d956
Merge branch 'main' into feat/add-current-screen-to-app-context
markushi Nov 8, 2023
67adcc4
Merge branch 'feat/add-current-screen-to-app-context' of github.com:g…
markushi Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Add thread information to spans ([#2998](https://github.com/getsentry/sentry-java/pull/2998))
- Use PixelCopy API for capturing screenshots on API level 24+ ([#3008](https://github.com/getsentry/sentry-java/pull/3008))
- Add current activity name to app context ([#2999](https://github.com/getsentry/sentry-java/pull/2999))

### Fixes

Expand Down
5 changes: 5 additions & 0 deletions sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import io.sentry.SpanStatus;
import io.sentry.TransactionContext;
import io.sentry.TransactionOptions;
import io.sentry.android.core.internal.util.ClassUtil;
import io.sentry.android.core.internal.util.FirstDrawDoneListener;
import io.sentry.protocol.MeasurementValue;
import io.sentry.protocol.TransactionNameSource;
Expand Down Expand Up @@ -372,6 +373,10 @@ public synchronized void onActivityCreated(
final @NotNull Activity activity, final @Nullable Bundle savedInstanceState) {
setColdStart(savedInstanceState);
addBreadcrumb(activity, "created");
if (hub != null) {
final @Nullable String activityClassName = ClassUtil.getClassName(activity);
hub.configureScope(scope -> scope.setScreen(activityClassName));
}
startTracing(activity);
final @Nullable ISpan ttfdSpan = ttfdSpanMap.get(activity);

Expand Down
24 changes: 12 additions & 12 deletions sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,19 +97,19 @@ public static Map<String, Object> serializeScope(
@Nullable App app = scope.getContexts().getApp();
if (app == null) {
app = new App();
app.setAppName(ContextUtils.getApplicationName(context, options.getLogger()));
app.setAppStartTime(DateUtils.toUtilDate(AppStartState.getInstance().getAppStartTime()));

final @NotNull BuildInfoProvider buildInfoProvider =
new BuildInfoProvider(options.getLogger());
final @Nullable PackageInfo packageInfo =
ContextUtils.getPackageInfo(
context, PackageManager.GET_PERMISSIONS, options.getLogger(), buildInfoProvider);
if (packageInfo != null) {
ContextUtils.setAppPackageInfo(packageInfo, buildInfoProvider, app);
}
scope.getContexts().setApp(app);
}
app.setAppName(ContextUtils.getApplicationName(context, options.getLogger()));
app.setAppStartTime(DateUtils.toUtilDate(AppStartState.getInstance().getAppStartTime()));

final @NotNull BuildInfoProvider buildInfoProvider =
new BuildInfoProvider(options.getLogger());
final @Nullable PackageInfo packageInfo =
ContextUtils.getPackageInfo(
context, PackageManager.GET_PERMISSIONS, options.getLogger(), buildInfoProvider);
if (packageInfo != null) {
ContextUtils.setAppPackageInfo(packageInfo, buildInfoProvider, app);
}
scope.getContexts().setApp(app);

writer.name("user").value(logger, scope.getUser());
writer.name("contexts").value(logger, scope.getContexts());
Expand Down
6 changes: 2 additions & 4 deletions sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.sentry.android.core.internal.gestures.ViewUtils;
import io.sentry.android.core.internal.util.AndroidCurrentDateProvider;
import io.sentry.android.core.internal.util.AndroidMainThreadChecker;
import io.sentry.android.core.internal.util.ClassUtil;
import io.sentry.android.core.internal.util.Debouncer;
import io.sentry.internal.viewhierarchy.ViewHierarchyExporter;
import io.sentry.protocol.ViewHierarchy;
Expand Down Expand Up @@ -240,10 +241,7 @@ private static void addChildren(
private static ViewHierarchyNode viewToNode(@NotNull final View view) {
@NotNull final ViewHierarchyNode node = new ViewHierarchyNode();

@Nullable String className = view.getClass().getCanonicalName();
if (className == null) {
className = view.getClass().getSimpleName();
}
@Nullable String className = ClassUtil.getClassName(view);
node.setType(className);

try {
Expand Down
6 changes: 2 additions & 4 deletions ...c/main/java/io/sentry/android/core/internal/gestures/AndroidViewGestureTargetLocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import android.widget.AbsListView;
import android.widget.ScrollView;
import androidx.core.view.ScrollingView;
import io.sentry.android.core.internal.util.ClassUtil;
import io.sentry.internal.gestures.GestureTargetLocator;
import io.sentry.internal.gestures.UiElement;
import org.jetbrains.annotations.ApiStatus;
Expand Down Expand Up @@ -44,10 +45,7 @@ && isViewScrollable(view, isAndroidXAvailable)) {
private UiElement createUiElement(final @NotNull View targetView) {
try {
final String resourceName = ViewUtils.getResourceId(targetView);
@Nullable String className = targetView.getClass().getCanonicalName();
if (className == null) {
className = targetView.getClass().getSimpleName();
}
@Nullable String className = ClassUtil.getClassName(targetView);
return new UiElement(targetView, className, resourceName, null, ORIGIN);
} catch (Resources.NotFoundException ignored) {
return null;
Expand Down
19 changes: 19 additions & 0 deletions sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ClassUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.sentry.android.core.internal.util;

import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.Nullable;

@ApiStatus.Internal
public class ClassUtil {

Check warning on line 7 in sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ClassUtil.java

View check run for this annotation

Codecov / codecov/patch

sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ClassUtil.java#L7 

Added line #L7 was not covered by tests

public static @Nullable String getClassName(final @Nullable Object object) {
if (object == null) {
return null;
}
final @Nullable String canonicalName = object.getClass().getCanonicalName();
if (canonicalName != null) {
return canonicalName;
}
return object.getClass().getSimpleName();
}
}
33 changes: 29 additions & 4 deletions sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,10 +1405,31 @@ class ActivityLifecycleIntegrationTest {
sut.register(fixture.hub, fixture.options)
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub).configureScope(any())
// once for the screen, and once for the tracing propagation context
verify(fixture.hub, times(2)).configureScope(any())
assertNotSame(propagationContextAtStart, scope.propagationContext)
}

@Test
fun `sets the activity as the current screen`() {
val sut = fixture.getSut()
val activity = mock<Activity>()
fixture.options.enableTracing = false

val argumentCaptor: ArgumentCaptor<ScopeCallback> = ArgumentCaptor.forClass(ScopeCallback::class.java)
val scope = mock<Scope>()
whenever(fixture.hub.configureScope(argumentCaptor.capture())).thenAnswer {
argumentCaptor.value.run(scope)
}

sut.register(fixture.hub, fixture.options)
sut.onActivityCreated(activity, fixture.bundle)

// once for the screen, and once for the tracing propagation context
verify(fixture.hub, times(2)).configureScope(any())
verify(scope).setScreen(any())
}

@Test
fun `does not start another new trace if one has already been started but does after activity was destroyed`() {
val sut = fixture.getSut()
Expand All @@ -1425,21 +1446,25 @@ class ActivityLifecycleIntegrationTest {
sut.register(fixture.hub, fixture.options)
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub).configureScope(any())
// once for the screen, and once for the tracing propagation context
verify(fixture.hub, times(2)).configureScope(any())

val propagationContextAfterNewTrace = scope.propagationContext
assertNotSame(propagationContextAtStart, propagationContextAfterNewTrace)

clearInvocations(fixture.hub)
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub, never()).configureScope(any())
// once for the screen, but not for the tracing propagation context
verify(fixture.hub).configureScope(any())
assertSame(propagationContextAfterNewTrace, scope.propagationContext)

sut.onActivityDestroyed(activity)

clearInvocations(fixture.hub)
sut.onActivityCreated(activity, fixture.bundle)
verify(fixture.hub).configureScope(any())
// once for the screen, and once for the tracing propagation context
verify(fixture.hub, times(2)).configureScope(any())
assertNotSame(propagationContextAfterNewTrace, scope.propagationContext)
}

Expand Down
34 changes: 34 additions & 0 deletions sentry-android-core/src/test/java/io/sentry/android/core/internal/util/ClassUtilTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package io.sentry.android.core.internal.util

import java.util.concurrent.Callable
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNull
import kotlin.test.assertTrue

class ClassUtilTest {

class Outer {
class Inner {
val x: Callable<Boolean> = Callable<Boolean> { false }
}
}

@Test
fun `getClassName returns cannonical name by default`() {
val name = ClassUtil.getClassName(Outer.Inner())
assertEquals("io.sentry.android.core.internal.util.ClassUtilTest.Outer.Inner", name)
}

@Test
fun `getClassName falls back to simple name for anonymous classes`() {
val name = ClassUtil.getClassName(Outer.Inner().x)
assertTrue(name!!.contains("$"))
}

@Test
fun `getClassName returns null when obj is null`() {
val name = ClassUtil.getClassName(null)
assertNull(name)
}
}
5 changes: 5 additions & 0 deletions sentry/api/sentry.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,7 @@ public final class io/sentry/Scope {
public fun getLevel ()Lio/sentry/SentryLevel;
public fun getPropagationContext ()Lio/sentry/PropagationContext;
public fun getRequest ()Lio/sentry/protocol/Request;
public fun getScreen ()Ljava/lang/String;
public fun getSession ()Lio/sentry/Session;
public fun getSpan ()Lio/sentry/ISpan;
public fun getTags ()Ljava/util/Map;
Expand All @@ -1406,6 +1407,7 @@ public final class io/sentry/Scope {
public fun setLevel (Lio/sentry/SentryLevel;)V
public fun setPropagationContext (Lio/sentry/PropagationContext;)V
public fun setRequest (Lio/sentry/protocol/Request;)V
public fun setScreen (Ljava/lang/String;)V
public fun setTag (Ljava/lang/String;Ljava/lang/String;)V
public fun setTransaction (Lio/sentry/ITransaction;)V
public fun setTransaction (Ljava/lang/String;)V
Expand Down Expand Up @@ -3107,6 +3109,7 @@ public final class io/sentry/protocol/App : io/sentry/JsonSerializable, io/sentr
public fun getInForeground ()Ljava/lang/Boolean;
public fun getPermissions ()Ljava/util/Map;
public fun getUnknown ()Ljava/util/Map;
public fun getViewNames ()Ljava/util/List;
public fun hashCode ()I
public fun serialize (Lio/sentry/ObjectWriter;Lio/sentry/ILogger;)V
public fun setAppBuild (Ljava/lang/String;)V
Expand All @@ -3119,6 +3122,7 @@ public final class io/sentry/protocol/App : io/sentry/JsonSerializable, io/sentr
public fun setInForeground (Ljava/lang/Boolean;)V
public fun setPermissions (Ljava/util/Map;)V
public fun setUnknown (Ljava/util/Map;)V
public fun setViewNames (Ljava/util/List;)V
}

public final class io/sentry/protocol/App$Deserializer : io/sentry/JsonDeserializer {
Expand All @@ -3137,6 +3141,7 @@ public final class io/sentry/protocol/App$JsonKeys {
public static final field BUILD_TYPE Ljava/lang/String;
public static final field DEVICE_APP_HASH Ljava/lang/String;
public static final field IN_FOREGROUND Ljava/lang/String;
public static final field VIEW_NAMES Ljava/lang/String;
public fun <init> ()V
}

Expand Down
41 changes: 41 additions & 0 deletions sentry/src/main/java/io/sentry/Scope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.sentry;

import io.sentry.protocol.App;
import io.sentry.protocol.Contexts;
import io.sentry.protocol.Request;
import io.sentry.protocol.TransactionNameSource;
Expand Down Expand Up @@ -33,6 +34,9 @@
/** Scope's user */
private @Nullable User user;

/** Scope's screen */
private @Nullable String screen;

/** Scope's request */
private @Nullable Request request;

Expand Down Expand Up @@ -97,6 +101,7 @@

final User userRef = scope.user;
this.user = userRef != null ? new User(userRef) : null;
this.screen = scope.screen;

final Request requestRef = scope.request;
this.request = requestRef != null ? new Request(requestRef) : null;
Expand Down Expand Up @@ -259,6 +264,41 @@
}
}

/**
* Returns the Scope's current screen, previously set by {@link Scope#setScreen(String)}
*
* @return the name of the screen
*/
@ApiStatus.Internal
public @Nullable String getScreen() {
return screen;
}

/**
* Sets the Scope's current screen
*
* @param screen the name of the screen
*/
@ApiStatus.Internal
public void setScreen(final @Nullable String screen) {
this.screen = screen;

final @NotNull Contexts contexts = getContexts();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: I'm wondering if we should stick to our approach of enriching contexts in the DefaultAndroidEventProcessor?

Probably it should live here and we also would have to check !HintUtils.isFromHybridSdk(hint) because hybrid SDKs will set their own view_names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good point, but I still think it should live on the scope because it represents a current state, which should be readable by other components.
E.g. when a span gets created (e.g. by some user interaction, a slow/frozen frame, ...) we'd need a way to get the current screen for adding it as span context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine with screen living on the scope, just meant the actual setting it to Contexts should be happening in the event processor maybe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I looked into this now, but as far as I can see EventProcessors have no direct access to the scope / a hub. But SentryClient.applyScope could be a good place for this, wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh you're right. But we're anyway already doing that there

      final Contexts contexts = sentryBaseEvent.getContexts();
      for (Map.Entry<String, Object> entry : new Contexts(scope.getContexts()).entrySet()) {
        if (!contexts.containsKey(entry.getKey())) {
          contexts.put(entry.getKey(), entry.getValue());
        }
      }

so I think it's fine to keep it as you initially implemented 👍

@Nullable App app = contexts.getApp();
if (app == null) {
app = new App();
contexts.setApp(app);
}

if (screen == null) {
app.setViewNames(null);

Check warning on line 294 in sentry/src/main/java/io/sentry/Scope.java

View check run for this annotation

Codecov / codecov/patch

sentry/src/main/java/io/sentry/Scope.java#L294 

Added line #L294 was not covered by tests
} else {
final @NotNull List<String> viewNames = new ArrayList<>(1);
viewNames.add(screen);
app.setViewNames(viewNames);
}
}

/**
* Returns the Scope's request
*
Expand Down Expand Up @@ -426,6 +466,7 @@
level = null;
user = null;
request = null;
screen = null;
fingerprint.clear();
clearBreadcrumbs();
tags.clear();
Expand Down
Loading
Loading