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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add request instead of response data in DioEventProcessor #933

Merged
merged 6 commits into from Jul 20, 2022

Conversation

kuhnroyal
Copy link
Contributor

馃摐 Description

Add request instead of response data to SentryRequest in DioEventProcessor

馃挕 Motivation and Context

Response data may contain PII and should not be logged by the framework.
The SentryRequest.data is meant to contain request data.
See discussion in #624 about PII.

馃挌 How did you test it?

Ran and adapted the 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

CC @ueman

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022

Codecov Report

Merging #933 (27563fe) into main (287b3ad) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #933   +/-   ##
=======================================
  Coverage   89.81%   89.81%           
=======================================
  Files         104      104           
  Lines        3211     3211           
=======================================
  Hits         2884     2884           
  Misses        327      327           
Impacted Files Coverage 螖
dio/lib/src/dio_event_processor.dart 93.47% <100.00%> (酶)

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 287b3ad...27563fe. Read the comment docs.

@ueman
Copy link
Collaborator

ueman commented Jul 5, 2022

This looks good to me.
I still whish, that there's not only a SentryRequest but also a SentryResponse.

On mobile it makes a lot of sense to record the response since, at least in my experience, that's a lot of the times also a source of errors (getsentry/develop#401).

Regarding PII, why does it matter? I mean response and request can contain PII?

@kuhnroyal
Copy link
Contributor Author

This looks good to me. I still whish, that there's not only a SentryRequest but also a SentryResponse.

On mobile it makes a lot of sense to record the response since, at least in my experience, that's a lot of the times also a source of errors.

Regarding PII, why does it matter? I mean response and request can contain PII?

I absolutely agree but it should be consistent in the SDK.
For me usually requests contain PII, responses contain weird validation errors which I am currently trying to manually log into SentryRequest.other map. But the order of DioEventProcessor and my custom processor can not be controlled. I need to add mine after DioEventProcessor somehow ...

@marandaneto
Copy link
Contributor

Makes sense, since the request data is guarded by maxRequestBodySize, wondering if this was an oversight or if we've decided to add the response in there for some reason, @ueman ?
@ueman the problem with the response is payload size, events can get easily dropped if we don't introduce a flag to limit that, such as #624 but that's a different issue.

@ueman
Copy link
Collaborator

ueman commented Jul 6, 2022

I think adding the response instead of the request was an oversight. The SentryHttpClient also adds the request and not the response.

@ueman ueman mentioned this pull request Jul 7, 2022
5 tasks
@marandaneto marandaneto enabled auto-merge (squash) July 20, 2022 12:24
@marandaneto marandaneto merged commit c32f612 into getsentry:main Jul 20, 2022
@kuhnroyal kuhnroyal deleted the bugfix/sentry-dio-request branch March 15, 2023 16:26
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

4 participants