Send envelopes that cannot be cached to disk #4294

denrase · 2024-08-21T14:23:37Z

📜 Description

Try to send envelopes that could not be saved to the disk cache.

💡 Motivation and Context

Relates to getsentry/team-mobile#52

💚 How did you test it?

Added unit test

📝 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

Check thread syncing.

github-actions · 2024-08-21T14:29:37Z

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

Generated by 🚫 dangerJS against 0b21b68

github-actions · 2024-08-21T15:09:21Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1239.53 ms	1254.43 ms	14.90 ms
Size	21.58 KiB	714.94 KiB	693.36 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`f4e0299`	1230.33 ms	1249.68 ms	19.35 ms
`2a894d5`	1202.07 ms	1227.66 ms	25.59 ms
`9f0d9e0`	1206.55 ms	1219.41 ms	12.86 ms
`8cf5bca`	1212.35 ms	1223.90 ms	11.54 ms
`3a31fc9`	1237.35 ms	1249.02 ms	11.67 ms
`984eb2d`	1220.62 ms	1235.24 ms	14.62 ms
`c471221`	1224.16 ms	1241.59 ms	17.43 ms
`156e771`	1228.06 ms	1242.64 ms	14.58 ms
`de033da`	1216.91 ms	1222.84 ms	5.92 ms
`f1c36e0`	1215.18 ms	1223.62 ms	8.43 ms

App size

Revision	Plain	With Sentry	Diff
`f4e0299`	20.76 KiB	427.54 KiB	406.78 KiB
`2a894d5`	21.58 KiB	414.57 KiB	392.99 KiB
`9f0d9e0`	21.58 KiB	424.28 KiB	402.70 KiB
`8cf5bca`	21.58 KiB	671.30 KiB	649.72 KiB
`3a31fc9`	20.76 KiB	414.45 KiB	393.69 KiB
`984eb2d`	20.76 KiB	425.77 KiB	405.01 KiB
`c471221`	22.85 KiB	413.89 KiB	391.04 KiB
`156e771`	20.76 KiB	419.70 KiB	398.94 KiB
`de033da`	21.58 KiB	418.15 KiB	396.57 KiB
`f1c36e0`	21.58 KiB	670.40 KiB	648.81 KiB

Previous results on branch: feat/send-envelope-on-cache-failure

Startup times

Revision	Plain	With Sentry	Diff
`cd07176`	1219.90 ms	1240.00 ms	20.10 ms
`70a2a20`	1250.63 ms	1266.24 ms	15.61 ms
`d8edaa7`	1231.65 ms	1257.69 ms	26.05 ms
`0d82804`	1227.37 ms	1252.37 ms	25.01 ms

App size

Revision	Plain	With Sentry	Diff
`cd07176`	21.58 KiB	706.96 KiB	685.38 KiB
`70a2a20`	21.58 KiB	709.33 KiB	687.75 KiB
`d8edaa7`	21.58 KiB	714.88 KiB	693.30 KiB
`0d82804`	21.58 KiB	714.79 KiB	693.21 KiB

philipphofmann

Thanks for doing this. A few suggestions.

Sources/Sentry/SentryHttpTransport.m

github-actions · 2024-09-16T13:41:09Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

…entry/sentry-cocoa into feat/send-envelope-on-cache-failure

github-actions · 2024-09-16T13:44:17Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2024-09-16T13:58:35Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2024-09-16T14:17:30Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

github-actions · 2024-09-17T10:05:43Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

codecov · 2024-09-17T10:12:46Z

Codecov Report

Attention: Patch coverage is 99.24242% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.688%. Comparing base (394ad33) to head (0b21b68).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
Sources/Sentry/SentryFileManager.m	0.000%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              main     #4294    +/-   ##
==========================================
  Coverage   91.687%   91.688%            
==========================================
  Files          621       623     +2     
  Lines        50441     50627   +186     
  Branches     18177     18325   +148     
==========================================
+ Hits         46248     46419   +171     
- Misses        4100      4115    +15     
  Partials        93        93

Files with missing lines	Coverage Δ
SentryTestUtils/TestClient.swift	`85.833% <100.000%> (+1.011%)`	⬆️
Sources/Sentry/SentryHttpTransport.m	`98.930% <100.000%> (+0.079%)`	⬆️
Sources/Sentry/SentryNSURLRequestBuilder.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentrySpotlightTransport.m	`97.142% <100.000%> (+11.848%)`	⬆️
...tryTests/Networking/SentryHttpTransportTests.swift	`99.708% <100.000%> (+0.005%)`	⬆️
...ts/Networking/SentryNSURLRequestBuilderTests.swift	`100.000% <100.000%> (ø)`
...sts/Networking/SentrySpotlightTransportTests.swift	`100.000% <100.000%> (ø)`
...s/SentryTests/Networking/TestNSURLRequestBuilder.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryFileManager.m	`93.979% <0.000%> (-0.772%)`	⬇️

... and 20 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 394ad33...0b21b68. Read the comment docs.

philipphofmann

Almost there 🥅

Sources/Sentry/SentryFileManager.m

Sources/Sentry/SentryHttpTransport.m

Tests/SentryTests/Networking/SentryHttpTransportTests.swift

# Conflicts: # Sources/Sentry/SentryFileManager.m # Sources/Sentry/include/SentryFileManager.h # Tests/SentryTests/Helper/SentryFileManagerTests.swift

github-actions · 2024-09-19T11:40:44Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

philipphofmann

Sorry that I didn't see this earlier. The SentryHttpTransport calls

sentry-cocoa/Sources/Sentry/SentryNSURLRequestBuilder.m

Lines 15 to 18 in 394ad33

    
           return [[SentryNSURLRequest alloc] 
        
               initEnvelopeRequestWithDsn:dsn 
        
                                  andData:[SentrySerialization dataWithEnvelope:envelope] 
        
                         didFailWithError:error];

The SentrySerialization dataWithEnvelope can return nil, but the SentryNSURLRequest expects data not to be nil; see

sentry-cocoa/Sources/Sentry/include/SentryNSURLRequest.h

Lines 13 to 24 in 394ad33

    
           - (_Nullable instancetype)initStoreRequestWithDsn:(SentryDsn *)dsn 
        
                                                     andData:(NSData *)data 
        
                                            didFailWithError:(NSError *_Nullable *_Nullable)error; 
        
           - (_Nullable instancetype)initEnvelopeRequestWithDsn:(SentryDsn *)dsn 
        
                                                        andData:(NSData *)data 
        
                                               didFailWithError:(NSError *_Nullable *_Nullable)error; 
        
           - (instancetype)initEnvelopeRequestWithURL:(NSURL *)url 
        
                                              andData:(NSData *)data 
        
                                           authHeader:(nullable NSString *)authHeader 
        
                                     didFailWithError:(NSError *_Nullable *_Nullable)error;

I think this never was a problem because the SentryHttpTransport never tried to send envelopes that it couldn't serialize. So we definitely need a test to validate what happens in that edge case. Maybe we need to change the logic of SentryNSURLRequestBuilder to not create an SentryNSURLRequest when it can't serialize the envelope.

…entry/sentry-cocoa into feat/send-envelope-on-cache-failure

github-actions · 2024-09-20T08:27:30Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

denrase · 2024-09-20T08:27:47Z

@philipphofmann Implemented handling of the case where envelope to data fails. Waiting for CI so we can do a final review/feedback round.

philipphofmann

We only miss tests for SentryNSURLRequestBuilder, then we can merge this. Thank you @denrase.

Sources/Sentry/SentryNSURLRequestBuilder.m

github-actions · 2024-09-20T09:44:01Z

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

Sources/Sentry/SentryFileManager.m

brustolin

Thanks @denrase, it looks good!

philipphofmann · 2024-09-20T12:13:11Z

Thank you @denrase and thanks @brustolin for approving the PR.

denrase added 4 commits August 21, 2024 15:54

add test

c351798

guard mock testfilemanager call

74bdb4e

try to send envelope that could not be saved

9597159

add changelog entry

a411e2e

denrase marked this pull request as ready for review August 21, 2024 14:29

denrase requested review from philipphofmann, brustolin and armcknight as code owners August 21, 2024 14:29

Format code

901db53

philipphofmann reviewed Sep 16, 2024

View reviewed changes

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

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

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

denrase added 2 commits September 16, 2024 15:08

Merge branch 'main' into feat/send-envelope-on-cache-failure

078656b

implement feedback

b4a0cd8

getsentry-bot and others added 3 commits September 16, 2024 13:41

Format code

6084bfc

mark as nullable

87ecb93

Merge branch 'feat/send-envelope-on-cache-failure' of github.com:gets…

6847c96

…entry/sentry-cocoa into feat/send-envelope-on-cache-failure

update helper

d9bd9bc

bind path

57ac294

fix test expectation

d68fb94

denrase enabled auto-merge (squash) September 17, 2024 10:07

denrase requested a review from philipphofmann September 17, 2024 10:07

philipphofmann reviewed Sep 17, 2024

View reviewed changes

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

Sources/Sentry/SentryHttpTransport.m Show resolved Hide resolved

Tests/SentryTests/Networking/SentryHttpTransportTests.swift Show resolved Hide resolved

Merge branch 'main' into feat/send-envelope-on-cache-failure

7d0a600

# Conflicts: # Sources/Sentry/SentryFileManager.m # Sources/Sentry/include/SentryFileManager.h # Tests/SentryTests/Helper/SentryFileManagerTests.swift

denrase added 3 commits September 19, 2024 13:20

fix test

5bfe11a

return early

b35202b

only check notStoredEnvelopes in synchronized block

7c3aba9

denrase requested a review from philipphofmann September 19, 2024 11:41

Format code

ef497bc

philipphofmann reviewed Sep 19, 2024

View reviewed changes

denrase added 2 commits September 20, 2024 10:26

handle case where serilizing envelope to data fails

5e26d8e

Merge branch 'feat/send-envelope-on-cache-failure' of github.com:gets…

4336fc7

…entry/sentry-cocoa into feat/send-envelope-on-cache-failure

Format code

cb1f409

denrase requested a review from philipphofmann September 20, 2024 08:34

philipphofmann reviewed Sep 20, 2024

View reviewed changes

Sources/Sentry/SentryNSURLRequestBuilder.m Show resolved Hide resolved

add SentryNSURLRequestBuilderTests

0b21b68

denrase requested a review from philipphofmann September 20, 2024 09:44

brustolin approved these changes Sep 20, 2024

View reviewed changes

denrase merged commit 26530fe into main Sep 20, 2024
65 checks passed

denrase deleted the feat/send-envelope-on-cache-failure branch September 20, 2024 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send envelopes that cannot be cached to disk #4294

Send envelopes that cannot be cached to disk #4294

denrase commented Aug 21, 2024

github-actions bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/send-envelope-on-cache-failure

Startup times

App size

philipphofmann left a comment

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 17, 2024

codecov bot commented Sep 17, 2024 •

edited

Loading

philipphofmann left a comment

github-actions bot commented Sep 19, 2024

philipphofmann left a comment

github-actions bot commented Sep 20, 2024

denrase commented Sep 20, 2024

philipphofmann left a comment

github-actions bot commented Sep 20, 2024

brustolin left a comment

philipphofmann commented Sep 20, 2024

	return [[SentryNSURLRequest alloc]
	initEnvelopeRequestWithDsn:dsn
	andData:[SentrySerialization dataWithEnvelope:envelope]
	didFailWithError:error];

	- (_Nullable instancetype)initStoreRequestWithDsn:(SentryDsn *)dsn
	andData:(NSData *)data
	didFailWithError:(NSError _Nullable _Nullable)error;

	- (_Nullable instancetype)initEnvelopeRequestWithDsn:(SentryDsn *)dsn
	andData:(NSData *)data
	didFailWithError:(NSError _Nullable _Nullable)error;

	- (instancetype)initEnvelopeRequestWithURL:(NSURL *)url
	andData:(NSData *)data
	authHeader:(nullable NSString *)authHeader
	didFailWithError:(NSError _Nullable _Nullable)error;

Send envelopes that cannot be cached to disk #4294

Send envelopes that cannot be cached to disk #4294

Conversation

denrase commented Aug 21, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Aug 21, 2024 • edited Loading

github-actions bot commented Aug 21, 2024 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/send-envelope-on-cache-failure

Startup times

App size

philipphofmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2024

🚨 Detected changes in high risk code 🚨

github-actions bot commented Sep 16, 2024

🚨 Detected changes in high risk code 🚨

github-actions bot commented Sep 16, 2024

🚨 Detected changes in high risk code 🚨

github-actions bot commented Sep 16, 2024

🚨 Detected changes in high risk code 🚨

github-actions bot commented Sep 17, 2024

🚨 Detected changes in high risk code 🚨

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

philipphofmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024

🚨 Detected changes in high risk code 🚨

philipphofmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024

🚨 Detected changes in high risk code 🚨

denrase commented Sep 20, 2024

philipphofmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024

🚨 Detected changes in high risk code 🚨

brustolin left a comment

Choose a reason for hiding this comment

philipphofmann commented Sep 20, 2024

github-actions bot commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading