Skip to content

fix: Remove internal unknown dict for Breadcrumbs #4803

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

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Feb 6, 2025

📜 Description

Remove unknown dict when initializing the crumbs with a dict added in #2820. The Java SDK, for example, implements this pattern to keep unknown JSON properties when deserializing and serializing classes. This allows more flexibility when a hybrid SDK or the native SDK serializes an event to disk, and the Java SDK deserializes this event. With the unknown dict, the Java SDK doesn't need to implement all the properties other SDKs have. We only have this for crumbs on the Cocoa SDK, and it doesn't make sense only to keep it for one class. Instead, we removed it to simplify the code because we would need to implement some complex deserialization logic with Swift Decodable for deserializing events. We can always reconsider this approach, but then we would need to add the unknown property concept for all serializable/decodable classes.

💡 Motivation and Context

Came up in #4785

Communicated to .NET, RN and Flutter on Slack.

💚 How did you test it?

Unit tests.

📝 Checklist

You have to check all boxes before merging:

  • 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.
  • I updated the wizard 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

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
philipphofmann Philipp Hofmann
Remove unknown dict when initializing the crumbs with a dict added in
#2820. The Java SDK, for example, implements this pattern to keep
unknown JSON properties when deserializing and serializing classes. This
allows more flexibility when a hybrid SDK or the native SDK serializes
an event to disk, and the Java SDK deserializes this event. With the
unknown dict, the Java SDK doesn't need to implement all the properties
other SDKs have. We only have this for crumbs on the Cocoa SDK, and it
doesn't make sense only to keep it for one class. Instead, we removed it
to simplify the code because we would need to implement some complex
deserialization logic with Swift Decodable for deserializing events. We
can always reconsider this approach, but then we would need to add the
unknown property concept for all serializable/decodable classes.
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.77 ms 1235.31 ms 18.54 ms
Size 22.31 KiB 780.75 KiB 758.44 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8f397a7 1219.12 ms 1236.67 ms 17.55 ms
00cbe5c 1233.57 ms 1248.24 ms 14.67 ms
97de6c8 1226.78 ms 1240.98 ms 14.20 ms
72c8d84 1257.34 ms 1276.96 ms 19.62 ms
ef5821b 1253.18 ms 1265.46 ms 12.28 ms
e324230 1230.35 ms 1236.45 ms 6.10 ms
89b12eb 1236.02 ms 1246.63 ms 10.61 ms
7cd187e 1239.02 ms 1261.42 ms 22.40 ms
7f14650 1236.00 ms 1255.66 ms 19.66 ms
1734d1b 1198.69 ms 1221.62 ms 22.93 ms

App size

Revision Plain With Sentry Diff
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
00cbe5c 21.58 KiB 573.16 KiB 551.58 KiB
97de6c8 21.58 KiB 613.99 KiB 592.41 KiB
72c8d84 22.85 KiB 408.88 KiB 386.03 KiB
ef5821b 21.58 KiB 414.96 KiB 393.37 KiB
e324230 22.85 KiB 408.87 KiB 386.02 KiB
89b12eb 20.76 KiB 432.88 KiB 412.11 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
7f14650 22.84 KiB 402.63 KiB 379.78 KiB
1734d1b 21.58 KiB 418.82 KiB 397.24 KiB

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.392%. Comparing base (65547cc) to head (2ba8868).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4803       +/-   ##
=============================================
- Coverage   91.401%   91.392%   -0.010%     
=============================================
  Files          627       627               
  Lines        74575     74553       -22     
  Branches     26834     26823       -11     
=============================================
- Hits         68163     68136       -27     
- Misses        6316      6321        +5     
  Partials        96        96               
Files with missing lines Coverage Δ
Sources/Sentry/SentryBreadcrumb.m 96.842% <ø> (+0.445%) ⬆️
...s/SentryTests/Protocol/SentryBreadcrumbTests.swift 100.000% <100.000%> (ø)

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65547cc...2ba8868. Read the comment docs.

@philipphofmann philipphofmann self-assigned this Feb 6, 2025
Copy link
Contributor

@philprime philprime left a comment

Choose a reason for hiding this comment

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

LGTM

@philipphofmann philipphofmann merged commit c9c88fa into main Feb 6, 2025
70 of 73 checks passed
@philipphofmann philipphofmann deleted the impr/remove-crumb-unknown branch February 6, 2025 10:25
@krystofwoldrich
Copy link
Contributor

RN doesn't use any unknown fields.

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