feat: add sample_rand / fix sample_rate in baggage #4751

philprime · 2025-01-24T11:41:13Z

📜 Description

Add sample_rand to the baggage
Fixed sample_rate not added to baggage
Add additional initializer tests for public interfaces

💡 Motivation and Context

See #4687

💚 How did you test it?

I kept the original tests to make sure the behaviour doesn't change
I added a lot of additional tests to ensure a stable API for the public headers
Used guidance here: https://github.com/getsentry/projects/issues/316#issuecomment-2612117275
CI
Checked the envelope header using a proxy debugging tool

📝 Checklist

You have to check all boxes before merging:

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.
I updated the wizard 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

Perform integrations tests

github-actions · 2025-01-24T11:41:38Z

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

Generated by 🚫 dangerJS against 6ad1eb7

Sources/Sentry/Profiling/SentryLaunchProfiling.m

codecov · 2025-01-24T11:49:05Z

Codecov Report

Attention: Patch coverage is 99.87700% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.306%. Comparing base (e76c341) to head (6ad1eb7).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
Sources/Sentry/SentrySampling.m	84.615%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4751       +/-   ##
=============================================
+ Coverage   91.197%   91.306%   +0.108%     
=============================================
  Files          623       624        +1     
  Lines        72505     74081     +1576     
  Branches     26407     26006      -401     
=============================================
+ Hits         66123     67641     +1518     
- Misses        6284      6349       +65     
+ Partials        98        91        -7

Files with missing lines	Coverage Δ
Sources/Sentry/Profiling/SentryLaunchProfiling.m	`91.129% <100.000%> (+0.145%)`	⬆️
Sources/Sentry/SentryBaggage.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryBuildAppStartSpans.m	`100.000% <ø> (ø)`
Sources/Sentry/SentryHub.m	`97.981% <100.000%> (+0.018%)`	⬆️
Sources/Sentry/SentrySamplerDecision.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryTraceContext.m	`96.644% <100.000%> (+2.289%)`	⬆️
Sources/Sentry/SentryTransactionContext.mm	`100.000% <100.000%> (ø)`
...yProfilerTests/SentryAppLaunchProfilingTests.swift	`100.000% <100.000%> (ø)`
.../SentryTests/Helper/SentrySerializationTests.swift	`99.807% <100.000%> (+0.010%)`	⬆️
...sts/SentryTests/Protocol/SentryEnvelopeTests.swift	`96.264% <100.000%> (+0.191%)`	⬆️
... and 8 more

... and 24 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 e76c341...6ad1eb7. Read the comment docs.

github-actions · 2025-01-24T11:53:07Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1226.31 ms	1245.65 ms	19.35 ms
Size	22.31 KiB	776.94 KiB	754.62 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`077e940`	1218.33 ms	1243.24 ms	24.92 ms
`f1ed6f8`	1210.94 ms	1230.78 ms	19.84 ms
`1f9387b`	1229.25 ms	1242.62 ms	13.37 ms
`35647ef`	1250.42 ms	1251.87 ms	1.46 ms
`b9a9ffd`	1221.18 ms	1235.37 ms	14.19 ms
`0001a09`	1223.90 ms	1249.56 ms	25.66 ms
`add9550`	1221.20 ms	1250.04 ms	28.84 ms
`c0b4b71`	1246.98 ms	1256.77 ms	9.79 ms
`061c982`	1226.33 ms	1243.65 ms	17.33 ms
`86e9bf2`	1225.70 ms	1252.19 ms	26.49 ms

App size

Revision	Plain	With Sentry	Diff
`077e940`	21.58 KiB	706.97 KiB	685.39 KiB
`f1ed6f8`	21.58 KiB	683.51 KiB	661.93 KiB
`1f9387b`	21.58 KiB	654.26 KiB	632.68 KiB
`35647ef`	21.58 KiB	706.97 KiB	685.39 KiB
`b9a9ffd`	21.58 KiB	418.43 KiB	396.85 KiB
`0001a09`	20.76 KiB	433.19 KiB	412.43 KiB
`add9550`	21.58 KiB	418.37 KiB	396.79 KiB
`c0b4b71`	20.76 KiB	430.98 KiB	410.22 KiB
`061c982`	21.58 KiB	683.51 KiB	661.93 KiB
`86e9bf2`	22.31 KiB	758.78 KiB	736.47 KiB

Previous results on branch: philprime/issue-4687-sampling-seed-propagation

Startup times

Revision	Plain	With Sentry	Diff
`544ce99`	1216.27 ms	1227.33 ms	11.06 ms
`a618a92`	1221.79 ms	1242.41 ms	20.62 ms
`ab1ebcb`	1214.52 ms	1235.14 ms	20.62 ms
`5400890`	1230.16 ms	1243.23 ms	13.07 ms
`63bb06c`	1216.49 ms	1237.31 ms	20.82 ms
`ca8ee23`	1227.45 ms	1247.34 ms	19.89 ms

App size

Revision	Plain	With Sentry	Diff
`544ce99`	22.31 KiB	777.60 KiB	755.29 KiB
`a618a92`	22.31 KiB	777.65 KiB	755.34 KiB
`ab1ebcb`	22.31 KiB	777.76 KiB	755.45 KiB
`5400890`	22.31 KiB	776.94 KiB	754.62 KiB
`63bb06c`	22.31 KiB	777.76 KiB	755.45 KiB
`ca8ee23`	22.31 KiB	775.60 KiB	753.29 KiB

philprime · 2025-01-27T16:43:36Z

Sources/Sentry/Profiling/SentryLaunchProfiling.m

@@ -153,9 +155,9 @@
    SENTRY_LOG_INFO(@"Starting app launch trace profile at %llu.", getAbsoluteTime());
    sentry_isTracingAppLaunch = YES;
    sentry_launchTracer =
-        [[SentryTracer alloc] initWithTransactionContext:sentry_context(tracesRate)
+        [[SentryTracer alloc] initWithTransactionContext:sentry_context(tracesRate, @1.0)


Is setting the random value to 1.0 the correct way? Should we set it to something < 1.0?

@armcknight do you have some input here?

Sources/Sentry/Public/SentryBaggage.h

philprime · 2025-01-27T16:47:45Z

Sources/Sentry/Public/SentrySpanContext.h

@@ -78,6 +88,19 @@ SENTRY_NO_INIT
 */
 - (instancetype)initWithOperation:(NSString *)operation sampled:(SentrySampleDecision)sampled;

+/**


Same as in #4751 (comment)

philprime · 2025-01-27T16:47:50Z

Sources/Sentry/Public/SentrySpanContext.h

@@ -91,6 +114,23 @@ SENTRY_NO_INIT
                      operation:(NSString *)operation
                        sampled:(SentrySampleDecision)sampled;

+/**


Ssame as in #4751 (comment)

Sources/Sentry/Public/SentrySpanContext.h

Sources/Sentry/Public/SentryTraceContext.h

Sources/Sentry/Public/SentryTransactionContext.h

philprime · 2025-01-28T09:45:54Z

Back to draft because I noticed missing test cases

brustolin

I do have one comment that I think would short this PR a lot.

Sources/Sentry/Public/SentrySpanProtocol.h

philipphofmann

I will give this a full review once we remove the span changes, as this will significantly reduce the scope of the PR.

philipphofmann · 2025-01-29T09:30:01Z

Makefile

+
+run-test-server-sync:
+	cd ./test-server && swift build
+	cd ./test-server && swift run
+
+.PHONY: run-test-server run-test-server-sync


l: Why did you have to change this? It would be great to put such changes in an extra PR to keep PRs smaller.

I don't want to have a daemon test server running on my machine in the background, which I can not control anymore without opening Activity Monitor or using kill, especially when it relates to the currently checked out commit.

I considered this a small enough change to be part of any PR, but I am fine with adding it in another PR.

It's OK to keep it.

Sources/Sentry/Public/SentryBaggage.h

Tests/SentryTests/Transaction/SentryBaggageTests.swift

Sources/Sentry/Public/SentrySpanProtocol.h

…sampling-seed-propagation

philipphofmann

LGTM, thanks for all the extra tests

philipphofmann · 2025-01-30T12:47:14Z

Sources/Sentry/SentrySampling.m

-    return [[SentrySamplerDecision alloc] initWithDecision:decision forSampleRate:rate];
+    return [[SentrySamplerDecision alloc] initWithDecision:decision
+                                             forSampleRate:rate
+                                            withSampleRand:[NSNumber numberWithDouble:random]];


l: A bit shorter.

Suggested change

withSampleRand:[NSNumber numberWithDouble:random]];

withSampleRand:@(random)];

philipphofmann · 2025-01-30T12:51:14Z

Sources/Sentry/Public/SentryTraceContext.h

+/**
+ * Random value used to determine if the trace is sampled.
+ */
+@property (nullable, nonatomic, readonly) NSString *sampleRand;


m: What a string? Ah, OK, I see that sampleRate is also a string. This is a bit confusing. Please add the reason for this as a comment. We do this in multiple places in the PR, so please update the PR everywhere.

philipphofmann · 2025-01-30T13:15:32Z

Tests/SentryTests/Protocol/SentryEnvelopeTests.swift

@@ -162,6 +162,24 @@ class SentryEnvelopeTests: XCTestCase {
        XCTAssertEqual(eventId, envelopeHeader.eventId)
        XCTAssertEqual(traceContext, envelopeHeader.traceContext)
    }
+
+    func testInitSentryEnvelopeHeader_withSampleRand_SetIdAndTraceState() {


m: What extra value does this test add? There aren't any changes in SentryEnvelopeHeader. We already have testInitSentryEnvelopeHeader_SetIdAndTraceState which validates that the init method of SentryEnvelopeHeader assigns the traceContext. Here, we only use a different initializer of the TraceContext. Am I missing something?

philipphofmann · 2025-01-30T13:20:23Z

Tests/SentryTests/SentryHubTests.swift

@@ -370,28 +370,68 @@ class SentryHubTests: XCTestCase {
        XCTAssertEqual(span.sampled, .no)
    }

+    @available(*, deprecated) // The test is marked as deprecated to silence the deprecation warning of the initializer


l: Maybe a bit cleaner.

Suggested change

@available(*, deprecated) // The test is marked as deprecated to silence the deprecation warning of the initializer

@available(*, deprecated, message: "The test is marked as deprecated to silence the deprecation warning of the initializer")

philprime commented Jan 24, 2025

View reviewed changes

Sources/Sentry/Profiling/SentryLaunchProfiling.m Outdated Show resolved Hide resolved

philprime force-pushed the philprime/issue-4687-sampling-seed-propagation branch from a0f0b4f to 07f9ad4 Compare January 27, 2025 13:41

add tons of tests and fixed to baggage

541c625

philprime force-pushed the philprime/issue-4687-sampling-seed-propagation branch from 07f9ad4 to 541c625 Compare January 27, 2025 16:40