Improve SentryHttpFailedRequestHandler issue grouping

[ericsampson]

Resolves #3582

With the current SentryHttpFailedRequestHandler, the captured exception stack trace is always the same two frames; this leads to all events getting lumped into a single Issue, even though the application code path (and remote API) are totally different. As you can imagine, this is less than ideal :)

This PR addresses this issue by capturing a full stack trace on .NET 6+ using ExceptionDispatchInfo.SetCurrentStackTrace.

Tests have been expanded and improved. The test of the new functionality (HandleResponse_StackTraceIncludesCallerContext) was verified to fail using the existing code, and pass using the new code.

Very similar changes could likely be made to the SentryGraphQLHttpFailedRequestHandler.cs to give the same improvement, but I didn't want to bundle too many changes together into one PR.

I'd love to get this into 5.16.3 or 6.0.0 if possible, depending on the release timing :)

Thanks!

[jamescrosswell]

This looks awesome @ericsampson - we're a bit tight on time but will definitely do our best to get this reviewed and included in version 6... it's a slight change in behaviour so might be good to do this in the major release.

[ericsampson]

Totally understood @jamescrosswell! Thanks. Would it be worthwhile to start just by approving all the GH workflows to run to make sure that they don't surface any issues.

1. Do you think we could get it into a 6.0.0 beta release? That way I can test it For Real in our prod API without having to vendor the repo into our codebase. If that sounds like a good plan, LMK if I should change this PR base branch and to what.

1b) Should I add a release note line?

2. Given the "slight change in behavior", do you think that I should add a way that customers can opt-out of this change in case that it turns out affecting them in a way that they don't like? I could potentially add it to SentryExperimentalOptions.

Best,
Eric

[Flash0ver]

Thank you so much for this improvement! Well done!
Sorry for my late review.
Also, great job on the testing part.

[ericsampson]

@Flash0ver thanks for the review! Comments make sense, I'll work through them.

[ericsampson]

@Flash0ver
Given the "slight change in behavior", do you think that I should add a way that customers can opt-out of this change in case that it turns out affecting them in a way that they don't like? I could potentially add it to SentryExperimentalOptions.

[jamescrosswell]

I suggested preserving one comment that got lost when inlining what used to be an extension method... but otherwise looks good to me.

The failing Format code workflow is an issue with our CI when running that workflow on forked reporitories... so don't worry about that (@Flash0ver or I can fix that).

Thank you so much for contributing!

[ericsampson]

@Flash0ver want to do a final re-review whenever you get a chance? Cheers

[codecov]

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.85%. Comparing base (96d9d6c) to head (3901dd0).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Sentry/SentryHttpFailedRequestHandler.cs	97.61%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4724      +/-   ##
==========================================
+ Coverage   73.83%   73.85%   +0.02%     
==========================================
  Files         485      485              
  Lines       17685    17689       +4     
  Branches     3496     3496              
==========================================
+ Hits        13057    13065       +8     
+ Misses       3768     3763       -5     
- Partials      860      861       +1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Flash0ver]

Needs another merge from main ... and I got two more nitpicks.

After that it's a ✅ from me.
Also doing a @sentry review for good measure.

Thanks for the great improvement ... and an additional thank you for improving our existing test suite!

[Flash0ver]

@sentry review

[Flash0ver]

@ericsampson, all ✅ from a review perspective.

There are merge conflicts, that I'm afraid we cannot view nor fix as we don't have push permissions.
I can imagine it's in the CHANGELOG.md. Perhaps we can think of a different way of adding entries to the CHANGELOG.md, as this is most commonly the merge conflict ... maybe we don't add it directly, but place it "somewhere" from where it's then pushed to the CHANGELOG.md in the correct place when (squash) merging 🤔 ... something for us to think about.

Anyway, sorry for that! We'll merge the PR after resolving.

[ericsampson]

@Flash0ver @jamescrosswell thanks for your help getting this through!

Having outside contributors add their updates in a separate temporary CHANGES.md file to prevent merge conflicts might be a good thing.

[jamescrosswell]

@Flash0ver @jamescrosswell thanks for your help getting this through!

Having outside contributors add their updates in a separate temporary CHANGES.md file to prevent merge conflicts might be a good thing.

It's a problem that affects us when making changes from the same repository, to be honest. Fixing it likely requires changes to our release process (which is shared across Sentry) but yes, it's been a pain for long enough that we should do something about it.

[jamescrosswell]

@ericsampson just merged. Huge thank you once again for the contribution!