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 SwiftUI performance tracking #2271

Merged
merged 164 commits into from
Dec 14, 2022
Merged

feat: Add SwiftUI performance tracking #2271

merged 164 commits into from
Dec 14, 2022

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Oct 10, 2022

📜 Description

Created a wrapper view to track performance of SwiftUI parts

💡 Motivation and Context

See #1737

💚 How did you test it?

With Sample app and UI Tests

📝 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

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.

Great job, @brustolin. The solution is so simple, and simple is hard to achieve.

CHANGELOG.md Show resolved Hide resolved
SentrySwiftUI.podspec Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryPerformanceView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryPerformanceView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryPerformanceView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryPerformanceView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryPerformanceView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryPerformanceView.swift Outdated Show resolved Hide resolved
///
///
@available(iOS 13, macOS 10.15, tvOS 13, watchOS 6.0, *)
public struct SentryPerformanceView<Content: View>: View {
Copy link
Member

Choose a reason for hiding this comment

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

m: We have some UITests for this class, but I think adding some unit tests would also be great. We could move all the logic to an internal class and write some unit tests for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most logics in here are related to SwiftUI events, I believe the best way to test this is in a UI test, unit tests would be just redundant, I could create a test for extractName, but it does not seems worth to have a unit test just for this, we are also testing its result in the UI test.

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@philipphofmann
Copy link
Member

philipphofmann commented Dec 2, 2022

As discussed in a call. We still need to

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #2271 (ce1f9b1) into 8.0.0 (52f226b) will increase coverage by 0.04%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            8.0.0    #2271      +/-   ##
==========================================
+ Coverage   78.39%   78.44%   +0.04%     
==========================================
  Files         241      241              
  Lines       22290    22273      -17     
  Branches     9842     9832      -10     
==========================================
- Hits        17475    17472       -3     
+ Misses       4361     4350      -11     
+ Partials      454      451       -3     
Impacted Files Coverage Δ
Sources/Sentry/SentryPerformanceTracker.m 100.00% <ø> (ø)
Sources/Sentry/SentryNetworkTracker.m 95.20% <0.00%> (-0.15%) ⬇️
Sources/Sentry/SentryClient.m 95.78% <0.00%> (ø)
Sources/Sentry/SentryANRTracker.m 100.00% <0.00%> (ø)
Sources/SentryCrash/Recording/Tools/fishhook.c 82.27% <0.00%> (+5.06%) ⬆️
Sources/Sentry/SentryTime.mm 63.15% <0.00%> (+5.26%) ⬆️
Sources/Sentry/NSLocale+Sentry.m 100.00% <0.00%> (+10.00%) ⬆️
Sources/Sentry/SentryRequestOperation.m 78.37% <0.00%> (+10.81%) ⬆️

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 52f226b...ce1f9b1. 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.

Some parts still need fixing.

Sources/SentrySwiftUI/SentryTracerView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryTracerView.swift Outdated Show resolved Hide resolved
Sources/SentrySwiftUI/SentryTracerView.swift Outdated Show resolved Hide resolved
brustolin and others added 2 commits December 13, 2022 10:03
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
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.

LGTM, thanks @brustolin 👏👍

@philipphofmann
Copy link
Member

CI is not happy, though @brustolin.

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.

LGTM

@brustolin brustolin merged commit b55055e into 8.0.0 Dec 14, 2022
@brustolin brustolin deleted the feat/swiftUI-tracking branch December 14, 2022 08:35
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.

4 participants