Add Exception HResult to mechanism data

[Lightczx]

Resolves #3939

Example

This is an example of the event that gets captured by the Sentry.Samples.Console.Basic demo. You can see the HResult along with the other mechanism data at the bottom of the screenshot:

[bruno-garcia]

Thank you for this! 1 review note only

[bruno-garcia]

Could you add a test too please?

We have tests already which you base off of:

sentry-dotnet/test/Sentry.Tests/Internals/MainExceptionProcessorTests.cs

Lines 15 to 25 in 47df982

	public void Process_ExceptionsWithoutData_MechanismDataIsEmpty()
	{
	var sut = _fixture.GetSut();
	var evt = new SentryEvent();
	var ex = GetHandledException();
	sut.Process(ex, evt);
	var sentryException = evt.SentryExceptions!.Single();
	Assert.Empty(sentryException.Mechanism!.Data);
	}

[bruno-garcia]

Oh one more note: https://learn.microsoft.com/en-us/dotnet/api/system.exception.hresult?view=net-9.0

How about we don't add the property if HResult is not default?

Otherwise we'll be getting HResult: 0 for lots of stuff

[whiskhub]

The chance for HResult being 0 in an exception is effectively 0 😅 because 0 means S_OK, so success.
The default HResult in C# is 0x80131500 COR_E_EXCEPTION.

[Lightczx]

Yes, unless someone manually change the HResult to 0, the value will always be like 0x800... . Every .NET BCL Exception have different default HResult.

So we can assume 0 is also meaningful that something is going wrong here (Deliberately set by developer or library author, although almost no one will do so).

[Lightczx]

Could you add a test too please?

Not familiar about that, maybe someone else can help?

[Lightczx]

-2146233088 is 0x80131500 which is COR_E_EXCEPTION

https://github.com/dotnet/runtime/blob/d17420560540e4184a78f431de8341f747068c4f/src/coreclr/pal/prebuilt/inc/corerror.h#L227

If a Exception is throw by BCL, then it may or may not be mapped/wrapped.
Especially for HttpRequestException, if the inner exception is IOException, the HResult is different from which inner exception is SocketException

[whiskhub]

I would avoid adding any filtering here. Any HResult value may give some information. I don‘t think it will confuse anyone, since already today the HResult value can be seen on any exception in the debugger.

[whiskhub]

I've written a short test. Feel free to commit it:

[Fact]
public void Process_ExceptionWithHResult_MechanismDataIsPopulated()
{
	var sut = _fixture.GetSut();
	var evt = new SentryEvent();
	const int RPC_E_WRONG_THREAD = -2147417842;
	const string RPC_E_WRONG_THREAD_MSG = "The application called an interface that was marshalled for a different thread.";
	var ex = new System.Runtime.InteropServices.COMException(RPC_E_WRONG_THREAD_MSG, RPC_E_WRONG_THREAD);

	sut.Process(ex, evt);

	var sentryException = evt.SentryExceptions!.Single();
	var data = sentryException.Mechanism!.Data;

	Assert.Contains(data, pair => pair.Key == "HResult" && pair.Value.Equals("0x8001010E"));
}

[bitsandfoxes]

-2146233088 is 0x80131500 which is COR_E_EXCEPTION
https://github.com/dotnet/runtime/blob/d17420560540e4184a78f431de8341f747068c4f/src/coreclr/pal/prebuilt/inc/corerror.h#L227

@bruno-garcia does it make sense to pull that mapping into the issues details page?

[Lightczx]

pull that mapping into the issues details page

Not quite, that's not the full set of the HRESULT, and COM/WinRT/Win32 components can have custom values.

[bruno-garcia]

Provide an option to disable/enable this behavior?

No need for an option. If HResult is not 0 we should always add the value

[bruno-garcia]

OK I'm convinced. Thanks @Lightczx and @whiskhub
@jamescrosswell do you agree with the reasoning to keep it as-is?

And would you be able to take over this? Add the test I mean and merge it in?

Received: SqlListenerTests.RecordsSqlAsync.DotNet9_0.received.txt
Verified: SqlListenerTests.RecordsSqlAsync.verified.txt

test failure seems unrelated?

[jamescrosswell]

@jamescrosswell do you agree with the reasoning to keep it as-is?

And would you be able to take over this? Add the test I mean and merge it in?

Yep all good. No reason not to send this information really (and I can't think what logic we'd apply to know when it was or wasn't relevant).

We already had tests - so I've updated the expectations on these.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.52%. Comparing base (495e742) to head (db59ed4).
Report is 508 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4029      +/-   ##
==========================================
+ Coverage   75.73%   76.52%   +0.79%     
==========================================
  Files         357      397      +40     
  Lines       13466    14515    +1049     
  Branches     2671     2927     +256     
==========================================
+ Hits        10198    11108     +910     
- Misses       2593     2689      +96     
- Partials      675      718      +43     

☔ 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.