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

Capture event for HTTP requests resulted in server error #2287

Merged
merged 43 commits into from
Oct 20, 2022

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Oct 10, 2022

📜 Description

HTTP Client errors such as bad response code are captured as error events and reported to Sentry. The error event will contain the request and response data such as url, status_code, and so on.

User facing docs getsentry/sentry-docs#5651

💡 Motivation and Context

getsentry/team-mobile#38

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Oct 10, 2022

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

Generated by 🚫 dangerJS against b79a165

@marandaneto marandaneto changed the title HTTP client errors are automatically captured Capture event for HTTP requests resulted in server error Oct 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 10, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 280.16 ms 348.41 ms 68.25 ms
Size 1.73 MiB 2.32 MiB 607.78 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
221a5df 350.61 ms 391.58 ms 40.97 ms
4ca1d7b 328.46 ms 368.22 ms 39.76 ms
54cebc8 331.12 ms 385.14 ms 54.02 ms
54cebc8 300.86 ms 341.43 ms 40.57 ms
2c5f172 310.20 ms 357.16 ms 46.96 ms
2c5f172 289.18 ms 307.56 ms 18.38 ms
7300956 324.20 ms 353.79 ms 29.58 ms
c5ccd8a 329.98 ms 365.52 ms 35.54 ms
d4087ee 278.00 ms 313.86 ms 35.86 ms
4ca1d7b 331.33 ms 335.78 ms 4.45 ms

App size

Revision Plain With Sentry Diff
221a5df 1.73 MiB 2.29 MiB 581.39 KiB
4ca1d7b 1.73 MiB 2.29 MiB 579.88 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
7300956 1.73 MiB 2.29 MiB 578.69 KiB
c5ccd8a 1.74 MiB 2.33 MiB 607.44 KiB
d4087ee 1.73 MiB 2.29 MiB 579.50 KiB
4ca1d7b 1.73 MiB 2.29 MiB 579.88 KiB

Previous results on branch: feat/httpclienterrors

Startup times

Revision Plain With Sentry Diff
43e9f47 303.30 ms 326.18 ms 22.88 ms
04bbe69 344.33 ms 371.84 ms 27.51 ms
42cf10a 333.70 ms 401.86 ms 68.16 ms
25164ef 328.60 ms 341.86 ms 13.26 ms
ab59cc7 348.10 ms 375.40 ms 27.30 ms
f0da475 361.31 ms 383.85 ms 22.54 ms
ae25959 347.51 ms 372.73 ms 25.22 ms
4364835 393.85 ms 454.24 ms 60.39 ms
0dbf473 317.16 ms 338.70 ms 21.54 ms
09897d4 317.26 ms 369.36 ms 52.10 ms

App size

Revision Plain With Sentry Diff
43e9f47 1.73 MiB 2.29 MiB 580.53 KiB
04bbe69 1.73 MiB 2.29 MiB 580.53 KiB
42cf10a 1.73 MiB 2.29 MiB 580.26 KiB
25164ef 1.73 MiB 2.29 MiB 581.87 KiB
ab59cc7 1.73 MiB 2.29 MiB 580.51 KiB
f0da475 1.73 MiB 2.32 MiB 607.78 KiB
ae25959 1.73 MiB 2.29 MiB 580.53 KiB
4364835 1.73 MiB 2.29 MiB 580.53 KiB
0dbf473 1.73 MiB 2.29 MiB 581.87 KiB
09897d4 1.73 MiB 2.29 MiB 580.26 KiB

hub.captureEvent(event, hint)
}

private fun containsStatusCode(statusCode: Int): Boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be called something like shouldStatusCodeCauseSentryEvent or similar?

Also should it be extracted into some util so it can be reused from other integrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, isInRange already can be reused.
In case the language has a built-in type for Range instead of HttpStatusCodeRange, they will need to implement this anyway.
Java does not have Range obj as part of the language.

sentry/src/main/java/io/sentry/HttpStatusCodeRange.java Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor Author

@romtsn @adinauer ready to be reviewed, its a Draft because it cant be merged before Relay and Snuba changes.

@romtsn
Copy link
Member

romtsn commented Oct 18, 2022

@marandaneto will take a look later today or tomorrow 👍

@marandaneto marandaneto marked this pull request as ready for review October 19, 2022 15:03
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, besides some minor things 👍

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.

8 participants