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

[v9] Remove max response body size #2709

Merged
merged 14 commits into from
Feb 18, 2025
Merged

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Feb 12, 2025

📜 Description

  • remove max response body size
  • Check content-size to attach response body size

💡 Motivation and Context

Closes #2637
Closes #2638

💚 How did you test it?

Tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.50%. Comparing base (79ccefb) to head (5f32c85).
Report is 251 commits behind head on v9.

Files with missing lines Patch % Lines
dart/lib/src/hint.dart 0.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##               v9    #2709       +/-   ##
===========================================
+ Coverage   67.94%   88.50%   +20.55%     
===========================================
  Files          15      262      +247     
  Lines         443     8758     +8315     
===========================================
+ Hits          301     7751     +7450     
- Misses        142     1007      +865     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denrase denrase marked this pull request as ready for review February 12, 2025 15:42
@buenaflor
Copy link
Contributor

@denrase nice! could you also prepare a PR deprecating these for the main branch pls

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

looks good, only got one question

dio/lib/src/dio_event_processor.dart Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -11,6 +11,9 @@
- Remove `options.autoAppStart` and `setAppStartEnd` ([#2680](https://github.com/getsentry/sentry-dart/pull/2680))
- Add hint for transactions ([#2675](https://github.com/getsentry/sentry-dart/pull/2675))
- `BeforeSendTransactionCallback` now has a `Hint` parameter
- Remove max response body size ([#2709](https://github.com/getsentry/sentry-dart/pull/2709))
- Responses are now only attached if size is below ~15mb
Copy link
Contributor

Choose a reason for hiding this comment

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

and also pls mention that this is currently only applicable for the dio integration, not our http integration

Copy link
Contributor

@buenaflor buenaflor Feb 13, 2025

Choose a reason for hiding this comment

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

our http integration doesn't support capturing the response data generally, we could add that feature later at some point

@denrase
Copy link
Collaborator Author

denrase commented Feb 13, 2025

@buenaflor The contexts still have response. Should we deprecate it?

@buenaflor
Copy link
Contributor

@denrase I believe so, could you check if other sdks have this in the contexts

@buenaflor
Copy link
Contributor

https://develop.sentry.dev/sdk/data-model/event-payloads/contexts/#response-context we can leave the response, we just have to be clear that we will not add the response body/data automatically, the user has to do that themselves via the hints

@denrase denrase requested a review from buenaflor February 17, 2025 12:26
CHANGELOG.md Outdated Show resolved Hide resolved
@denrase denrase requested a review from buenaflor February 17, 2025 13:36
Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

nice lgtm

@denrase denrase merged commit 9fa06c0 into v9 Feb 18, 2025
150 of 153 checks passed
@denrase denrase deleted the v9-remove-max-response-body-size branch February 18, 2025 11:01
@vaind vaind mentioned this pull request Feb 18, 2025
6 tasks
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.

2 participants