feat: Add support for profiling on iOS #1652

armcknight · 2022-01-27T02:01:36Z

📜 Description

Integrates a subset of the Specto iOS SDK code to implement support for profiling on iOS. This PR contains the changes to bring in that existing code and also integrate it with the Sentry SDK such that profiles are captured alongside transactions.

💡 Motivation and Context

The profiling product will first be available on iOS & Android, based on tech acquired from Specto. This is the first attempt at integrating that functionality into the Sentry Cocoa SDK.

💚 How did you test it?

Add new automated tests, build and run in the iOS-Swift test app and ensure that the envelope generated with the new profiling envelope item contains the right data.

📝 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

Add tests for SentryProfiler
Add the debug_meta field so the backend can symbolicate
Add other metadata fields (device/OS metadata etc.) to the profile payload
Improve the efficiency of the profiler -- right now the entire thing is buffered in memory using inefficient data structures

to disambiguate from log/src/Log.{h,cpp}; they became ambiguous after putting them in the same target vs separate bazel targets, as well as the includes being the same name after removing the bazel path prefixes

it seemed to become ambiguous with stdlib <time.h> and caused a build error: 'chrono' file not found

…en all

- change all files from pure C++ to Objective-C++ (.mm extensions) - using the polyfills instead of protobufs - add a few more needed files - remove spdlog and fmt for now, remove all log logic and define the macros to no-ops - fix the cast to dispatch_queue_t in ThreadHandle that was breaking under ARC

at the point where Specto used to serialize the protobuf to data and write it to packets to the ring buffer, we now create a NSDictionary containing all the same fields with original names and serialize that to NSData that is then written to a SentryEnvelope add old configuration options like sampling rate, wether profiling is enabled etc into SentryOptions

github-actions · 2022-01-27T02:02:05Z

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

Generated by 🚫 dangerJS against c3a5bc7

philipphofmann

I'm thrilled to see the first draft PR from Specto 🎉, @armcknight.

FYI, we use the logaf scale for PR review comments. In short, we start the comments with l,m,h standing for the importance of the comment low, medium, high. Low are just nitpicks, medium is worth addressing and discussing, and high is something really important.

I didn't take a close look at the profiling code, because I guess this code was already reviewed. If you have tests for the profiling code I think we should also add them and run them in CI.

@armcknight CI is complaining, so I think we still need to adapt the Sentry.podspec and Package.swift.

For the SentryOptions we still need tests. For the BOOL properties, you can add tests as simple as

sentry-cocoa/Tests/SentryTests/SentryOptionsTest.m

Lines 176 to 179 in 06f40f9

    
           - (void)testEnableNetworkBreadcrumbs 
        
           { 
        
               [self testBooleanField:@"enableNetworkBreadcrumbs"]; 
        
           }

Sources/Sentry/Profiling/MemorySafety.mm

Sources/Sentry/Profiling/SpectoProtoPolyfills.mm

Sources/Sentry/Profiling/SpectoTime.mm

Sources/Sentry/Profiling/ThreadMetadataCache.mm

Sources/Sentry/Public/SentryOptions.h

Sources/Sentry/SentryOptions.m

Sources/Configuration/Sentry.xcconfig

Sources/Sentry/Profiling/Backtrace.h

Sources/Sentry/Profiling/BacktracePlugin.h

Sources/Sentry/Profiling/BlockSink.h

Sources/Sentry/Profiling/CPU.h

Sources/Sentry/Profiling/Exception.h

Sources/Sentry/Profiling/LogHookSink.h

Sources/Sentry/Profiling/SpectoProtoPolyfills.h

Sources/Sentry/Profiling/SpectoTime.mm

Sources/Sentry/Profiling/ThreadPriority.h

to try to roll back from C++17, as that was when this was first supported

- remove __APPLE__ and __ANDROID__ specific gates - change NDEBUG (bazel) usages to DEBUG (Xcode)

codecov-commenter · 2022-02-01T06:58:16Z

Codecov Report

Merging #1652 (7a97916) into master (bf46d0a) will decrease coverage by 2.34%.
The diff coverage is 72.77%.

❗ Current head 7a97916 differs from pull request most recent head c3a5bc7. Consider uploading reports for the commit c3a5bc7 to get more accurate results

@@            Coverage Diff             @@
##           master    #1652      +/-   ##
==========================================
- Coverage   94.87%   92.53%   -2.35%     
==========================================
  Files         171      187      +16     
  Lines        7751     8657     +906     
==========================================
+ Hits         7354     8011     +657     
- Misses        397      646     +249

Impacted Files	Coverage Δ
Sources/Sentry/include/SentryAsyncSafeLogging.h	`0.00% <0.00%> (ø)`
Sources/Sentry/SentryMachLogging.cpp	`30.25% <30.25%> (ø)`
Sources/Sentry/SentryTime.mm	`60.00% <60.00%> (ø)`
Sources/Sentry/include/SentryProfilingLogging.hpp	`61.90% <61.90%> (ø)`
Sources/Sentry/SentryThreadHandle.cpp	`69.06% <69.06%> (ø)`
Sources/Sentry/include/SentryStackFrame.hpp	`72.72% <72.72%> (ø)`
Sources/Sentry/include/SentryMachLogging.hpp	`77.77% <77.77%> (ø)`
Sources/Sentry/SentryProfilingLogging.mm	`78.57% <78.57%> (ø)`
Sources/Sentry/SentrySamplingProfiler.cpp	`88.50% <88.50%> (ø)`
Sources/Sentry/SentryHub.m	`97.87% <90.00%> (-0.67%)`	⬇️
... and 13 more

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 bf46d0a...c3a5bc7. Read the comment docs.

indragiek · 2022-02-02T21:32:25Z

@philipphofmann I'm taking over this PR and will address all the comments since @armcknight is now out on paternity, will have an update soon! Thank you.

brustolin · 2022-02-03T08:26:27Z

@philipphofmann I'm taking over this PR and will address all the comments since @armcknight is now out on paternity, will have an update soon! Thank you.

Hey @indragiek, Phillip is out on vocation the entire month. If you need something, you can ping me or @bruno-garcia

…try-cocoa into armcknight/specto-cpp-port

philipphofmann

A few minor things. Thanks 🙏

philipphofmann · 2022-03-17T08:44:13Z

Tests/SentryTests/Performance/SentryTracerTests.swift

@@ -395,6 +395,42 @@ class SentryTracerTests: XCTestCase {
        XCTAssertEqual(["key": 0], sut.data as! [String: Int])
    }

+    #if os(iOS)


l:

Suggested change

#if os(iOS)

#if os(iOS) || os(tvOS)

Profiling doesn't work on tvOS because the necessary APIs are not exported, same with watchOS. See SENTRY_TARGET_PROFILING_SUPPORTED. I did notice this should have macOS added to it, I will make that change.

Changed this check to support macOS, iOS, and mac catalyst (but not tvOS or watchOS)

Sources/Sentry/SentryTracer.m

philipphofmann

LGTM 🚀

armcknight added 17 commits January 19, 2022 17:11

add all source files from Specto's C++ layer

6c0d53c

fixup! add all source files from Specto's C++ layer

6965c8f

convert includes from bazel-style paths to plain filenames

ef108ca

comment includes for generated protobuf headers we will leave behind

6ec3513

rename darwin/log/src/Log.{h,cpp} to darwin/log/src/DarwinLog.{h,cpp}

1ef3854

to disambiguate from log/src/Log.{h,cpp}; they became ambiguous after putting them in the same target vs separate bazel targets, as well as the includes being the same name after removing the bazel path prefixes

rename Time.{h,cpp} to SpectoTime.{h,cpp}

7b505fc

it seemed to become ambiguous with stdlib <time.h> and caused a build error: 'chrono' file not found

only keep sources needed for SamplingProfiler/Backtrace, flatten all

d290692

fixup! only keep sources needed for SamplingProfiler/Backtrace, flatt…

2c63bb3

…en all

add settings to get the C++ sources compiling

c1f86a8

add protobuf polyfills

c121003

fixup! add settings to get the C++ sources compiling

3e63321

fixup! add protobuf polyfills

6473816

add BacktracePlugin source from specto

2eea5c7

sort files by name

9446af5

integrate backtrace profiler with sentry sdk initialization

be34635

Format code

f8dd090

philipphofmann reviewed Jan 27, 2022

View reviewed changes

indragiek requested changes Jan 27, 2022

View reviewed changes

armcknight added 5 commits January 28, 2022 13:37

resolve crashes and other issues constructing the envelope

85bc94f

remove commented QoS code

7e44e4f

un-nest namespace declarations

b827063

to try to roll back from C++17, as that was when this was first supported

remove copyright comments

0f9112c

fix compilation gates

15d43ef

- remove __APPLE__ and __ANDROID__ specific gates - change NDEBUG (bazel) usages to DEBUG (Xcode)

Format code

5b87983

philipphofmann added Component: Profiling Type: Feature Request labels Mar 16, 2022

indragiek changed the title ~~feat: iOS Profiler~~ feat: Add support for profiling on iOS Mar 16, 2022

indragiek and others added 13 commits March 16, 2022 16:03

Check SENTRY_TARGET_PROFILING_SUPPORTED in SentryOptions

0f421ce

Format code

0640557

Fix compilation of SentryOptions

273d94e

Merge branch 'armcknight/specto-cpp-port' of github.com:getsentry/sen…

4b8b17d

…try-cocoa into armcknight/specto-cpp-port

Add new SentryClient test for additionalEnvelopeItems

c18d122

Only allow one profiler to be running

d73ba85

Format code

e62358b

Fix SentryTracer build

ab83675

Merge branch 'armcknight/specto-cpp-port' of github.com:getsentry/sen…

10f12bd

…try-cocoa into armcknight/specto-cpp-port

Add tests for changes to SentryTracer

4b30af9

Fix lint

eb925f8

Fix SentryTracerTests

3d2a10a

Remove unnecessary overrides

ef1e1e0

philipphofmann reviewed Mar 17, 2022

View reviewed changes

indragiek added 2 commits March 17, 2022 13:55

Enable profiling tests on macOS

e26ba8f

Check enableProfiling before taking lock

5e4a6bc

philipphofmann approved these changes Mar 18, 2022

View reviewed changes

philipphofmann added 3 commits March 18, 2022 10:27

Merge branch 'master' into armcknight/specto-cpp-port

56cab69

Merge branch 'master' into armcknight/specto-cpp-port

7a97916

fix device test patch

c3a5bc7

philipphofmann changed the base branch from master to profiling March 21, 2022 15:48

philipphofmann merged commit cf5bbbb into profiling Mar 21, 2022

philipphofmann deleted the armcknight/specto-cpp-port branch March 21, 2022 15:48

This was referenced May 2, 2022

deleted triplef/sentry-cocoa#1

Closed

Use pod_target_xcconfig for Podspec #1792

Merged

bruno-garcia mentioned this pull request Jan 16, 2023

Initial support for profiling - internal release getsentry/sentry-dotnet#1955

Closed

bruno-garcia mentioned this pull request Apr 1, 2023

JVM Profiling getsentry/sentry-java#2635

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for profiling on iOS #1652

feat: Add support for profiling on iOS #1652

armcknight commented Jan 27, 2022 •

edited by indragiek

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

philipphofmann left a comment

codecov-commenter commented Feb 1, 2022 •

edited

Loading

indragiek commented Feb 2, 2022

brustolin commented Feb 3, 2022

philipphofmann left a comment

philipphofmann Mar 17, 2022

indragiek Mar 17, 2022

indragiek Mar 17, 2022

philipphofmann left a comment

	- (void)testEnableNetworkBreadcrumbs
	{
	[self testBooleanField:@"enableNetworkBreadcrumbs"];
	}

feat: Add support for profiling on iOS #1652

feat: Add support for profiling on iOS #1652

Conversation

armcknight commented Jan 27, 2022 • edited by indragiek Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jan 27, 2022 • edited Loading

philipphofmann left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 1, 2022 • edited Loading

Codecov Report

indragiek commented Feb 2, 2022

brustolin commented Feb 3, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Mar 17, 2022

Choose a reason for hiding this comment

indragiek Mar 17, 2022

Choose a reason for hiding this comment

indragiek Mar 17, 2022

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

armcknight commented Jan 27, 2022 •

edited by indragiek

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

codecov-commenter commented Feb 1, 2022 •

edited

Loading