Add `sent_at` to envelope header #2859

denrase · 2023-04-04T15:52:18Z

📜 Description

Add sent_at to envelope header
Set to current date before sending out envelope in transport adapter

💡 Motivation and Context

Relates to getsentry/sentry-dart#1337
Documentation https://develop.sentry.dev/sdk/envelopes/

💚 How did you test it?

Added tests

📝 Checklist

You have to check all boxes before merging:

I reviewed the submitted code.
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.
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

Test if this works correctly wit hybrid (dart) sdks.

github-actions · 2023-04-04T15:53:48Z

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

Generated by 🚫 dangerJS against f3c8571

github-actions · 2023-04-04T16:06:24Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1218.57 ms	1227.84 ms	9.27 ms
Size	20.76 KiB	434.72 KiB	413.96 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`4d68229`	1233.50 ms	1262.92 ms	29.42 ms
`8f397a7`	1230.10 ms	1253.88 ms	23.77 ms
`371db89`	1226.40 ms	1251.54 ms	25.14 ms
`fd6a31c`	1204.73 ms	1222.34 ms	17.61 ms
`d60f70a`	1219.63 ms	1228.54 ms	8.91 ms
`630ddf4`	1216.50 ms	1235.94 ms	19.44 ms
`25737cb`	1235.02 ms	1250.06 ms	15.04 ms
`06548c0`	1226.71 ms	1252.37 ms	25.66 ms
`c021422`	1199.15 ms	1222.20 ms	23.05 ms
`b2f82fa`	1237.78 ms	1256.02 ms	18.24 ms

App size

Revision	Plain	With Sentry	Diff
`4d68229`	20.76 KiB	432.34 KiB	411.58 KiB
`8f397a7`	20.76 KiB	420.55 KiB	399.79 KiB
`371db89`	20.76 KiB	427.31 KiB	406.55 KiB
`fd6a31c`	20.76 KiB	436.50 KiB	415.74 KiB
`d60f70a`	20.76 KiB	430.97 KiB	410.21 KiB
`630ddf4`	20.76 KiB	432.37 KiB	411.61 KiB
`25737cb`	20.76 KiB	436.29 KiB	415.53 KiB
`06548c0`	20.76 KiB	427.36 KiB	406.59 KiB
`c021422`	20.76 KiB	435.64 KiB	414.88 KiB
`b2f82fa`	20.76 KiB	419.62 KiB	398.86 KiB

Previous results on branch: feat/envelope-header-sent-at

Startup times

Revision	Plain	With Sentry	Diff
`30236ed`	1239.67 ms	1252.84 ms	13.17 ms
`3031b91`	1235.29 ms	1246.32 ms	11.03 ms
`6789a10`	1209.59 ms	1232.30 ms	22.71 ms
`7044a83`	1242.82 ms	1264.12 ms	21.30 ms
`f50cd0f`	1218.71 ms	1225.84 ms	7.12 ms
`1fe5466`	1196.30 ms	1232.38 ms	36.08 ms
`5d48ce8`	1220.37 ms	1240.67 ms	20.30 ms
`7078353`	1217.05 ms	1243.16 ms	26.12 ms
`8ef64f7`	1236.76 ms	1263.14 ms	26.38 ms

App size

Revision	Plain	With Sentry	Diff
`30236ed`	20.76 KiB	432.67 KiB	411.91 KiB
`3031b91`	20.76 KiB	434.72 KiB	413.96 KiB
`6789a10`	20.76 KiB	432.41 KiB	411.65 KiB
`7044a83`	20.76 KiB	433.29 KiB	412.53 KiB
`f50cd0f`	20.76 KiB	432.59 KiB	411.83 KiB
`1fe5466`	20.76 KiB	431.64 KiB	410.88 KiB
`5d48ce8`	20.76 KiB	435.04 KiB	414.28 KiB
`7078353`	20.76 KiB	432.67 KiB	411.91 KiB
`8ef64f7`	20.76 KiB	431.64 KiB	410.88 KiB

codecov · 2023-04-04T16:16:43Z

Codecov Report

Merging #2859 (f3c8571) into main (0dfdaaa) will increase coverage by 0.025%.
The diff coverage is 100.000%.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #2859       +/-   ##
=============================================
+ Coverage   88.802%   88.827%   +0.025%     
=============================================
  Files          492       492               
  Lines        52878     52900       +22     
  Branches     18955     18966       +11     
=============================================
+ Hits         46957     46990       +33     
+ Misses        4954      4946        -8     
+ Partials       967       964        -3

Impacted Files	Coverage Δ
Sources/Sentry/SentryNSURLRequest.m	`76.190% <ø> (-1.083%)`	⬇️
Sources/Sentry/SentryHttpTransport.m	`97.747% <100.000%> (+0.010%)`	⬆️
Sources/Sentry/SentrySerialization.m	`90.035% <100.000%> (+0.180%)`	⬆️
.../SentryTests/Helper/SentrySerializationTests.swift	`97.269% <100.000%> (+0.028%)`	⬆️
Tests/SentryTests/Networking/SentryDsnTests.m	`100.000% <100.000%> (ø)`
...tryTests/Networking/SentryHttpTransportTests.swift	`97.566% <100.000%> (+0.026%)`	⬆️
...Tests/Networking/SentryTransportAdapterTests.swift	`97.297% <100.000%> (ø)`

... and 15 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 0dfdaaa...f3c8571. Read the comment docs.

Sources/Sentry/SentrySerialization.m

brustolin

LGTM

philipphofmann

Thanks for doing this, @denrase. We have to change two important pieces. See one h comment and we also have to remove the sentry_timestamp

sentry-cocoa/Sources/Sentry/SentryNSURLRequest.m

Lines 106 to 108 in 2cf1b84

    
           [string 
        
               appendFormat:@"%@,", 
        
               newHeaderPart(@"sentry_timestamp", @((NSInteger)[[NSDate date] timeIntervalSince1970]))];

See docs:

Also note that the sent_at header replaces the sentry_timestamp key previously set in authorization headers, which has now been fully deprecated. You should only send sent_at, and not sentry_timestamp.

philipphofmann · 2023-04-07T09:07:12Z

Sources/Sentry/SentryTransportAdapter.m

@@ -94,6 +95,7 @@ - (void)sendUserFeedback:(SentryUserFeedback *)userFeedback

 - (void)sendEnvelope:(SentryEnvelope *)envelope
 {
+    envelope.header.sentAt = SentryCurrentDate.date;


h: This sets the sentAt timestamp to when we are caching to disk, which we must not do according to the docs:

For example, SDKs that implement caching features should avoid writing the sent_at header when caching to disk. Only write it when actually sending the event to Sentry.

Instead, we have to set sentAt directly before we send it:

sentry-cocoa/Sources/Sentry/SentryHttpTransport.m

Lines 281 to 283 in 2cf1b84

NSURLRequest *request = [self.requestBuilder createEnvelopeRequest:rateLimitedEnvelope

dsn:self.options.parsedDsn

didFailWithError:&requestError];

You are right, sry about that. Moved this to SentryHttpTransport.m

Sources/Sentry/include/HybridPublic/SentryEnvelope.h

# Conflicts: # Sources/Sentry/SentrySerialization.m

…entry-cocoa into feat/envelope-header-sent-at

philipphofmann

Almost LGTM, thanks @denrase.

Tests/SentryTests/Networking/SentryTransportAdapterTests.swift

Sources/Sentry/SentryTransportAdapter.m

philipphofmann · 2023-04-18T13:11:54Z

Tests/SentryTests/Networking/SentryHttpTransportTests.swift

@@ -52,14 +52,17 @@ class SentryHttpTransportTests: XCTestCase {
            attachmentEnvelopeItem = SentryEnvelopeItem(attachment: TestData.dataAttachment, maxAttachmentSize: 5 * 1_024 * 1_024)!

            eventEnvelope = SentryEnvelope(id: event.eventId, items: [SentryEnvelopeItem(event: event), attachmentEnvelopeItem])
+            eventEnvelope.header.sentAt = CurrentDate.date()


m: Thanks for adapting the tests. It would be great if the tests fail, when setting the sentAt before creating the request, for example, in SentryHttpTransport.sendEnvelope. If it's too complicated to achieve, please don't do it. Adding a comment as explained in #2859 (comment) could also be sufficient.

Please check if the comment i added is sufficient.

Sources/Sentry/SentryHttpTransport.m

philipphofmann

LGTM, but the Changelog needs some fixes.

CHANGELOG.md

philipphofmann

Again LGTM, @denrase. What's stopping you from merging this PR?

krystofwoldrich · 2023-05-04T20:13:22Z

Hello everyone,
the unit tests are not happy, by the error message it looks like it could be related to the changes.

Could we get this merged soon? There are RN customers waiting for this as it will fix issues with clock drift in transactions.

philipphofmann · 2023-05-05T09:59:18Z

@krystofwoldrich, @denrase is not working full-time for Sentry. I can take it and finish the PR if he doesn't get to it by Tuesday.

krystofwoldrich · 2023-05-10T09:31:14Z

@philipphofmann Okay, thanks for the info, that would be awesome. What is missing, I thought it was done?

krystofwoldrich · 2023-05-11T16:18:01Z

@philipphofmann @denrase Thank you.

denrase added 3 commits April 4, 2023 17:11

add sentAt to envelope header

48542b4

set headet sentAt in transport adapter

6f6a2d2

add changelog entry

3fd8afe

denrase marked this pull request as ready for review April 4, 2023 15:53

denrase requested review from philipphofmann, brustolin and armcknight as code owners April 4, 2023 15:53

Format code

473ef69

brustolin reviewed Apr 5, 2023

View reviewed changes

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

brustolin approved these changes Apr 5, 2023

View reviewed changes

philipphofmann reviewed Apr 7, 2023

View reviewed changes

denrase and others added 7 commits April 17, 2023 10:38

Merge branch 'main' into feat/envelope-header-sent-at

ce0cff3

# Conflicts: # Sources/Sentry/SentrySerialization.m

Format code

4297bf0

document field

7a1815e

Merge branch 'feat/envelope-header-sent-at' of github.com:getsentry/s…

0c48e97

…entry-cocoa into feat/envelope-header-sent-at

assign sentAt in http transport instead of adapter

7cc60d7

remove sentry_timestamp from header

acffda9

Format code

a38a1e8

denrase requested a review from philipphofmann April 17, 2023 09:12

denrase mentioned this pull request Apr 17, 2023

Add sent_at to envelope header item getsentry/sentry-java#2638

Merged

7 tasks

philipphofmann reviewed Apr 18, 2023

View reviewed changes

denrase and others added 2 commits April 18, 2023 16:11

implement review feedback

54f6569

Format code

016dae5

philipphofmann approved these changes Apr 18, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

denrase added 2 commits April 24, 2023 16:33

Merge branch 'main' into feat/envelope-header-sent-at

21313e1

fix changelog

b9819f7

denrase requested a review from marandaneto April 25, 2023 08:18

Merge branch 'main' into feat/envelope-header-sent-at

ea64152

marandaneto mentioned this pull request Apr 25, 2023

Support SentryEnvelopeHeader#sent_at getsentry/team-mobile#32

Closed

philipphofmann approved these changes Apr 26, 2023

View reviewed changes

philipphofmann added 3 commits May 11, 2023 14:18

Merge branch 'main' into feat/envelope-header-sent-at

068741e

fix changelog

5b02c6e

undo change in SentryTransportAdapterTests

f3c8571

philipphofmann merged commit b483671 into main May 11, 2023

philipphofmann deleted the feat/envelope-header-sent-at branch May 11, 2023 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `sent_at` to envelope header #2859

Add `sent_at` to envelope header #2859

denrase commented Apr 4, 2023

github-actions bot commented Apr 4, 2023 •

edited

Loading

github-actions bot commented Apr 4, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/envelope-header-sent-at

Startup times

App size

codecov bot commented Apr 4, 2023 •

edited

Loading

brustolin left a comment

philipphofmann left a comment

philipphofmann Apr 7, 2023

denrase Apr 17, 2023

philipphofmann left a comment

philipphofmann Apr 18, 2023 •

edited

Loading

denrase Apr 18, 2023

philipphofmann left a comment

philipphofmann left a comment

krystofwoldrich commented May 4, 2023

philipphofmann commented May 5, 2023

krystofwoldrich commented May 10, 2023 •

edited

Loading

krystofwoldrich commented May 11, 2023

	[string
	appendFormat:@"%@,",
	newHeaderPart(@"sentry_timestamp", @((NSInteger)[[NSDate date] timeIntervalSince1970]))];

	NSURLRequest *request = [self.requestBuilder createEnvelopeRequest:rateLimitedEnvelope
	dsn:self.options.parsedDsn
	didFailWithError:&requestError];

Add sent_at to envelope header #2859

Add sent_at to envelope header #2859

Conversation

denrase commented Apr 4, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Apr 4, 2023 • edited Loading

github-actions bot commented Apr 4, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/envelope-header-sent-at

Startup times

App size

codecov bot commented Apr 4, 2023 • edited Loading

Codecov Report

brustolin left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Apr 7, 2023

Choose a reason for hiding this comment

denrase Apr 17, 2023

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

denrase Apr 18, 2023

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

krystofwoldrich commented May 4, 2023

philipphofmann commented May 5, 2023

krystofwoldrich commented May 10, 2023 • edited Loading

krystofwoldrich commented May 11, 2023

Add `sent_at` to envelope header #2859

Add `sent_at` to envelope header #2859

github-actions bot commented Apr 4, 2023 •

edited

Loading

github-actions bot commented Apr 4, 2023 •

edited

Loading

codecov bot commented Apr 4, 2023 •

edited

Loading

philipphofmann Apr 18, 2023 •

edited

Loading

krystofwoldrich commented May 10, 2023 •

edited

Loading