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: profile concurrent transactions #2105

Merged
merged 58 commits into from
Sep 24, 2022

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Aug 29, 2022

We currently only profile one transaction at a time–if subsequent transactions are begun, only the first will be profiled. Now we will start the profiler for a transaction, and keep it running until the last stacked transaction is finished.

This PR also moves the growing amount of profiling-related code from SentryTracer.m back into SentryProfiler.mm. There are three methods there that the tracer calls:

  • + [SentryProfiler startForSpanID:hub:]: will start a profiler if there's not one already running, and add the span id to the profiler's list of concurrent transactions it's tracking
  • + [SentryProfiler stopProfilingSpan:]:
    - if the provided span id is the last one being profiled, stops the profiler
    - otherwise, remove that span from tracking and continue profiling
  • + [SentryProfiler linkTransaction:]: grabs the appropriate profiler instance waiting on this transaction for a span it was profiling, and captures an envelope with the data it has gathered if it was the last transaction it was waiting on

Also moved the call to stop the profiler to after another early return in - [SentryTracer finishInternal:], which I wonder if this was causing premature stoppage of the profiler when UI event tracking was enabled, which I first noticed in the benchmark tests: https://github.com/getsentry/sentry-cocoa/pull/2105/files#diff-84fda3cf94bf21dd7ceabf3c0ced60d74c124620d5a8dfdd3b8661563a79ec49R447. Similarly, also moved the call to capture the profiling envelope there until after the early return that drops transactions due to pre-warm durations (with a function to do so: + [SentryProfiler dropTransaction:])

@armcknight

This comment was marked as outdated.

@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch 2 times, most recently from 061086e to a66d5ce Compare August 31, 2022 22:41
@github-actions
Copy link

github-actions bot commented Aug 31, 2022

Performance metrics 🚀

Plain With Sentry Diff
Startup time (ms) 1260.65 1274.22 13.57
Size (bytes) 21157 332695 311538

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.

@armcknight, can you please elaborate on why profiling uses mach_absoluate_time/clock_gettime_nsec_np(CLOCK_UPTIME_RAW)? I just gave this PR a glance because still WIP.

Sources/Sentry/Public/SentryAppStartMeasurement.h Outdated Show resolved Hide resolved
@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch from 895129a to 693bd04 Compare September 7, 2022 01:44
@armcknight
Copy link
Member Author

@philipphofmann I left some details in a previous comment on the timestamp stuff, but basically, that API does not have a fixed reference date like NSDate does, so they can't be compared against each other, but I needed to get the relative timestamps of transactions that are linked to a given profile.

@armcknight
Copy link
Member Author

Oh but you asked why profiling uses the system time instead of NSDate... I think because it's guaranteed to be monotonic whereas NSDate is not, and it's also much faster than NSDate, just a syscall vs alloc/initing new class instances. Check out https://kandelvijaya.com/2016/10/25/precisiontiminginios/

@armcknight armcknight marked this pull request as ready for review September 7, 2022 02:04
@armcknight armcknight changed the title wip: profiling concurrent transactions feat: profile concurrent transactions Sep 7, 2022
@github-actions
Copy link

github-actions bot commented Sep 7, 2022

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

Generated by 🚫 dangerJS against 573b656

@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch from c0fc528 to b02a820 Compare September 7, 2022 02:15
@armcknight

This comment was marked as outdated.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

First pass, it looks good.
A few comments to address.

Sources/Sentry/Public/SentrySpanProtocol.h Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryEvent.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryProfiler.mm Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
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.

Just gave this a quick glance, will do a full review on Monday.

Sources/Sentry/Public/SentryEvent.h Outdated Show resolved Hide resolved
@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch from 4c2631d to 0e48b2e Compare September 9, 2022 11:01
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.86 ms 1250.59 ms 14.73 ms
Size 20.51 KiB 335.49 KiB 314.98 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
5025d2e 1248.52 ms 1251.72 ms 3.20 ms
b869536 1250.37 ms 1274.84 ms 24.47 ms
5025d2e 1245.14 ms 1268.58 ms 23.44 ms
4a66f00 1259.84 ms 1281.66 ms 21.82 ms
4a66f00 1224.73 ms 1241.14 ms 16.41 ms
9fc2dd0 1246.14 ms 1275.00 ms 28.86 ms

App size

Revision Plain With Sentry Diff
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
b869536 20.51 KiB 331.79 KiB 311.28 KiB
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
9fc2dd0 20.50 KiB 331.79 KiB 311.28 KiB

Previous results on branch: armcknight/profile-concurrent-transactions

Startup times

Revision Plain With Sentry Diff
0264ffd 1202.52 ms 1240.36 ms 37.84 ms
586e4e8 1201.44 ms 1237.12 ms 35.68 ms
1db0272 1201.75 ms 1245.74 ms 43.99 ms
c7ab130 1237.20 ms 1242.43 ms 5.22 ms
962039b 1227.00 ms 1252.12 ms 25.12 ms
e951eb0 1198.49 ms 1228.20 ms 29.71 ms
fb3beab 1234.20 ms 1248.73 ms 14.53 ms

App size

Revision Plain With Sentry Diff
0264ffd 20.51 KiB 335.71 KiB 315.21 KiB
586e4e8 20.51 KiB 335.71 KiB 315.21 KiB
1db0272 20.51 KiB 335.71 KiB 315.21 KiB
c7ab130 20.51 KiB 335.71 KiB 315.20 KiB
962039b 20.51 KiB 335.72 KiB 315.21 KiB
e951eb0 20.51 KiB 335.71 KiB 315.21 KiB
fb3beab 20.51 KiB 335.71 KiB 315.21 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.

This is going to be a cool feature and looking forward to seeing this PR merged. I added a couple of comments.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/Sentry/PrivateSentrySDKOnly.m Outdated Show resolved Hide resolved
Sources/Sentry/include/SentryLog.h Outdated Show resolved Hide resolved
Tests/SentryTests/SentryHubTests.swift Show resolved Hide resolved
Sources/Sentry/SentryProfiler.mm Show resolved Hide resolved
Sources/Sentry/SentryProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch 3 times, most recently from c750768 to 1edb474 Compare September 16, 2022 04:58
Copy link
Member Author

@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.

@philipphofmann here are the changes to the test code that I moved from the hub tests to the new profiler swift tests. RE #2105 (comment)

Sources/Sentry/SentryProfiler.mm Show resolved Hide resolved
Sources/Sentry/SentryProfiler.mm Outdated Show resolved Hide resolved
Sources/Sentry/SentryProfiler.mm Show resolved Hide resolved
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 think the race condition still is possible, and I have a few concerns about the tests.

@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch from 50e8263 to d474029 Compare September 19, 2022 22:23
@armcknight armcknight force-pushed the armcknight/profile-concurrent-transactions branch from 492bc68 to 380d6e7 Compare September 22, 2022 20:39
@armcknight armcknight merged commit e43ce74 into master Sep 24, 2022
@armcknight armcknight deleted the armcknight/profile-concurrent-transactions branch September 24, 2022 02:18
armcknight added a commit that referenced this pull request Sep 26, 2022
armcknight added a commit that referenced this pull request Sep 26, 2022
armcknight added a commit that referenced this pull request Sep 26, 2022
kevinrenskers added a commit that referenced this pull request Sep 27, 2022
* master:
  release: 7.26.0
  meta: Fix Changelog concurrent transactions (#2229)
  build(deps): bump fastlane from 2.210.0 to 2.210.1 (#2224)
  Revert "feat: profile concurrent transactions (#2105)" (#2225)
  fix: Align core data span operations (#2222)
  test: Remove empty assert msg for AppStateTests (#2221)
  meta: Fix Changelog (#2219)
  ref: Add typealias for bytes (#2209)
  feat: profile concurrent transactions (#2105)
  ci: Readd cache for UI tests (#2215)

# Conflicts:
#	Sentry.xcodeproj/project.pbxproj
armcknight added a commit that referenced this pull request Oct 25, 2022
armcknight added a commit that referenced this pull request Oct 25, 2022
armcknight added a commit that referenced this pull request Oct 25, 2022
armcknight added a commit that referenced this pull request Oct 25, 2022
* Revert "Revert "feat: profile concurrent transactions (#2105)" (#2225)"

This reverts commit 4c1fab4.

* fix tests and test sdk logging
* provide default for no build numbers
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.

5 participants