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.

Sign up for GitHub

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

Jump to bottom

Add Client Reports feature #1556

Merged
merged 63 commits into from
May 3, 2022
Merged

Add Client Reports feature #1556

merged 63 commits into from
May 3, 2022

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Mar 26, 2022 •
edited
Loading

Implement Client Reports. Resolves #1455

Ready for review!

@mattjohnsonpint
Merge branch 'main' into add-client-reports
e192ded
@mattjohnsonpint
Update CHANGELOG.md
af2992c
@mattjohnsonpint
Update test
9f97777 
CI is much slower than expected, so can't rely on timing part of the test.
The test still proves that the expected values are achieved.
@mattjohnsonpint
Improve test
4cf2dcd 
Parallel.ForEach is a better choice than async/await for this
bruno-garcia
bruno-garcia reviewed Mar 28, 2022
View reviewed changes
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

This is looking great. One concern about value tuples on netfx

src/Sentry/Internal/BackgroundWorker.cs Outdated Show resolved Hide resolved
@mattjohnsonpint mattjohnsonpint mentioned this pull request Mar 29, 2022
Closed
philipphofmann
philipphofmann reviewed Mar 30, 2022
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This already goes in the right direction. I don't see any issues from a concept point of view right now. Thanks for doing this 🎉. I added a few comments.

src/Sentry/SentryOptions.cs Show resolved Hide resolved
test/Sentry.Tests/Internals/ThreadsafeCounterDictionaryTests.cs Show resolved Hide resolved
test/Sentry.Tests/Internals/ThreadsafeCounterDictionaryTests.cs Show resolved Hide resolved
src/Sentry/Internal/BackgroundWorker.cs Outdated Show resolved Hide resolved
@mattjohnsonpint
Copy link
Contributor Author

Note, I'll need to refactor this to use the transport afterall. Doing it in the background worker will get in the way of the changes I'm getting ready to submit to support extensibility (for Unity WebGL). I have a plan...

philipphofmann
philipphofmann reviewed Apr 6, 2022
View reviewed changes
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.

Looking better every day 👍

@getsentry getsentry deleted a comment from codecov-commenter Apr 8, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2022 •
edited
Loading

Codecov Report

Merging #1556 (4cf2dcd) into main (7604242) will decrease coverage by 0.13%.
The diff coverage is 73.46%.

❗ Current head 4cf2dcd differs from pull request most recent head 8cac99e. Consider uploading reports for the commit 8cac99e to get more accurate results

@@            Coverage Diff             @@
##             main    #1556      +/-   ##
==========================================
- Coverage   84.34%   84.21%   -0.14%     
==========================================
  Files         219      224       +5     
  Lines        7423     7520      +97     
  Branches     1417     1427      +10     
==========================================
+ Hits         6261     6333      +72     
- Misses        735      759      +24     
- Partials      427      428       +1
Impacted Files Coverage Δ
src/Sentry/Internal/Extensions/JsonExtensions.cs 78.21% <0.00%> (-1.79%) ⬇️
src/Sentry/Internal/ClientReport.cs 31.57% <31.57%> (ø)
src/Sentry/Envelopes/EnvelopeItem.cs 90.20% <61.53%> (-2.87%) ⬇️
src/Sentry/Internal/Enumeration.cs 83.33% <83.33%> (ø)
src/Sentry/Internal/ThreadsafeCounterDictionary.cs 85.71% <85.71%> (ø)
src/Sentry/Envelopes/Envelope.cs 96.87% <100.00%> (+0.10%) ⬆️
src/Sentry/Internal/BackgroundWorker.cs 94.07% <100.00%> (+0.60%) ⬆️
src/Sentry/Internal/DataCategory.cs 100.00% <100.00%> (ø)
src/Sentry/Internal/DiscardReason.cs 100.00% <100.00%> (ø)
src/Sentry/SentryOptions.cs 90.36% <100.00%> (+0.05%) ⬆️
... and 1 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 80324c2...8cac99e. Read the comment docs.

@mattjohnsonpint mattjohnsonpint added Impact: Large Feature New feature or request labels Apr 28, 2022
@mattjohnsonpint mattjohnsonpint enabled auto-merge (squash) May 3, 2022 01:02
@mattjohnsonpint mattjohnsonpint merged commit a443a18 into main May 3, 2022
@mattjohnsonpint mattjohnsonpint deleted the add-client-reports branch May 3, 2022 02:00
@mattjohnsonpint mattjohnsonpint mentioned this pull request Oct 18, 2022
Merged
@jamescrosswell jamescrosswell mentioned this pull request Apr 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marandaneto marandaneto marandaneto left review comments

@sl0thentr0py sl0thentr0py sl0thentr0py left review comments

@SimonCropp SimonCropp SimonCropp approved these changes

@bruno-garcia bruno-garcia Awaiting requested review from bruno-garcia

@philipphofmann philipphofmann Awaiting requested review from philipphofmann

@adinauer adinauer Awaiting requested review from adinauer

Assignees
No one assigned
Labels
Feature New feature or request
Projects
Mobile & Cross Platform SDK
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add 'Client Reports' to the .NET SDK
8 participants
@mattjohnsonpint @codecov-commenter @bruno-garcia @SimonCropp @philipphofmann @adinauer @marandaneto @sl0thentr0py