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

[Dio] Replace failed http client adapter with interceptor #728

Merged

Conversation

ueman
Copy link
Collaborator

@ueman ueman commented Jan 30, 2022

📜 Description

This PR removes the failed http client adapter from the Dio integrations.
There's no need for this functionality, as Dio can be configured to throw on certain status codes. The users are probably using Dios way, which would cause having to reports for a bad status code. For reference, see ValidateStatus here. So instead we're adding an interceptor which can listen for exceptions.
We automatically listen for those exceptions and report them.

The reporting of DioErrors gets improved in #718

💡 Motivation and Context

This came out of the discussion over at #718 (comment)

💚 How did you test it?

New 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

Copy link
Collaborator

@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.

I agree with this decision.

@ueman ueman marked this pull request as draft January 31, 2022 08:22
@marandaneto
Copy link
Contributor

@ueman @brustolin

Dio can be configured to throw on certain status codes

That's fine, they are free to disable captureFailedRequests or to configure failedRequestStatusCodes properly.

The users are probably using Dios way

How is dio way? That still requires wrapping on try/catch blocks and calling Sentry.captureException(...) manually, or letting uncaught exceptions bubble up on the uncaught error handler, which would not be personalized with headers, request data, etc...

which would cause having to reports for a bad status code.

Only if they are using both features at the same time, and not configuring one or the other to avoid duplicate events.

We're pretty much removing a feature that captures errors automatically based on the user's configuration, without any other alternative, unless I'm missing something here.

@ueman
Copy link
Collaborator Author

ueman commented Jan 31, 2022

How is dio way? That still requires wrapping on try/catch blocks and calling Sentry.captureException(...) manually, or letting uncaught exceptions bubble up on the uncaught error handler, which would not be personalized with headers, request data, etc...

That's what #718 fixes.

@marandaneto
Copy link
Contributor

How is dio way? That still requires wrapping on try/catch blocks and calling Sentry.captureException(...) manually, or letting uncaught exceptions bubble up on the uncaught error handler, which would not be personalized with headers, request data, etc...

That's what #718 fixes.

Yep, got that, but event processors only run if there's a captured event, and with the removal of FailedRequestClientAdapter, users would need to do it manually.

@kuhnroyal
Copy link
Contributor

Wondering again if we should consider moving to an interceptor approach, which probably would solve most cases.

@brustolin
Copy link
Collaborator

How is dio way? That still requires wrapping on try/catch blocks and calling Sentry.captureException(...) manually, or letting uncaught exceptions bubble up on the uncaught error handler, which would not be personalized with headers, request data, etc...

That's what #718 fixes.

Yep, got that, but event processors only run if there's a captured event, and with the removal of FailedRequestClientAdapter, users would need to do it manually.

Didn't thought that. Capturing crash manually would be bad.

I like @kuhnroyal suggestion. Using interceptors we could capture the error but let the request continue its flow.

…-request-adapter

# Conflicts:
#	dio/lib/src/failed_request_client_adapter.dart
@ueman ueman changed the title Ref: Remove failed http client adapter from Dio [Dio] Replace failed http client adapter with interceptor Feb 7, 2022
@ueman ueman marked this pull request as ready for review February 8, 2022 17:38
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2022

Codecov Report

Merging #728 (deca26b) into main (c2893b8) will increase coverage by 1.56%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #728      +/-   ##
==========================================
+ Coverage   90.56%   92.13%   +1.56%     
==========================================
  Files         104       17      -87     
  Lines        3359      483    -2876     
==========================================
- Hits         3042      445    -2597     
+ Misses        317       38     -279     
Impacted Files Coverage Δ
dio/lib/src/sentry_dio_client_adapter.dart 77.77% <ø> (-4.05%) ⬇️
dio/lib/src/sentry_dio_extension.dart 80.00% <0.00%> (-20.00%) ⬇️
dio/lib/src/failed_request_interceptor.dart 100.00% <100.00%> (ø)
dart/lib/src/transport/rate_limit_category.dart
dart/lib/src/protocol/sdk_version.dart
dart/lib/src/transport/encode.dart
dart/lib/src/protocol/span_status.dart
dart/lib/src/protocol/sentry_event.dart
dart/lib/src/protocol/mechanism.dart
dart/lib/src/protocol/sentry_app.dart
... and 80 more

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 c2893b8...deca26b. Read the comment docs.

Copy link
Collaborator

@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.

LGTM

ueman and others added 2 commits February 9, 2022 23:18
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
@ueman
Copy link
Collaborator Author

ueman commented Feb 11, 2022

Can this be merged, so I can integrate it to the other Dio PR?

@marandaneto marandaneto merged commit a3d7007 into getsentry:main Feb 11, 2022
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.

None yet

5 participants