feat: Replace tracestate header with baggage #1867

brustolin · 2022-06-07T14:20:25Z

📜 Description

Replaced the tracestate header in outbound http requests for a baggage header

💡 Motivation and Context

This is a required feature to enable dynamic sampling

💚 How did you test it?

Unit tests

📝 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-06-07T14:21:15Z

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

Generated by 🚫 dangerJS against 1b79195

into feat/baggage

codecov-commenter · 2022-06-07T14:34:22Z

Codecov Report

Merging #1867 (76a7166) into master (472d146) will increase coverage by 0.21%.
The diff coverage is 93.06%.

❗ Current head 76a7166 differs from pull request most recent head 1b79195. Consider uploading reports for the commit 1b79195 to get more accurate results

@@            Coverage Diff             @@
##           master    #1867      +/-   ##
==========================================
+ Coverage   91.91%   92.13%   +0.21%     
==========================================
  Files         200      202       +2     
  Lines        9450     9497      +47     
==========================================
+ Hits         8686     8750      +64     
+ Misses        764      747      -17

Impacted Files	Coverage Δ
Sources/Sentry/SentryHttpTransport.m	`100.00% <ø> (ø)`
Sources/Sentry/include/SentryBaggage.h	`0.00% <0.00%> (ø)`
Sources/Sentry/include/SentryNetworkTracker.h	`100.00% <ø> (ø)`
Sources/Sentry/include/SentryTracer.h	`100.00% <ø> (ø)`
Sources/Sentry/Public/SentryEnvelope.h	`100.00% <100.00%> (ø)`
Sources/Sentry/SentryBaggage.m	`100.00% <100.00%> (ø)`
Sources/Sentry/SentryClient.m	`99.15% <100.00%> (+<0.01%)`	⬆️
Sources/Sentry/SentryEnvelope.m	`93.12% <100.00%> (ø)`
Sources/Sentry/SentryNetworkTracker.m	`98.87% <100.00%> (ø)`
Sources/Sentry/SentrySerialization.m	`87.50% <100.00%> (+1.09%)`	⬆️
... and 7 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 1706d81...1b79195. Read the comment docs.

brustolin · 2022-06-14T07:11:22Z

Hey @adinauer and @AbhiPrasad, it would be nice if you could take a look at this PR.

adinauer

Only looked at baggage specific things, so best get another review for cocoa things :-)

Sources/Sentry/SentryBaggage.m

Sources/Sentry/SentrySerialization.m

Tests/SentryTests/Helper/SentrySerializationTests.swift

Tests/SentryTests/Transaction/SentryBaggageTests.swift

adinauer

One more tiny thing regarding the character limit, otherwise LGTM.

Sources/Sentry/SentrySerialization.m

philipphofmann

Almost LGTM. Good job 👏

Sources/Sentry/SentryBaggage.m

Sources/Sentry/SentrySerialization.m

philipphofmann · 2022-06-15T12:30:23Z

Sources/Sentry/SentrySerialization.m

+            [[value description] stringByAddingPercentEncodingWithAllowedCharacters:allowedSet];
+
+        NSString *item = [NSString stringWithFormat:@"%@=%@", keyDescription, valueDescription];
+        if (item.length + currentSize <= 8192) {


l: We could break if we exceeded the max size

That's why we have this check, to avoid exceeding the max size.

I think @philipphofmann is suggesting we do not look at any more items once we reach max size. It could be possible though that there's smaller items that still fit even though the current loop item doesn't. So I think it's better this way.

Ow, I see. @AbhiPrasad can correct me if Im wrong, but I remember reading somewhere from him that we should continue adding values that fits. So, break is not the best solution.

If the resulting baggage-string would be greater than 8192 bytes, some list-members MAY be dropped until the resulting baggage-string is 8192 characters or less.

and

If a platform does not propagate all list-members, it is left to the implementer to decide which list-members to propagate.

See https://www.w3.org/TR/baggage/#limits

philipphofmann · 2022-06-15T12:30:59Z

Tests/SentryTests/Helper/SentrySerializationTests.swift

@@ -245,6 +245,23 @@ class SentrySerializationTests: XCTestCase {
        XCTAssertNil(actual)
    }

+    func testDictionaryToBaggageEncoded() {


m: Can we add a test for checking if the Baggage exceeds the max size please?

We already have it.

sentry-cocoa/Tests/SentryTests/Helper/SentrySerializationTests.swift

Lines 258 to 262 in 76a7166

let largeValue = String(repeating: "a", count: 8_188)

XCTAssertEqual(SentrySerialization.baggageEncodedDictionary(["key": largeValue]), "key=\(largeValue)")

XCTAssertEqual(SentrySerialization.baggageEncodedDictionary(["AKey": "something", "BKey": largeValue]), "AKey=something")

XCTAssertEqual(SentrySerialization.baggageEncodedDictionary(["AKey": "something", "BKey": largeValue, "CKey": "Other Value"]), "AKey=something,CKey=Other%20Value")

philipphofmann

LGTM 🚀

Sources/Sentry/include/SentrySerialization.h

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

Add baggage

498af75

brustolin and others added 3 commits June 7, 2022 16:21

Update CHANGELOG.md

bdb5759

Format code

3c9c24a

Merge branch 'feat/baggage' of https://github.com/getsentry/sentry-cocoa

261764c

into feat/baggage

philipphofmann mentioned this pull request Jun 10, 2022

Replace trace state HTTP headers with baggage getsentry/team-mobile#15

Closed

2 tasks

brustolin and others added 7 commits June 13, 2022 11:41

Renaming SentryTraceState to SentryTraceContext

e8a91fb

BaggageTest

4c62086

some tests

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

da37325

Merge branch 'master' into feat/baggage

e48726a

Update SentryNetworkTrackerTests.swift

31dcfd8

tests

03bb57a

Format code

6b4811d

brustolin marked this pull request as ready for review June 13, 2022 14:28

brustolin requested a review from philipphofmann as a code owner June 13, 2022 14:28

Update routes.swift

2fadf55

adinauer requested changes Jun 14, 2022

View reviewed changes

brustolin and others added 2 commits June 14, 2022 10:15

restrict baggage size and change separator

06e36b2

Format code

374dea3

brustolin requested a review from adinauer June 14, 2022 08:16

Merge branch 'master' into feat/baggage

76a7166

adinauer approved these changes Jun 14, 2022

View reviewed changes

Sources/Sentry/SentrySerialization.m Outdated Show resolved Hide resolved

AbhiPrasad approved these changes Jun 14, 2022

View reviewed changes

philipphofmann reviewed Jun 15, 2022

View reviewed changes

philipphofmann assigned brustolin Jun 17, 2022

brustolin added 3 commits June 20, 2022 07:58

max size constant

8925f15

toHeader as non nullable

74bcc41

Merge branch 'master' into feat/baggage

e538f39

brustolin and others added 2 commits June 20, 2022 08:07

fixing merge

791966c

Format code

9b6a25c

philipphofmann approved these changes Jun 20, 2022

View reviewed changes

Sources/Sentry/include/SentrySerialization.h Outdated Show resolved Hide resolved

brustolin and others added 2 commits June 20, 2022 10:58

Update Sources/Sentry/include/SentrySerialization.h

b217d73

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

Update SentrySerialization.m

1b79195

brustolin merged commit fa49e72 into master Jun 20, 2022

brustolin deleted the feat/baggage branch June 20, 2022 10:53

	let largeValue = String(repeating: "a", count: 8_188)

	XCTAssertEqual(SentrySerialization.baggageEncodedDictionary(["key": largeValue]), "key=\(largeValue)")
	XCTAssertEqual(SentrySerialization.baggageEncodedDictionary(["AKey": "something", "BKey": largeValue]), "AKey=something")
	XCTAssertEqual(SentrySerialization.baggageEncodedDictionary(["AKey": "something", "BKey": largeValue, "CKey": "Other Value"]), "AKey=something,CKey=Other%20Value")

feat: Replace tracestate header with baggage #1867

feat: Replace tracestate header with baggage #1867

Conversation

brustolin commented Jun 7, 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 Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brustolin commented Jun 14, 2022

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adinauer 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!

Uh oh!

Uh oh!

philipphofmann Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

brustolin Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

adinauer Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

brustolin Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

philipphofmann Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

brustolin Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brustolin commented Jun 7, 2022 •

edited

Loading

github-actions bot commented Jun 7, 2022 •

edited

Loading

codecov-commenter commented Jun 7, 2022 •

edited

Loading

brustolin Jun 20, 2022 •

edited

Loading