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

ref: Use SentryTracerConfiguration to initialize SentryTracer #2821

Merged
merged 16 commits into from
Apr 5, 2023
Merged

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Mar 21, 2023

Instead of having multiple functions to initialize SentryTracer with different configurations we should we one complex object that contains all the configurations.

#skip-changelog

@github-actions
Copy link

github-actions bot commented Mar 21, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1203.43 ms 1228.20 ms 24.77 ms
Size 20.76 KiB 431.13 KiB 410.37 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
005bb4c 1237.38 ms 1255.54 ms 18.16 ms
8f397a7 1236.76 ms 1256.76 ms 20.00 ms
d80d410 1231.50 ms 1240.14 ms 8.64 ms
3f1be0f 1208.12 ms 1225.72 ms 17.60 ms
f576153 1210.02 ms 1228.94 ms 18.92 ms
b6ba04e 1217.45 ms 1248.92 ms 31.47 ms
eaa1002 1205.50 ms 1235.08 ms 29.58 ms
e79ead7 1216.91 ms 1247.06 ms 30.15 ms
e5ac362 1204.12 ms 1229.41 ms 25.29 ms
c6504da 1232.06 ms 1243.28 ms 11.22 ms

App size

Revision Plain With Sentry Diff
005bb4c 20.76 KiB 419.70 KiB 398.94 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
d80d410 20.76 KiB 425.71 KiB 404.95 KiB
3f1be0f 20.76 KiB 414.44 KiB 393.69 KiB
f576153 20.76 KiB 425.77 KiB 405.01 KiB
b6ba04e 20.76 KiB 414.45 KiB 393.69 KiB
eaa1002 20.76 KiB 423.19 KiB 402.43 KiB
e79ead7 20.76 KiB 426.11 KiB 405.35 KiB
e5ac362 20.76 KiB 426.11 KiB 405.34 KiB
c6504da 20.76 KiB 414.44 KiB 393.69 KiB

Previous results on branch: feat/inp

Startup times

Revision Plain With Sentry Diff
8352738 1221.84 ms 1241.44 ms 19.60 ms

App size

Revision Plain With Sentry Diff
8352738 20.76 KiB 426.57 KiB 405.80 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

I like the idea of using a builder pattern for the initialization of the tracer, as it's getting more and more complex. In languages like Swift, Java, C# the builder pattern would make sense, but for Objective-C, this doesn't work like that. I like the approach in this article. Instead of the SentryTracerConfiguration, we could use a normal Objective-C class, which you then pass to the tracer on init. I wouldn't copy every parameter as the downside is that when adding a new parameter you need to change it in two locations. The advantage of an Objective-C class is that we can have default values for parameters, and create a constructor with the required parameters.

I would do this refactoring in an extra PR to merge it quickly and not in the INP PR.

@brustolin brustolin changed the title feat: Interaction to next print measure ref: Use a SentryTracerConfiguration to initialize SentryTracer Mar 21, 2023
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

I like the direction. It's definitely an improvement.

@brustolin brustolin changed the title ref: Use a SentryTracerConfiguration to initialize SentryTracer ref: Use SentryTracerConfiguration to initialize SentryTracer Mar 21, 2023
@brustolin brustolin marked this pull request as ready for review March 21, 2023 19:40
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #2821 (e4766d3) into main (cf724da) will increase coverage by 0.06%.
The diff coverage is 95.40%.

❗ Current head e4766d3 differs from pull request most recent head 4c23ae0. Consider uploading reports for the commit 4c23ae0 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2821      +/-   ##
==========================================
+ Coverage   81.37%   81.43%   +0.06%     
==========================================
  Files         259      261       +2     
  Lines       24356    24343      -13     
  Branches    10688    10810     +122     
==========================================
+ Hits        19819    19824       +5     
+ Misses       4032     4015      -17     
+ Partials      505      504       -1     
Impacted Files Coverage Δ
Sources/Sentry/SentryTracer.m 96.58% <92.15%> (-0.19%) ⬇️
Sources/Sentry/SentryHub.m 95.87% <100.00%> (-0.19%) ⬇️
Sources/Sentry/SentryPerformanceTracker.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryTracerConfiguration.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryUIEventTracker.m 100.00% <100.00%> (ø)

... and 11 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 cf724da...4c23ae0. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann 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 refactoring 👍. LGTM

@brustolin brustolin merged commit 33b2f51 into main Apr 5, 2023
@brustolin brustolin deleted the feat/inp branch April 5, 2023 15:02
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.

4 participants