ref: slice profiles per transaction

[armcknight]

📜 Description

#skip-changelog

This PR changes the interface between SentryTracer and SentryProfiler to always grab pertinent profiling data for each transaction and attach it as an additional envelope item (this was how the original implementation worked as well; the difference now is that it needs to work for concurrent transactions).

This will remove the span/transaction bookkeeping from SentryProfiler and move whatever is needed into SentryTracer, which will now be in full control of when to start and stop the profiler, and request data from it.

TODO

• Fix remaining broken unit tests
  • Fix the concurrent transaction test
  • Fix tests exercising timeout logic: need to callback to the tracer and prepare the profile payload with transaction info provided by the tracer and hold it there for when the transaction finally finishes, so the profile can be attached as an envelope item to the transaction
• Write unit test for trimming out frame render timestamps outside of transaction/profile bounds, in addition to the other profile payload info that gets trimmed

💡 Motivation and Context

Our current relationship of sending a separate profile envelope with many references to transactions resulted in a system that cannot know which profiles to keep or sample out with dynamic sampling. We need to be able to send associated profile data with each transaction envelope so that dynamic sampling will correctly keep and sample out profile data for transactions that it is already sampling on. This change allows SentryTracer to query SentryProfiler for the relevant profile data for each transaction it uploads.

💚 How did you test it?

Augment the concurrent transaction profile test to see if the correct samples and metrics are present in each resulting envelope. See testConcurrentProfilingTransactions.

📝 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]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1212.34 ms	1239.68 ms	27.34 ms
Size	20.76 KiB	424.45 KiB	403.69 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
ecd9ecd	1215.77 ms	1238.70 ms	22.93 ms
156e771	1228.06 ms	1242.64 ms	14.58 ms
cb2eefe	1222.04 ms	1243.04 ms	21.00 ms
ecd9ecd	1214.16 ms	1232.59 ms	18.43 ms
7fb7afb	1227.73 ms	1243.16 ms	15.43 ms
5cabf7e	1247.12 ms	1249.61 ms	2.49 ms
8f397a7	1230.10 ms	1253.88 ms	23.77 ms
443723a	1205.24 ms	1220.52 ms	15.28 ms
7c5d161	1224.38 ms	1249.66 ms	25.28 ms
8f397a7	1236.76 ms	1256.76 ms	20.00 ms

App size

Revision	Plain	With Sentry	Diff
ecd9ecd	20.76 KiB	420.23 KiB	399.47 KiB
156e771	20.76 KiB	419.70 KiB	398.94 KiB
cb2eefe	20.76 KiB	419.62 KiB	398.86 KiB
ecd9ecd	20.76 KiB	420.23 KiB	399.47 KiB
7fb7afb	20.76 KiB	419.70 KiB	398.94 KiB
5cabf7e	20.76 KiB	419.67 KiB	398.91 KiB
8f397a7	20.76 KiB	420.55 KiB	399.79 KiB
443723a	20.76 KiB	414.44 KiB	393.68 KiB
7c5d161	20.76 KiB	414.44 KiB	393.68 KiB
8f397a7	20.76 KiB	420.55 KiB	399.79 KiB

Previous results on branch: armcknight/ref/slice-profiles-per-transaction

Startup times

Revision	Plain	With Sentry	Diff
c273b7c	1246.28 ms	1249.33 ms	3.05 ms
ad55419	1250.39 ms	1268.28 ms	17.89 ms
4733020	1226.61 ms	1231.58 ms	4.97 ms
dd6138f	1220.30 ms	1244.82 ms	24.52 ms
9cef0d4	1211.18 ms	1228.69 ms	17.51 ms

App size

Revision	Plain	With Sentry	Diff
c273b7c	20.76 KiB	420.75 KiB	399.99 KiB
ad55419	20.75 KiB	409.16 KiB	388.40 KiB
4733020	20.76 KiB	420.74 KiB	399.98 KiB
dd6138f	20.75 KiB	408.86 KiB	388.11 KiB
9cef0d4	20.75 KiB	408.99 KiB	388.24 KiB

[philipphofmann]

I like the approach. I added a few high-level comments.

[philipphofmann]

This PR is huge. I'm sorry, but I don't have the time today to give it a full pass. I'm going to have another look tomorrow.

[philipphofmann]

@armcknight, I merged main into this branch as I did some flaky test fixes recently. As some of the tests were failing on this branch, I want to eliminate some of the flakiness.

[philipphofmann]

That PR has loads of great code in it 🥇. I think we are close to an LGTM. CI is still a bit unhappy. I also made a couple of nit picks l, which you can also ignore if you want.:

[philipphofmann]

m: The file should be located in the include folder. I really should write a linter for that.

[philipphofmann]

m: The first time you call this, it's nil and hence the lock won't work if I'm not mistaken.

[armcknight]

Ooh good catch. @synchronizeing on the data structure didn't feel right anyways. I replaced this with a lock_guard

[philipphofmann]

LGTM, please address the locking issue (#2545 (comment)) in SentryTracerConcurrency before merging.

[philipphofmann]

Thanks 👏