feat: Support tracePropagationTargets by kevinrenskers · Pull Request #2217 · getsentry/sentry-cocoa

kevinrenskers · 2022-09-23T13:56:25Z

📜 Description

We now have a new SentryOption tracePropagationTargets which determines if our two headers are added to outgoing requests.

💡 Motivation and Context

Closes #2148.

💚 How did you test it?

Unit test & running the app with a debug proxy to view actual requests and their headers.

📝 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 · 2022-09-23T13:57:39Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

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

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Support tracePropagationTargets ([#2217](https://github.com/getsentry/sentry-cocoa/pull/2217))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 46e7e67

kevinrenskers · 2022-09-23T14:02:38Z

Right now SentrySDK.options.tracePropagationTargets can contain either instances of NSString, or instances of NSRegularExpression.

In the case of NSString, the request's URL.host must match the value precisely.
In the case of NSRegularExpression, the entire URL will be used for matching.

See addHeadersForRequestWithURL in the changed files.

This matches the docs and example on https://develop.sentry.dev/sdk/performance/#tracepropagationtargets, and makes the most sense to me, and matches what I've seen in other systems (like CORS whitelists in Django).

@brustolin on the other hand thinks that the entire url should always be used, and we should use contains rather than isEqualToString. I wanted to get early feedback on this. To me it doesn't really make sense, because if you add sentry.io to SentrySDK.options.tracePropagationTargets, what you mean is that all request from that domain are allowed. But sentry.io.evil-hacker.com would also match by using contains! If you really want to have that kind of freedom you can always use NSRegularExpression.

@philipphofmann @bruno-garcia what do you think?

github-actions · 2022-09-23T14:10:35Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1256.00 ms	1268.35 ms	12.35 ms
Size	20.51 KiB	332.90 KiB	312.40 KiB

Baseline results on branch: master

Startup times

Revision	Plain	With Sentry	Diff
`864c39a`	1191.14 ms	1233.38 ms	42.24 ms
`4a66f00`	1224.73 ms	1241.14 ms	16.41 ms
`5ecf9f6`	1230.76 ms	1232.86 ms	2.10 ms
`202334c`	1265.41 ms	1277.34 ms	11.93 ms
`791123d`	1217.52 ms	1253.08 ms	35.56 ms
`b172a8b`	1257.68 ms	1272.38 ms	14.70 ms
`7138b7d`	1243.40 ms	1252.08 ms	8.68 ms
`172c95a`	1220.08 ms	1251.74 ms	31.66 ms
`7c867f1`	1206.16 ms	1234.88 ms	28.71 ms
`9fc2dd0`	1246.14 ms	1275.00 ms	28.86 ms

App size

Revision	Plain	With Sentry	Diff
`864c39a`	20.51 KiB	335.57 KiB	315.06 KiB
`4a66f00`	20.51 KiB	331.79 KiB	311.28 KiB
`5ecf9f6`	20.51 KiB	332.66 KiB	312.15 KiB
`202334c`	20.51 KiB	331.79 KiB	311.28 KiB
`791123d`	20.51 KiB	331.81 KiB	311.30 KiB
`b172a8b`	20.51 KiB	331.79 KiB	311.28 KiB
`7138b7d`	20.51 KiB	331.79 KiB	311.28 KiB
`172c95a`	20.51 KiB	335.57 KiB	315.06 KiB
`7c867f1`	20.51 KiB	332.59 KiB	312.09 KiB
`9fc2dd0`	20.50 KiB	331.79 KiB	311.28 KiB

Previous results on branch: feat/2148-tracePropagationTargets

Startup times

Revision	Plain	With Sentry	Diff
`bbcc973`	1261.96 ms	1272.67 ms	10.72 ms
`f7b4806`	1215.08 ms	1238.28 ms	23.20 ms
`5d57218`	1214.27 ms	1235.16 ms	20.89 ms
`8f68482`	1249.96 ms	1262.08 ms	12.12 ms
`ca11fbf`	1227.39 ms	1246.36 ms	18.97 ms
`279fa8c`	1216.81 ms	1238.68 ms	21.87 ms
`c2aeeea`	1244.12 ms	1249.53 ms	5.41 ms
`ff930b0`	1268.13 ms	1291.84 ms	23.71 ms
`0e35c66`	1227.96 ms	1256.63 ms	28.67 ms
`f6518a9`	1202.39 ms	1240.90 ms	38.51 ms

App size

Revision	Plain	With Sentry	Diff
`bbcc973`	20.51 KiB	331.90 KiB	311.39 KiB
`f7b4806`	20.51 KiB	331.92 KiB	311.42 KiB
`5d57218`	20.51 KiB	331.96 KiB	311.45 KiB
`8f68482`	20.51 KiB	332.26 KiB	311.75 KiB
`ca11fbf`	20.51 KiB	331.92 KiB	311.42 KiB
`279fa8c`	20.51 KiB	332.03 KiB	311.52 KiB
`c2aeeea`	20.51 KiB	331.94 KiB	311.43 KiB
`ff930b0`	20.51 KiB	332.61 KiB	312.10 KiB
`0e35c66`	20.51 KiB	331.96 KiB	311.46 KiB
`f6518a9`	20.51 KiB	332.91 KiB	312.40 KiB

* 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

kevinrenskers · 2022-09-27T12:02:56Z

I'm getting test failures in testGetRequest_SpanCreatedAndBaggageHeaderAdded, it seems that it uses a cached version of the server maybe? It all succeeds locally when I run the server with my changes. But if we're going to remove those tests that hit the server, I don't really have to look into any further. Let me know!

brustolin · 2022-09-27T14:27:42Z

We are missing 'tracePropagationTargets' initialisation during validateOptions:didFailWithError: function in SentryOptions

kevinrenskers · 2022-09-27T14:29:49Z

We are missing 'tracePropagationTargets' initialisation during validateOptions:didFailWithError: function in SentryOptions

Yeah I actually wanted to ask about that - what is the use of that initWithDict method anyways? Who's using this, and why?

brustolin · 2022-09-27T14:31:50Z

Yeah I actually wanted to ask about that - what is the use of that initWithDict method anyways? Who's using this, and why?

I believe this is legacy stuff.
@philipphofmann do you have a better answer?

kevinrenskers · 2022-09-27T14:32:49Z

Perhaps something we can clean up with v8?

philipphofmann · 2022-09-27T15:40:04Z

Yeah I actually wanted to ask about that - what is the use of that initWithDict method anyways? Who's using this, and why?

I believe this is legacy stuff. @philipphofmann do you have a better answer?

Legacy and hybrid SDKs. So no, I don't think we can rid of that in V8.

philipphofmann · 2022-09-27T16:38:40Z

@kevinrenskers, it's supposed to be as pointed out in the develop docs. We have an internal DACI, which I'm going to send you somewhere in private.

Co-authored-by: Dhiogo Brustolin <[email protected]>

philipphofmann

Thanks for taking care of this, @kevinrenskers.

philipphofmann · 2022-09-28T08:04:09Z

                          infoDict[@"CFBundleShortVersionString"], infoDict[@"CFBundleVersion"]];
        }
+
+        NSRegularExpression *everythingAllowedRegex =


h: We are missing tests for this new functionality.

This is tested using testAddHeadersForRequestWithURL. What are you missing?

I miss

That the default is an array with a regex .*.

Setting an array with different values than NSString or NSRegularExpression. We don't need to validate the logging functionality, but we should ensure we are not crashing.

A test for the custom setter

A test for setting tracePropagationTargets with the startWithDict.

That the default is an array with a regex .*.

That is basically what is tested using testAddHeadersForRequestWithURL. Not that it literally is a regex .*, but that it has the correct behavior, which seems more useful to me than testing if it contains this specific regex?

I'll look into the other points.

philipphofmann · 2022-09-28T08:38:33Z

+        XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["sentry-trace"] ?? "", "")
+    }
+
+    func testAddHeadersForRequestWithURL() {


l: Excellent test case, but I think we should split it up into several ones.

Just wondering why you think so? Splitting it up doesn't really do anything, and I like how all these related things are now in one test function.

An approach to organizing your tests is to test one functionality. When the test fails its name should tell you what's wrong. Up to you.

brustolin

LGTM

Just wait for @philipphofmann replies in his comments before merge this.

philipphofmann

Only missing tests and the wrong link to the docs.

philipphofmann · 2022-09-29T09:18:02Z

                          infoDict[@"CFBundleShortVersionString"], infoDict[@"CFBundleVersion"]];
        }
+
+        NSRegularExpression *everythingAllowedRegex =


I miss

That the default is an array with a regex .*.

Setting an array with different values than NSString or NSRegularExpression. We don't need to validate the logging functionality, but we should ensure we are not crashing.

A test for the custom setter

A test for setting tracePropagationTargets with the startWithDict.

philipphofmann · 2022-09-29T09:21:30Z

+        XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["sentry-trace"] ?? "", "")
+    }
+
+    func testAddHeadersForRequestWithURL() {


An approach to organizing your tests is to test one functionality. When the test fails its name should tell you what's wrong. Up to you.

philipphofmann

Thanks, 🙏

philipphofmann · 2022-09-29T10:03:36Z

    XCTAssertNil(options.beforeBreadcrumb);
 }

+- (void)testTracePropagationTargets


m: Let's please also validate the default value. We only need to add a line somewhere here

sentry-cocoa/Tests/SentryTests/SentryOptionsTest.m

Line 519 in 5ecf9f6

XCTAssertEqual(YES, options.enableAutoBreadcrumbTracking);

* master: meta: Changelog link start-up crashes (#2238) ci: Point to latest GH hash for VLC tests (#2237) ci: Fix typo in comments for tests.yml (#2241) release: 7.27.0 feat: Support tracePropagationTargets (#2217)

kevinrenskers added 3 commits September 23, 2022 14:10

WIP

0fb53c0

WIP2

93f97bb

WIP3

ad70bb7

kevinrenskers added 3 commits September 23, 2022 16:04

Changelog

d67967a

Uhm wtf did Nova do there

04d8b7f

Better log

dda4371

kevinrenskers added 7 commits September 27, 2022 11:26

Fix tests

0890340

Add tests for testAddHeadersForRequestWithURL

3d59a1e

Remove

e394918

Remove

f6bee40

Add two tests that I think can replace the entire test server?

5f7468b

Move changelog

e093569

kevinrenskers commented Sep 27, 2022

View reviewed changes

Comment thread Tests/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerTests.swift

kevinrenskers commented Sep 27, 2022

View reviewed changes

Comment thread test-server/Sources/App/routes.swift

testNoHeadersForWrongUrl

0ead2c4

kevinrenskers marked this pull request as ready for review September 27, 2022 12:46

kevinrenskers requested review from armcknight, brustolin, indragiek and philipphofmann as code owners September 27, 2022 12:46

brustolin reviewed Sep 27, 2022

View reviewed changes

Comment thread Sources/Sentry/SentryNetworkTracker.m Outdated

Init tracePropagationTargets from options dict

6ee437c

brustolin reviewed Sep 27, 2022

View reviewed changes

Comment thread Tests/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerIntegrationTests.swift Outdated

Remove self.canHeaderBeAdded check

07d0c06

kevinrenskers and others added 2 commits September 28, 2022 09:09

Update Sources/Sentry/SentryNetworkTracker.m

d4b481d

Co-authored-by: Dhiogo Brustolin <[email protected]>

Update docs, update tests

bf65c8e

philipphofmann reviewed Sep 28, 2022

View reviewed changes

kevinrenskers mentioned this pull request Sep 28, 2022

feat(apple): Add apple to trace-propagation-targets getsentry/sentry-docs#5574

Merged

kevinrenskers added 6 commits September 28, 2022 11:07

Address feedback part 1

6bc73a5

Feedback part 2

76d2455

Restore removeSentryKeysFromBaggage functionality

06d46ba

Reverse

64468bc

No need for the separate private header file

5eacf37

Add 2 more tests

e52e223

brustolin approved these changes Sep 28, 2022

View reviewed changes

Merge branch 'master' into feat/2148-tracePropagationTargets

7dd402a

philipphofmann reviewed Sep 29, 2022

View reviewed changes

Comment thread Sources/Sentry/Public/SentryOptions.h Outdated

Use correct doc url

6ceba32

philipphofmann reviewed Sep 29, 2022

View reviewed changes

More tests

fabba3f

philipphofmann approved these changes Sep 29, 2022

View reviewed changes

Add test for default pattern

46e7e67

kevinrenskers merged commit b6ef18d into master Sep 29, 2022

kevinrenskers deleted the feat/2148-tracePropagationTargets branch September 29, 2022 10:23

philipphofmann mentioned this pull request Feb 28, 2024

Add support for tracing origins #1236

Closed

Uh oh!

Uh oh!

Conversation

kevinrenskers commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions Bot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions and example for changelog

Uh oh!

kevinrenskers commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: master

Startup times

App size

Previous results on branch: feat/2148-tracePropagationTargets

Startup times

App size

Uh oh!

Uh oh!

Uh oh!

kevinrenskers commented Sep 27, 2022

Uh oh!

Uh oh!

brustolin commented Sep 27, 2022

Uh oh!

kevinrenskers commented Sep 27, 2022

Uh oh!

brustolin commented Sep 27, 2022

Uh oh!

kevinrenskers commented Sep 27, 2022

Uh oh!

Uh oh!

philipphofmann commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipphofmann commented Sep 27, 2022

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brustolin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kevinrenskers commented Sep 23, 2022 •

edited

Loading

github-actions Bot commented Sep 23, 2022 •

edited

Loading

kevinrenskers commented Sep 23, 2022 •

edited

Loading

github-actions Bot commented Sep 23, 2022 •

edited

Loading

philipphofmann commented Sep 27, 2022 •

edited

Loading