Skip to content

Mutable Data Classes#2818

Merged
denrase merged 32 commits into
mainfrom
enha/immutable-sentry-event
Apr 3, 2025
Merged

Mutable Data Classes#2818
denrase merged 32 commits into
mainfrom
enha/immutable-sentry-event

Conversation

@denrase

@denrase denrase commented Mar 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

📜 Description

This PR changes all classes that provide a copyWith method to be mutable.

  • Users don't have to create a copy with copyWith just to assign a property a new value.
  • Users can now reset nullable values.
  • The copyWith and clone methods of sentry were deprecated

💡 Motivation and Context

Closes #2672

💚 How did you test it?

Run unit tests.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

This will also enable us to improve performance throughout the SDK, as less copying will be necessary. Will be done in a separate PR.

@codecov

codecov Bot commented Mar 24, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 91.57088% with 22 lines in your changes missing coverage. Please review.

Project coverage is 87.67%. Comparing base (b184b0b) to head (6f091e4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...cessor/exception/io_exception_event_processor.dart 72.72% 3 Missing ⚠️
dart/lib/src/protocol/mechanism.dart 33.33% 2 Missing ⚠️
flutter/lib/src/native/sentry_native_channel.dart 90.47% 2 Missing ⚠️
.../src/event_processor/enricher/flutter_runtime.dart 50.00% 1 Missing ⚠️
...rt/lib/src/load_dart_debug_images_integration.dart 50.00% 1 Missing ⚠️
dart/lib/src/protocol/debug_meta.dart 75.00% 1 Missing ⚠️
dart/lib/src/protocol/sentry_culture.dart 66.66% 1 Missing ⚠️
dart/lib/src/protocol/sentry_event.dart 0.00% 1 Missing ⚠️
dart/lib/src/protocol/sentry_feedback.dart 50.00% 1 Missing ⚠️
dart/lib/src/protocol/sentry_response.dart 50.00% 1 Missing ⚠️
... and 8 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2818      +/-   ##
==========================================
- Coverage   88.58%   87.67%   -0.92%     
==========================================
  Files         265      266       +1     
  Lines        8871     8897      +26     
==========================================
- Hits         7858     7800      -58     
- Misses       1013     1097      +84

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Mar 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1263.63 ms 1272.90 ms 9.27 ms
Size 8.43 MiB 9.99 MiB 1.56 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d089990 1206.19 ms 1233.08 ms 26.89 ms
6aab859 1245.14 ms 1247.59 ms 2.45 ms
dd5521e 1254.28 ms 1263.37 ms 9.09 ms
07cd9e8 1237.04 ms 1257.50 ms 20.46 ms
21845e2 1279.37 ms 1298.81 ms 19.45 ms
a7acb24 1296.71 ms 1317.69 ms 20.98 ms
e4d5aa8 1224.15 ms 1245.72 ms 21.57 ms
a61674e 1275.51 ms 1290.81 ms 15.30 ms
6034b0a 1244.89 ms 1270.22 ms 25.33 ms
73d70bf 1249.08 ms 1268.41 ms 19.33 ms

App size

Revision Plain With Sentry Diff
d089990 8.33 MiB 9.40 MiB 1.07 MiB
6aab859 8.29 MiB 9.36 MiB 1.07 MiB
dd5521e 8.43 MiB 9.97 MiB 1.54 MiB
07cd9e8 8.38 MiB 9.77 MiB 1.40 MiB
21845e2 8.15 MiB 9.12 MiB 991.34 KiB
a7acb24 8.16 MiB 9.17 MiB 1.01 MiB
e4d5aa8 8.33 MiB 9.62 MiB 1.29 MiB
a61674e 8.10 MiB 9.16 MiB 1.07 MiB
6034b0a 8.33 MiB 9.40 MiB 1.07 MiB
73d70bf 8.38 MiB 9.70 MiB 1.33 MiB

Previous results on branch: enha/immutable-sentry-event

Startup times

Revision Plain With Sentry Diff
b06ea5b 1247.69 ms 1261.51 ms 13.82 ms
7e9f265 1252.87 ms 1272.04 ms 19.17 ms
6c5e6ae 1263.75 ms 1276.71 ms 12.96 ms

App size

Revision Plain With Sentry Diff
b06ea5b 8.43 MiB 9.98 MiB 1.55 MiB
7e9f265 8.43 MiB 9.98 MiB 1.55 MiB
6c5e6ae 8.43 MiB 9.98 MiB 1.55 MiB

@denrase
denrase marked this pull request as ready for review March 24, 2025 15:55
buenaflor
buenaflor reviewed Mar 25, 2025
View reviewed changes

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good so far, only missing thing is deprecating the copyWith

Comment thread CHANGELOG.md
@github-actions

github-actions Bot commented Mar 25, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 497.77 ms 532.92 ms 35.15 ms
Size 6.44 MiB 7.43 MiB 1007.22 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
03e4c9b 410.34 ms 493.20 ms 82.86 ms
0a82a1e 321.02 ms 393.82 ms 72.80 ms
6fedcab 388.26 ms 487.42 ms 99.16 ms
803f3a9 424.66 ms 505.24 ms 80.58 ms
ffae3e3 360.67 ms 466.24 ms 105.57 ms
bf4aed7 311.24 ms 365.66 ms 54.42 ms
ddc97ad 331.45 ms 384.06 ms 52.61 ms
d53c6fa 282.83 ms 344.00 ms 61.17 ms
f922f8f 332.31 ms 374.67 ms 42.37 ms
547db82 453.40 ms 482.88 ms 29.48 ms

App size

Revision Plain With Sentry Diff
03e4c9b 6.35 MiB 7.42 MiB 1.07 MiB
0a82a1e 6.15 MiB 7.11 MiB 981.82 KiB
6fedcab 6.33 MiB 7.30 MiB 987.83 KiB
803f3a9 6.46 MiB 7.48 MiB 1.02 MiB
ffae3e3 6.26 MiB 7.20 MiB 958.78 KiB
bf4aed7 6.06 MiB 7.03 MiB 997.04 KiB
ddc97ad 6.16 MiB 7.14 MiB 1003.75 KiB
d53c6fa 6.16 MiB 7.14 MiB 1011.18 KiB
f922f8f 5.94 MiB 6.95 MiB 1.01 MiB
547db82 6.49 MiB 7.56 MiB 1.07 MiB

Previous results on branch: enha/immutable-sentry-event

Startup times

Revision Plain With Sentry Diff
b06ea5b 462.02 ms 530.60 ms 68.58 ms
6c5e6ae 500.48 ms 551.34 ms 50.86 ms
7e9f265 459.94 ms 517.74 ms 57.80 ms

App size

Revision Plain With Sentry Diff
b06ea5b 6.44 MiB 7.56 MiB 1.12 MiB
6c5e6ae 6.44 MiB 7.56 MiB 1.12 MiB
7e9f265 6.44 MiB 7.57 MiB 1.12 MiB

ueman
ueman reviewed Mar 25, 2025
View reviewed changes
Comment thread flutter/lib/src/event_processor/android_platform_exception_event_processor.dart Outdated
@denrase

denrase commented Mar 25, 2025

Copy link
Copy Markdown
Collaborator Author

@buenaflor Deprecated the copyWith and clone methods as well and replaced internal usage.

buenaflor
buenaflor reviewed Mar 26, 2025
View reviewed changes
Comment thread dart/lib/src/scope.dart Outdated
@denrase
denrase requested review from buenaflor and ueman March 27, 2025 08:55
buenaflor
buenaflor reviewed Apr 1, 2025
View reviewed changes

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't comment every place but we can use the .. cascade notation wherever it makes sense

looks good!

Comment thread dart/lib/src/sentry_client.dart
Comment thread dart/lib/src/event_processor/enricher/web_enricher_event_processor.dart Outdated
Comment thread flutter/lib/src/event_processor/flutter_enricher_event_processor.dart Outdated
@denrase
denrase requested a review from buenaflor April 2, 2025 08:18
buenaflor
buenaflor approved these changes Apr 2, 2025
View reviewed changes

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good, there's a bunch of package analysis errors but those are unrelated to the PR

👍

@buenaflor

buenaflor commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

@denrase feel free to merge when you're ready

@denrase
denrase merged commit 4c72876 into main Apr 3, 2025
@denrase
denrase deleted the enha/immutable-sentry-event branch April 3, 2025 13:32
@luis901101

luis901101 commented Jun 17, 2025

Copy link
Copy Markdown

Why deprecating copyWith? The motivation and context for making class mutable is clear, but having a copyWith option is also useful and covers different use case because it returns a new instance not the same instance.

@coderabbitai coderabbitai Bot mentioned this pull request May 7, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@buenaflor buenaflor buenaflor approved these changes
@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich
@stefanosiano stefanosiano Awaiting requested review from stefanosiano
@ueman ueman Awaiting requested review from ueman

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[v9]: Not possible to remove information in beforeSend (make SentryEvent and related classes mutable)

4 participants

@denrase @buenaflor @luis901101 @ueman