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

Feat: Client Report Envelope Item #810

Merged
merged 9 commits into from
Apr 13, 2022

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Mar 28, 2022

#skip-changelog

📜 Description

Create envelope item from client report model.

💡 Motivation and Context

The item will be added to an envelope so we can send it to the backend.

💚 How did you test it?

Unit tests.

📝 Checklist

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

@denrase denrase marked this pull request as ready for review March 28, 2022 14:39
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.

Regarding the code, it looks good to me.
I'm not fully aware of the specifics on "client report".

@marandaneto can take a look at that.

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

Merging #810 (1c38943) into feat/client-report-recorder (4576880) will increase coverage by 0.23%.
The diff coverage is n/a.

@@                       Coverage Diff                       @@
##           feat/client-report-recorder     #810      +/-   ##
===============================================================
+ Coverage                        89.70%   89.94%   +0.23%     
===============================================================
  Files                              118        9     -109     
  Lines                             3700      169    -3531     
===============================================================
- Hits                              3319      152    -3167     
+ Misses                             381       17     -364     
Impacted Files Coverage Δ
dart/lib/src/client_reports/client_report.dart
dart/lib/src/client_reports/discarded_event.dart
dart/lib/src/sentry_envelope_item.dart
dart/lib/src/http_client/tracing_client.dart
dart/lib/src/integration.dart
dart/lib/src/protocol/max_request_body_size.dart
dart/lib/src/protocol/debug_meta.dart
dart/lib/src/throwable_mechanism.dart
...lib/src/environment/_io_environment_variables.dart
dart/lib/src/protocol/dsn.dart
... and 99 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 4576880...1c38943. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM from a high-level concept point of view. Can't add some feedback to the dart code. Thanks for doing this 👏

@mattjohnsonpint
Copy link
Contributor

Conceptually seems fine. Though I see some build check failures?

@marandaneto
Copy link
Contributor

Conceptually seems fine. Though I see some build check failures?

A few files were renamed but not on test files, likely a merging issue @denrase ?

@denrase
Copy link
Collaborator Author

denrase commented Apr 11, 2022

@marandaneto Sry about that, let me check/fix

@denrase denrase merged commit 9c40c3b into feat/client-report-recorder Apr 13, 2022
@denrase denrase deleted the feat/client-report-envelope-item branch April 13, 2022 07:28
@denrase denrase mentioned this pull request Apr 13, 2022
13 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.

None yet

7 participants