Add current activity name to app context#2999
Conversation
|
Performance metrics 🚀
|
| 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 ReportAttention:
... and 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
…etsentry/sentry-java into feat/add-current-screen-to-app-context
| public void setScreen(final @Nullable String screen) { | ||
| this.screen = screen; | ||
|
|
||
| final @NotNull Contexts contexts = getContexts(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
…etsentry/sentry-java into feat/add-current-screen-to-app-context
I don't think there's a need for this, as |
Oh true, that was the missing part, now it looks good 👍 |
📜 Description
Adds a screen property to the scope which gets synced to the app context.
Our
ActivityLifecycleIntegrationnow sets the scope's screen on everyActivity.onCreate.As discussed let's keep the scope API private for now.
💡 Motivation and Context
Fixes #2664
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps