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.

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

Add current activity name to app context #2999

Merged
merged 14 commits into from
Nov 8, 2023

Conversation

markushi
Copy link
Member

@markushi markushi commented Oct 19, 2023

📜 Description

Adds a screen property to the scope which gets synced to the app context.
Our ActivityLifecycleIntegration now sets the scope's screen on every Activity.onCreate.

As discussed let's keep the scope API private for now.

  • keep API private

💡 Motivation and Context

Fixes #2664

💚 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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

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

Generated by 🚫 dangerJS against 67adcc4

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 374.02 ms 451.00 ms 76.98 ms
Size 1.72 MiB 2.29 MiB 577.69 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3d8bd2b 375.34 ms 446.32 ms 70.98 ms
c7e2fbc 377.85 ms 426.35 ms 48.50 ms
3d8bd2b 364.76 ms 469.98 ms 105.22 ms
3d8bd2b 379.45 ms 450.36 ms 70.90 ms
c3f503e 360.41 ms 434.67 ms 74.27 ms
93a76ca 377.96 ms 447.52 ms 69.56 ms
86f0096 368.63 ms 446.92 ms 78.29 ms
a3c77bc 375.80 ms 445.85 ms 70.06 ms
93a76ca 378.48 ms 451.78 ms 73.30 ms
0404ea3 332.47 ms 401.12 ms 68.66 ms

App size

Revision Plain With Sentry Diff
3d8bd2b 1.72 MiB 2.29 MiB 577.53 KiB
c7e2fbc 1.72 MiB 2.29 MiB 576.40 KiB
3d8bd2b 1.72 MiB 2.29 MiB 577.53 KiB
3d8bd2b 1.72 MiB 2.29 MiB 577.53 KiB
c3f503e 1.72 MiB 2.29 MiB 577.04 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
86f0096 1.72 MiB 2.29 MiB 576.50 KiB
a3c77bc 1.72 MiB 2.29 MiB 577.53 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
0404ea3 1.72 MiB 2.29 MiB 577.52 KiB

Previous results on branch: feat/add-current-screen-to-app-context

Startup times

Revision Plain With Sentry Diff
b7abef4 384.65 ms 471.02 ms 86.37 ms
263f68f 416.78 ms 494.20 ms 77.42 ms
87f4326 386.96 ms 459.96 ms 73.00 ms
13aac53 375.00 ms 459.70 ms 84.70 ms
be5c36b 397.11 ms 474.31 ms 77.20 ms
19a0f51 377.24 ms 445.84 ms 68.60 ms
39500b5 363.14 ms 464.48 ms 101.34 ms

App size

Revision Plain With Sentry Diff
b7abef4 1.72 MiB 2.29 MiB 576.67 KiB
263f68f 1.72 MiB 2.29 MiB 576.67 KiB
87f4326 1.72 MiB 2.29 MiB 577.49 KiB
13aac53 1.72 MiB 2.29 MiB 576.59 KiB
be5c36b 1.72 MiB 2.29 MiB 576.67 KiB
19a0f51 1.72 MiB 2.29 MiB 576.67 KiB
39500b5 1.72 MiB 2.29 MiB 576.53 KiB

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...ntry/android/core/ViewHierarchyEventProcessor.java 82.64% <100.00%> (+1.34%) ⬆️
...rnal/gestures/AndroidViewGestureTargetLocator.java 78.78% <100.00%> (-1.22%) ⬇️
...try/android/core/ActivityLifecycleIntegration.java 83.10% <66.66%> (-0.17%) ⬇️
...java/io/sentry/android/core/InternalSentrySdk.java 88.17% <90.00%> (ø)
...o/sentry/android/core/internal/util/ClassUtil.java 85.71% <85.71%> (ø)
sentry/src/main/java/io/sentry/Scope.java 96.20% <86.66%> (-0.53%) ⬇️
sentry/src/main/java/io/sentry/protocol/App.java 86.71% <61.53%> (-1.22%) ⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

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 👍

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM, besides one comment.

  • Also, maybe could you update PersistingScopeObserver and AnrV2EventProcessor so we get this context for ANRv2 events?

@markushi
Copy link
Member Author

markushi commented Nov 8, 2023

LGTM, besides one comment.

  • Also, maybe could you update PersistingScopeObserver and AnrV2EventProcessor so we get this context for ANRv2 events?

I don't think there's a need for this, as screen is part of app context and thus already synced. But I added a test/fix to verify the changes are properly propagated.

@romtsn
Copy link
Member

romtsn commented Nov 8, 2023

LGTM, besides one comment.

  • Also, maybe could you update PersistingScopeObserver and AnrV2EventProcessor so we get this context for ANRv2 events?

I don't think there's a need for this, as screen is part of app context and thus already synced. But I added a test/fix to verify the changes are properly propagated.

Oh true, that was the missing part, now it looks good 👍

@markushi markushi merged commit 283d83e into main Nov 8, 2023
20 checks passed
@markushi markushi deleted the feat/add-current-screen-to-app-context branch November 8, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report as app context what's the current activity during the error
2 participants