Skip to content
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

Merged
merged 64 commits into from
Nov 3, 2022
Merged

HTTP Client errors #2308

merged 64 commits into from
Nov 3, 2022

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Oct 20, 2022

📜 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?

Screenshot 2022-10-25 at 10 08 46

Screenshot 2022-10-25 at 10 09 04

Screenshot 2022-10-25 at 10 09 17

Screenshot 2022-10-25 at 10 09 39

📝 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

Sentry has to render the response headers nicely as the Request headers.
User facing docs

@github-actions
Copy link

github-actions bot commented Oct 20, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1213.64 ms 1231.66 ms 18.02 ms
Size 20.51 KiB 365.16 KiB 344.65 KiB

Baseline results on branch: master

Startup times

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

Copy link
Contributor

@brustolin brustolin left a 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...

Sources/Sentry/SentryNetworkTracker.m Outdated Show resolved Hide resolved
NS_SWIFT_NAME(Request)
@interface SentryRequest : NSObject <SentrySerializable, NSCopying>

// TODO: data, env
Copy link
Contributor Author

@marandaneto marandaneto Oct 21, 2022

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.

Copy link
Contributor Author

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

@marandaneto
Copy link
Contributor Author

@brustolin @philipphofmann this is ready for a 1st review feature-wise and SDK guidelines, then I will start with tests, etc.
Since I am not an objC dev, please double-check everything that makes sense :)

marandaneto and others added 4 commits October 31, 2022 08:44
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@marandaneto
Copy link
Contributor Author

User facing docs getsentry/sentry-docs#5702

Copy link
Contributor

@brustolin brustolin left a 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.

@marandaneto
Copy link
Contributor Author

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

@marandaneto
Copy link
Contributor Author

@brustolin @philipphofmann an approval would be awesome since there are no pending comments.

Copy link
Contributor

@brustolin brustolin left a 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!!

@marandaneto marandaneto merged commit 59afa00 into master Nov 3, 2022
@marandaneto marandaneto deleted the chore/http-client-errors branch November 3, 2022 12:19
kevinrenskers added a commit that referenced this pull request Nov 7, 2022
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants