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

[SR] Align breadcrumbs with frontend #3406

Open
wants to merge 2 commits into
base: rz/feat/session-replay-touch-events
Choose a base branch
from

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented May 3, 2024

#skip-changelog

📜 Description

  • Change categories and data according to this table (what frontend expects)
  • Had to introduce a new breadcrumb for battery status changes (with throttling of 1 minute, analogous to what iOS does)

💡 Motivation and Context

Part of getsentry/sentry#70065

Copy link
Contributor

github-actions bot commented May 3, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 399.17 ms 495.22 ms 96.05 ms
Size 1.70 MiB 2.33 MiB 639.92 KiB

Baseline results on branch: rz/feat/session-replay-touch-events

Startup times

Revision Plain With Sentry Diff
c5a385c 371.06 ms 433.14 ms 62.08 ms
0a9348e 330.94 ms 405.53 ms 74.59 ms
51792bd 461.35 ms 479.57 ms 18.22 ms

App size

Revision Plain With Sentry Diff
c5a385c 1.70 MiB 2.33 MiB 638.75 KiB
0a9348e 1.70 MiB 2.32 MiB 638.56 KiB
51792bd 1.70 MiB 2.33 MiB 640.05 KiB

Previous results on branch: rz/feat/session-replay-breadcrumbs-v2

Startup times

Revision Plain With Sentry Diff
2c855ae 366.32 ms 429.25 ms 62.93 ms

App size

Revision Plain With Sentry Diff
2c855ae 1.70 MiB 2.32 MiB 638.51 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -18,7 +18,7 @@ public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger
writer.value(name().toLowerCase(Locale.ROOT));
}

static final class Deserializer implements JsonDeserializer<SentryLevel> {
public static final class Deserializer implements JsonDeserializer<SentryLevel> {
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to mark it as @ApiStatus.Internal then?

if (isBatteryChanged) {
final Float batteryLevel = DeviceInfoUtil.getBatteryLevel(intent, options);
if (batteryLevel != null) {
breadcrumb.setData("level", batteryLevel);
Copy link
Member

Choose a reason for hiding this comment

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

float is fine here right? The notion doc is not 100% clear about this.

var breadcrumbMessage: String? = null
val breadcrumbCategory: String?
var breadcrumbCategory: String? = null
var breadcrumbLevel: SentryLevel? = null
val breadcrumbData = mutableMapOf<String, Any?>()
when {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@romtsn what do you think about extracting this to a class so we can override it from RN and Flutter to have custom mapping?

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.

None yet

3 participants