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

perf: Don't attach headers for SentryNoOpSpan #2498

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

kevinrenskers
Copy link
Contributor

@kevinrenskers kevinrenskers commented Dec 6, 2022

📜 Description

Don't attach headers for SentryNoOpSpan. A small performance gain, but no other functional impact.

#skip-changelog

💡 Motivation and Context

Closes #2401.

💚 How did you test it?

Before I add tests, I'd like to know if this is indeed the correct way to go about this.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1237.51 ms 1253.90 ms 16.39 ms
Size 20.75 KiB 404.82 KiB 384.07 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
c6fb5a9 1194.80 ms 1232.68 ms 37.88 ms
010583c 1198.23 ms 1225.52 ms 27.29 ms
a9e77dc 1231.94 ms 1254.85 ms 22.91 ms
f91714d 1222.06 ms 1247.00 ms 24.94 ms
d446105 1237.06 ms 1261.34 ms 24.28 ms
2ae7db9 1231.37 ms 1239.98 ms 8.61 ms
ffa889e 1229.37 ms 1253.38 ms 24.01 ms
8361c4c 1204.07 ms 1252.74 ms 48.67 ms
4dc66f6 1202.59 ms 1228.34 ms 25.75 ms
9be1db2 1219.42 ms 1245.66 ms 26.24 ms

App size

Revision Plain With Sentry Diff
c6fb5a9 20.75 KiB 383.76 KiB 363.01 KiB
010583c 20.75 KiB 383.61 KiB 362.85 KiB
a9e77dc 20.75 KiB 379.12 KiB 358.36 KiB
f91714d 20.75 KiB 381.87 KiB 361.12 KiB
d446105 20.75 KiB 383.37 KiB 362.62 KiB
2ae7db9 20.75 KiB 381.87 KiB 361.12 KiB
ffa889e 20.75 KiB 383.83 KiB 363.08 KiB
8361c4c 20.75 KiB 383.87 KiB 363.12 KiB
4dc66f6 20.75 KiB 381.81 KiB 361.06 KiB
9be1db2 20.75 KiB 373.94 KiB 353.19 KiB

Previous results on branch: fix/2401-no-headers-NoOpSpan

Startup times

Revision Plain With Sentry Diff
9d980f4 1215.96 ms 1241.36 ms 25.40 ms
bd20fde 1226.45 ms 1254.98 ms 28.53 ms

App size

Revision Plain With Sentry Diff
9d980f4 20.75 KiB 404.86 KiB 384.11 KiB
bd20fde 20.75 KiB 404.82 KiB 384.07 KiB

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #2498 (976772a) into 8.0.0 (6bb5919) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2498      +/-   ##
==========================================
- Coverage   78.44%   78.42%   -0.02%     
==========================================
  Files         242      242              
  Lines       22282    22277       -5     
  Branches     9839     9834       -5     
==========================================
- Hits        17479    17471       -8     
- Misses       4351     4354       +3     
  Partials      452      452              
Impacted Files Coverage Δ
Sources/Sentry/SentryNetworkTracker.m 94.25% <100.00%> (-1.10%) ⬇️

Continue to review full report at Codecov.

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

@kevinrenskers kevinrenskers marked this pull request as ready for review December 6, 2022 15:09
@kevinrenskers
Copy link
Contributor Author

Same here.

codecov/project Failing after 1s — 78.42% (-0.02%) compared to 6bb5919

Quite annoying to have a red cross in the PR list, think a test failed, scroll through all the workflows hunting for the cross.. and then it's this thing.

@kevinrenskers kevinrenskers merged commit 4a67681 into 8.0.0 Dec 7, 2022
@kevinrenskers kevinrenskers deleted the fix/2401-no-headers-NoOpSpan branch December 7, 2022 13:58
kevinrenskers added a commit that referenced this pull request Dec 9, 2022
* 8.0.0:
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  feat: Add synthetic for mechanism (#2501)
kevinrenskers added a commit that referenced this pull request Dec 16, 2022
* 8.0.0: (31 commits)
  tests: Reenable testAddAndRemoveData (#2533)
  feat: support SENTRY_DSN environment var on macOS (#2534)
  Remove duplicate entry (#2532)
  fix: ARC issue for FileManager (#2525)
  feat: Add SwiftUI performance tracking (#2271)
  fix: Remove all permission checks (#2529)
  Remove the automatic `viewAppearing` span (#2511)
  Fix and reenable testANRDetected_UpdatesAppStateToTrue (#2526)
  fix: Don't add out of date context for crashes (#2523)
  ref: Rename isOOM to watchdog in Client (#2520)
  test: Fix disabled failing watchdog test (#2521)
  build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#2516)
  Rename the watchdog option and integration (#2513)
  feat: Enable CaptureFailedRequests by default (#2507)
  test: Fix asserts for SentryCrashTestInstallation (#2500)
  meta: User interaction tracing enabled per default (#2506)
  ref: Rename OOM to Watchdog Terminations (#2499)
  feat: enableUserInteractionTracing is GA (#2503)
  build(deps): bump nokogiri from 1.13.9 to 1.13.10 (#2505)
  perf: Don't attach headers for SentryNoOpSpan (#2498)
  ...
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.

Dont attach headers for NoOpSpans
2 participants