-
-
Notifications
You must be signed in to change notification settings - Fork 375
HTTP Client errors #2308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTP Client errors #2308
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0e2e593 | 1234.78 ms | 1266.10 ms | 31.32 ms |
| 0fdf0b2 | 1245.88 ms | 1247.69 ms | 1.82 ms |
| 791123d | 1256.10 ms | 1266.68 ms | 10.58 ms |
| 1453a8a | 1224.98 ms | 1250.31 ms | 25.33 ms |
| db5f62a | 1234.47 ms | 1257.80 ms | 23.33 ms |
| 202334c | 1265.41 ms | 1277.34 ms | 11.93 ms |
| b869536 | 1250.37 ms | 1274.84 ms | 24.47 ms |
| 8168e86 | 1210.14 ms | 1233.16 ms | 23.02 ms |
| 604586a | 1216.06 ms | 1249.10 ms | 33.04 ms |
| 1453a8a | 1204.22 ms | 1230.84 ms | 26.61 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0e2e593 | 20.50 KiB | 333.88 KiB | 313.38 KiB |
| 0fdf0b2 | 20.51 KiB | 332.90 KiB | 312.39 KiB |
| 791123d | 20.51 KiB | 331.81 KiB | 311.30 KiB |
| 1453a8a | 20.50 KiB | 361.89 KiB | 341.39 KiB |
| db5f62a | 20.51 KiB | 333.16 KiB | 312.65 KiB |
| 202334c | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| b869536 | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| 8168e86 | 20.50 KiB | 338.99 KiB | 318.49 KiB |
| 604586a | 20.51 KiB | 333.15 KiB | 312.65 KiB |
| 1453a8a | 20.50 KiB | 361.89 KiB | 341.39 KiB |
Previous results on branch: chore/http-client-errors
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4f016f2 | 1197.98 ms | 1227.16 ms | 29.18 ms |
| 68d4fc1 | 1244.96 ms | 1268.82 ms | 23.86 ms |
| 0e5b17b | 1209.06 ms | 1230.80 ms | 21.74 ms |
| 1d0f66e | 1199.73 ms | 1225.34 ms | 25.61 ms |
| c9f8158 | 1263.24 ms | 1270.18 ms | 6.94 ms |
| 788a132 | 1249.22 ms | 1254.41 ms | 5.19 ms |
| d34a8cf | 1245.30 ms | 1276.49 ms | 31.19 ms |
| c031d22 | 1258.33 ms | 1261.78 ms | 3.45 ms |
| f0235d8 | 1206.04 ms | 1230.42 ms | 24.38 ms |
| c02af8b | 1236.67 ms | 1258.16 ms | 21.49 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4f016f2 | 20.51 KiB | 346.14 KiB | 325.64 KiB |
| 68d4fc1 | 20.51 KiB | 346.30 KiB | 325.80 KiB |
| 0e5b17b | 20.51 KiB | 346.30 KiB | 325.80 KiB |
| 1d0f66e | 20.51 KiB | 341.73 KiB | 321.23 KiB |
| c9f8158 | 20.51 KiB | 341.62 KiB | 321.12 KiB |
| 788a132 | 20.51 KiB | 365.16 KiB | 344.65 KiB |
| d34a8cf | 20.51 KiB | 340.57 KiB | 320.06 KiB |
| c031d22 | 20.51 KiB | 348.37 KiB | 327.86 KiB |
| f0235d8 | 20.51 KiB | 341.67 KiB | 321.17 KiB |
| c02af8b | 20.51 KiB | 341.34 KiB | 320.84 KiB |
brustolin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass, it looks good.
I have only one item to discuss...
| NS_SWIFT_NAME(Request) | ||
| @interface SentryRequest : NSObject <SentrySerializable, NSCopying> | ||
|
|
||
| // TODO: data, env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't address it in this PR, but it's part of the request protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something for getsentry/team-mobile#56
…y-cocoa into chore/http-client-errors
|
@brustolin @philipphofmann this is ready for a 1st review feature-wise and SDK guidelines, then I will start with tests, etc. |
...ngMovies/TrendingMovies.xcodeproj/xcshareddata/xcschemes/ProfileDataGeneratorUITest.xcscheme
Show resolved
Hide resolved
Co-authored-by: Philipp Hofmann <[email protected]>
…y-cocoa into chore/http-client-errors
|
User facing docs getsentry/sentry-docs#5702 |
…y-cocoa into chore/http-client-errors
brustolin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good.
I think we're only missing an integration test that actually do a request to our test server.
You can take a look at testGetRequest_SpanCreatedAndBaggageHeaderAdded test, and add a new entry point at test-server/Sources/App/routes.swift. If you need, I can help with the test server.
done |
|
@brustolin @philipphofmann an approval would be awesome since there are no pending comments. |
brustolin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations @marandaneto. This is a nice work!!
* master: test: Delete empty OOMLogicTests (#2361) fix: profiling transaction timestamps (#2359) fix: profiling transaction thread IDs (#2358) test: Use flush for macOS-SPM sample (#2360) release: 7.30.0 fix: SentryCrash writing nan for invalid number (#2348) HTTP Client errors (#2308) ci: Call make for analyze (#2353) fix: CoreData tracking entity name retrieval (#2329) fix: sampled profile format backend validation errors (#2350) build(deps): bump github/codeql-action from 2.1.29 to 2.1.30 (#2351) build(deps): bump github/codeql-action from 2.1.28 to 2.1.29 (#2344) fix: Fixed flaky testTimezoneChangeNotificationBreadcrumb (#2335) # Conflicts: # Sentry.xcodeproj/project.pbxproj
📜 Description
Captures HTTP Client errors automatically if within a status code range and targets.
💡 Motivation and Context
getsentry/team-mobile#38
💚 How did you test it?
📝 Checklist
🔮 Next steps
Sentry has to render the response headers nicely as the Request headers.
User facing docs