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

feat: Adds breadcrumb.origin field #4358

Merged
merged 13 commits into from
Sep 26, 2024
Merged

feat: Adds breadcrumb.origin field #4358

merged 13 commits into from
Sep 26, 2024

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Sep 20, 2024

📜 Description

Adds a new SentryBreadcrumb optional private string property origin

💡 Motivation and Context

Fixes #4043
The discussion in getsentry/sentry-capacitor#629 (comment) adds some context on the need of a new field.

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

  • 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

Copy link

github-actions bot commented Sep 20, 2024

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

Generated by 🚫 dangerJS against 5da364f

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.465%. Comparing base (e2a3ac7) to head (5da364f).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4358       +/-   ##
=============================================
+ Coverage   91.426%   91.465%   +0.038%     
=============================================
  Files          630       631        +1     
  Lines        50668     50780      +112     
  Branches     18296     18406      +110     
=============================================
+ Hits         46324     46446      +122     
+ Misses        4252      4242       -10     
  Partials        92        92               
Files with missing lines Coverage Δ
Sources/Sentry/SentryBreadcrumb.m 97.701% <100.000%> (+0.170%) ⬆️
Sources/Sentry/SentryCrashReportConverter.m 95.737% <100.000%> (+0.014%) ⬆️
...s/SentryTests/Protocol/SentryBreadcrumbTests.swift 100.000% <100.000%> (ø)
Tests/SentryTests/SentryInterfacesTests.m 100.000% <100.000%> (ø)
Tests/SentryTests/SentryTests.m 100.000% <100.000%> (ø)

... and 21 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 e2a3ac7...5da364f. Read the comment docs.

Copy link

github-actions bot commented Sep 20, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.57 ms 1253.65 ms 28.08 ms
Size 21.58 KiB 714.38 KiB 692.80 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f4e0299 1230.33 ms 1249.68 ms 19.35 ms
2a894d5 1202.07 ms 1227.66 ms 25.59 ms
9f0d9e0 1206.55 ms 1219.41 ms 12.86 ms
8cf5bca 1212.35 ms 1223.90 ms 11.54 ms
3a31fc9 1237.35 ms 1249.02 ms 11.67 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
c471221 1224.16 ms 1241.59 ms 17.43 ms
156e771 1228.06 ms 1242.64 ms 14.58 ms
de033da 1216.91 ms 1222.84 ms 5.92 ms
f1c36e0 1215.18 ms 1223.62 ms 8.43 ms

App size

Revision Plain With Sentry Diff
f4e0299 20.76 KiB 427.54 KiB 406.78 KiB
2a894d5 21.58 KiB 414.57 KiB 392.99 KiB
9f0d9e0 21.58 KiB 424.28 KiB 402.70 KiB
8cf5bca 21.58 KiB 671.30 KiB 649.72 KiB
3a31fc9 20.76 KiB 414.45 KiB 393.69 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
c471221 22.85 KiB 413.89 KiB 391.04 KiB
156e771 20.76 KiB 419.70 KiB 398.94 KiB
de033da 21.58 KiB 418.15 KiB 396.57 KiB
f1c36e0 21.58 KiB 670.40 KiB 648.81 KiB

Previous results on branch: antonis-add-breadrumb-origin

Startup times

Revision Plain With Sentry Diff
7e1ee3a 1235.37 ms 1252.23 ms 16.86 ms

App size

Revision Plain With Sentry Diff
7e1ee3a 21.58 KiB 714.37 KiB 692.79 KiB

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

First pass, it looks good.

Just a few comments.

Sources/Sentry/Public/SentryBreadcrumb.h Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Thank you for reviewing @brustolin and the feedback 🙇
I applied your suggestions and it is ready for another pass.

Note that the Test / Unit iOS - Xcode 15.4 - OS 17.2 and Benchmarking / Collect app metrics CI checks are failing for me. I think they are flaky and I'll try to rerun. Let me know if you have more context since I'm new to the repo 🙏

Sources/Sentry/Public/SentryBreadcrumb.h Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryBreadcrumb.h Outdated Show resolved Hide resolved
@antonis antonis marked this pull request as ready for review September 24, 2024 09:36
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Thanks for this. Looks good!

@antonis
Copy link
Collaborator Author

antonis commented Sep 26, 2024

Thank you for the review and the feedback 🙇
Given that the Benchmarking / Collect app metrics is also failing on main at this point I'll proceed with merging.

@antonis antonis merged commit 3eb2070 into main Sep 26, 2024
65 of 66 checks passed
@antonis antonis deleted the antonis-add-breadrumb-origin branch September 26, 2024 10:31
@krystofwoldrich
Copy link
Member

Thank you for adding the field. @antonis

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.

Add breadrumb.origin
4 participants