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: Add slow and frozen frames to spans #3450

Merged
merged 11 commits into from
Dec 5, 2023

Conversation

philipphofmann
Copy link
Member

📜 Description

Add total, slow and frozen frame numbers to span data.

💡 Motivation and Context

Fixes GH-3448

💚 How did you test it?

Unit tests and simulator.

📝 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

Add total, slow and frozen frame numbers to span data.

Fixes GH-3448
Copy link

github-actions bot commented Nov 27, 2023

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

Generated by 🚫 dangerJS against a9a790d

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #3450 (a9a790d) into main (b9a9dca) will increase coverage by 0.078%.
The diff coverage is 99.375%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3450       +/-   ##
=============================================
+ Coverage   88.973%   89.052%   +0.078%     
=============================================
  Files          525       525               
  Lines        56736     56906      +170     
  Branches     20413     20460       +47     
=============================================
+ Hits         50480     50676      +196     
+ Misses        5334      5312       -22     
+ Partials       922       918        -4     
Files Coverage Δ
SentryTestUtils/TestDisplayLinkWrapper.swift 94.871% <100.000%> (ø)
Sources/Sentry/SentryBuildAppStartSpans.m 95.000% <100.000%> (ø)
Sources/Sentry/SentryFramesTracker.m 98.601% <100.000%> (+0.050%) ⬆️
Sources/Sentry/SentrySpan.m 98.395% <100.000%> (+2.690%) ⬆️
Tests/SentryTests/PrivateSentrySDKOnlyTests.swift 98.795% <100.000%> (ø)
Tests/SentryTests/SentryClientTests.swift 95.965% <100.000%> (ø)
...ests/SentryTests/Transaction/SentrySpanTests.swift 99.405% <100.000%> (+0.144%) ⬆️
...ts/SentryTests/Transaction/SentryTracerTests.swift 98.834% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.456% <92.307%> (+0.256%) ⬆️

... and 16 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 b9a9dca...a9a790d. Read the comment docs.

Copy link

github-actions bot commented Nov 27, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.43 ms 1263.60 ms 19.17 ms
Size 21.58 KiB 414.96 KiB 393.38 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8f397a7 1219.12 ms 1236.67 ms 17.55 ms
90d17d3 1261.18 ms 1278.18 ms 17.00 ms
44ce888 1208.98 ms 1224.72 ms 15.74 ms
5f8ee7a 1249.48 ms 1252.20 ms 2.72 ms
154f795 1250.38 ms 1274.54 ms 24.16 ms
e998fd0 1254.41 ms 1272.78 ms 18.37 ms
e71cf92 1201.69 ms 1226.52 ms 24.83 ms
f0283e8 1253.36 ms 1263.12 ms 9.76 ms
6c31077 1233.80 ms 1245.34 ms 11.54 ms
407ff99 1225.49 ms 1232.88 ms 7.39 ms

App size

Revision Plain With Sentry Diff
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
90d17d3 20.76 KiB 432.17 KiB 411.41 KiB
44ce888 22.85 KiB 414.65 KiB 391.80 KiB
5f8ee7a 22.85 KiB 411.93 KiB 389.08 KiB
154f795 20.76 KiB 435.25 KiB 414.49 KiB
e998fd0 21.58 KiB 414.59 KiB 393.01 KiB
e71cf92 20.76 KiB 419.85 KiB 399.10 KiB
f0283e8 20.76 KiB 393.36 KiB 372.60 KiB
6c31077 22.84 KiB 401.65 KiB 378.81 KiB
407ff99 20.76 KiB 427.87 KiB 407.10 KiB

Previous results on branch: feat/slow-frozen-frames-to-spans

Startup times

Revision Plain With Sentry Diff
b7546b9 1227.57 ms 1247.26 ms 19.69 ms
abb12f7 1213.14 ms 1230.54 ms 17.40 ms
d3cf395 1245.84 ms 1260.40 ms 14.56 ms
e688a62 1223.88 ms 1233.74 ms 9.86 ms

App size

Revision Plain With Sentry Diff
b7546b9 22.85 KiB 413.59 KiB 390.74 KiB
abb12f7 22.85 KiB 413.59 KiB 390.74 KiB
d3cf395 22.85 KiB 414.45 KiB 391.60 KiB
e688a62 22.85 KiB 413.59 KiB 390.74 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I offered suggestions for using the Nimble expectation DSL, but it works out to the same failure messages if they fail so you can take those or leave them. Couple other questions/suggestions.

Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Comment on lines +517 to +519
expect(sut.data["frames.total"]) == nil
expect(sut.data["frames.slow"]) == nil
expect(sut.data["frames.frozen"]) == nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(sut.data["frames.total"]) == nil
expect(sut.data["frames.slow"]) == nil
expect(sut.data["frames.frozen"]) == nil
expect(sut.data["frames.total"]).to(beNil())
expect(sut.data["frames.slow"]).to(beNil())
expect(sut.data["frames.frozen"]).to(beNil())

Comment on lines +532 to +534
expect(sut.data["frames.total"]) == nil
expect(sut.data["frames.slow"]) == nil
expect(sut.data["frames.frozen"]) == nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(sut.data["frames.total"]) == nil
expect(sut.data["frames.slow"]) == nil
expect(sut.data["frames.frozen"]) == nil
expect(sut.data["frames.total"]).to(beNil())
expect(sut.data["frames.slow"]).to(beNil())
expect(sut.data["frames.frozen"]).to(beNil())

Comment on lines +485 to +487
expect(sut.data["frames.total"] as? NSNumber) == NSNumber(value: slow + frozen + normal)
expect(sut.data["frames.slow"] as? NSNumber) == NSNumber(value: slow)
expect(sut.data["frames.frozen"] as? NSNumber) == NSNumber(value: frozen)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(sut.data["frames.total"] as? NSNumber) == NSNumber(value: slow + frozen + normal)
expect(sut.data["frames.slow"] as? NSNumber) == NSNumber(value: slow)
expect(sut.data["frames.frozen"] as? NSNumber) == NSNumber(value: frozen)
expect(sut.data["frames.total"] as? NSNumber).to(equal(NSNumber(value: slow + frozen + normal)))
expect(sut.data["frames.slow"] as? NSNumber).to(equal(NSNumber(value: slow)))
expect(sut.data["frames.frozen"] as? NSNumber).to(equal(NSNumber(value: frozen)))

Tests/SentryTests/Transaction/SentrySpanTests.swift Outdated Show resolved Hide resolved
Tests/SentryTests/Transaction/SentrySpanTests.swift Outdated Show resolved Hide resolved
Tests/SentryTests/Transaction/SentrySpanTests.swift Outdated Show resolved Hide resolved
@philipphofmann
Copy link
Member Author

philipphofmann commented Nov 28, 2023

Edit: we decided to move forward with this PR. I moved the problems to the GH issue here getsentry/team-mobile#156 (comment).

@philipphofmann philipphofmann merged commit ef5821b into main Dec 5, 2023
68 of 70 checks passed
@philipphofmann philipphofmann deleted the feat/slow-frozen-frames-to-spans branch December 5, 2023 08:06
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 number of slow and frozen frames to span data
3 participants