Feat: Failed requests HTTP client

[ueman]

📜 Description

This is a HTTP client wich automatically add failed requests to an event.

💡 Motivation and Context

Closes #292

💚 How did you test it?

It still needs tests.

📝 Checklist

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

🔮 Next steps

[codecov-commenter]

Codecov Report

Merging #473 (5d382cf) into main (6873600) will decrease coverage by 0.01%.
The diff coverage is 89.58%.

@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
- Coverage   91.24%   91.23%   -0.02%     
==========================================
  Files          63       66       +3     
  Lines        2091     2168      +77     
==========================================
+ Hits         1908     1978      +70     
- Misses        183      190       +7     

Impacted Files	Coverage Δ
dart/lib/src/http_client/sentry_http_client.dart	76.19% <75.00%> (-13.81%)	⬇️
dart/lib/src/http_client/breadcrumb_client.dart	88.23% <88.23%> (ø)
...art/lib/src/http_client/failed_request_client.dart	94.00% <94.00%> (ø)
dart/lib/src/protocol/breadcrumb.dart	100.00% <100.00%> (ø)
dart/lib/src/protocol/max_request_body_size.dart	100.00% <100.00%> (ø)
dart/lib/src/protocol/sentry_request.dart	98.03% <0.00%> (+1.96%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6873600...5d382cf. Read the comment docs.

[ueman]

@bruno-garcia
What do you think of splitting each HTTP client related functionality in its own client?
So recording breadcrumbs goes into the BreadcrumbClient, capturing "bad" requests (requests which throw or have a 400/500 status code for example) goes into the FailedRequestClient.
Additionally there's the SentryHttpClient which then uses both internally and it's the user facing class.

In the future there would then be a third client for the distributed tracing thingy.

[marandaneto]

@bruno-garcia
What do you think of splitting each HTTP client related functionality in its own client?
So recording breadcrumbs goes into the BreadcrumbClient, capturing "bad" requests (requests which throw or have a 400/500 status code for example) goes into the FailedRequestClient.
Additionally there's the SentryHttpClient which then uses both internally and it's the user facing class.

In the future there would then be a third client for the distributed tracing thingy.

@bruno-garcia opinions?

[bruno-garcia]

Sorry I'll take a look in a bit

[bruno-garcia]

The decorator approach LGTM as its indeed easier to test and compose features as needed.

A couple of things to keep in mind is the usability though, the user should have a simple way into the API without having to worry about the decoration composition. This will also to allow us to add new decorators later without affecting the user.

So if we can keep: SentryHttpClient() that bundles all decorators by default, it's a great approach.
The order also is important, for example add the breadcrumb after capturing the event (so the event doesn't include double crumbs, the artificial one created by Sentry plus the one from the integration).

[ueman]

So if we can keep: SentryHttpClient() that bundles all decorators by default, it's a great approach.

That's the plan.

The order also is important, for example add the breadcrumb after capturing the event (so the event doesn't include double crumbs, the artificial one created by Sentry plus the one from the integration).

That's a good point. Just changed it accordingly.

[bruno-garcia]

The integration creating events directly will open a new precedence for other SDKs. We don't do this yet from anywhere AFAIK. At least not in the Mobile team's SDKs. Including .NET and Java bases.

Would love to hear your learnings once this is live and some customer feedback came in

[marandaneto]

LGTM

[marandaneto]

@bruno-garcia would you like to do a final review before we merge and release it as beta? as this PR drives the PoC

[ueman]

There's the risk of creating events too large for ingestion.

What is the largest allowed size? I'll add another test for that.

Finally, PII is another concern so docs should clearly state that there's this risk and point to the scrubbing data parts of the docs. I'm not too familiar with the automatic scrubbing we already have, worth pointing to that too since there's some level of default protection against PII getting in already.

Gonna do that, too

[marandaneto]

There's the risk of creating events too large for ingestion.

What is the largest allowed size? I'll add another test for that.

as we are using MaxRequestBodySize, I guess we don't need to do anything more, if the user is using always its their own risk to get events dropped, see a similar issue getsentry/sentry-java#910
checking the size of the event before sending has runtime cost so that's why we don't do it.
just in case of know-how, https://develop.sentry.dev/sdk/envelopes/#size-limits
worth noting that this is for envelopes, which is gonna be into the next major as well

[marandaneto]

LGTM