Skip to content

Add Exception HResult to mechanism data #4029

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

Merged
merged 8 commits into from
Mar 17, 2025
Merged

Add Exception HResult to mechanism data #4029

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e45134
impl #3939
Lightczx Mar 8, 2025
b2a029b
Updated tests and changelog
jamescrosswell Mar 14, 2025
7ae51b8
Update CHANGELOG.md
jamescrosswell Mar 14, 2025
dcdbbf7
Merge branch 'main' into main
jamescrosswell Mar 14, 2025
695d32d
Merge branch 'main' into pr/4029
jamescrosswell Mar 16, 2025
07e3d61
Update CHANGELOG.md
jamescrosswell Mar 16, 2025
332d0e4
Verify tests
jamescrosswell Mar 16, 2025
db59ed4
Merge branch 'main' into main
jamescrosswell Mar 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Features

- User Feedback can now be captured without errors/exceptions. Note that these APIs replace the older UserFeedback APIs, which have now been marked as obsolete (and will be removed in a future major version bump) ([#3981](https://github.com/getsentry/sentry-dotnet/pull/3981))
- Exception.HResult is now included in the mechanism data for all exceptions ([#4029](https://github.com/getsentry/sentry-dotnet/pull/4029))

Check failure on line 8 in CHANGELOG.md

View workflow job for this annotation

GitHub Actions / danger / danger 

The changelog entry seems to be part of an already released section `## 5.3.0`.
      Consider moving the entry to the `## Unreleased` section, please.

Check failure on line 8 in CHANGELOG.md

View workflow job for this annotation

GitHub Actions / danger / danger 

The changelog entry seems to be part of an already released section `## 5.3.0`.
      Consider moving the entry to the `## Unreleased` section, please.

### Fixes

Expand Down
3 changes: 3 additions & 0 deletions src/Sentry/Internal/MainExceptionProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,9 @@ private static Mechanism GetMechanism(Exception exception, int id, int? parentId
exception.Data.Remove(Mechanism.DescriptionKey);
}

// Add HResult to mechanism data before adding exception data, so that it can be overridden.
mechanism.Data["HResult"] = $"0x{exception.HResult:X8}";
Copy link
Member

Choose a reason for hiding this comment

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

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);
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

@whiskhub whiskhub Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you add a test too please?

Not familiar about that, maybe someone else can help?

Copy link
Contributor Author

@Lightczx Lightczx Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

-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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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"));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

-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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// Copy remaining exception data to mechanism data.
foreach (var key in exception.Data.Keys.OfType<string>())
{
Expand Down
5 changes: 4 additions & 1 deletion test/Sentry.NLog.Tests/IntegrationTests.Simple.DotNet8_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@
Type: generic,
Handled: true,
Synthetic: false,
IsExceptionGroup: false
IsExceptionGroup: false,
Data: {
HResult: 0x80131500
}
}
}
],
Expand Down
5 changes: 4 additions & 1 deletion test/Sentry.NLog.Tests/IntegrationTests.Simple.DotNet9_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@
Type: generic,
Handled: true,
Synthetic: false,
IsExceptionGroup: false
IsExceptionGroup: false,
Data: {
HResult: 0x80131500
}
}
}
],
Expand Down
5 changes: 4 additions & 1 deletion test/Sentry.NLog.Tests/IntegrationTests.Simple.Mono4_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@
Type: generic,
Handled: true,
Synthetic: false,
IsExceptionGroup: false
IsExceptionGroup: false,
Data: {
HResult: 0x80131500
}
}
}
],
Expand Down
3 changes: 2 additions & 1 deletion test/Sentry.Serilog.Tests/IntegrationTests.Simple.DotNet8_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,8 @@
Synthetic: false,
IsExceptionGroup: false,
Data: {
details: Do work always throws.
details: Do work always throws.,
HResult: 0x80131500
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion test/Sentry.Serilog.Tests/IntegrationTests.Simple.DotNet9_0.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,8 @@
Synthetic: false,
IsExceptionGroup: false,
Data: {
details: Do work always throws.
details: Do work always throws.,
HResult: 0x80131500
}
}
}
Expand Down
13 changes: 10 additions & 3 deletions ....Tests/Internals/MainExceptionProcessorTests.CreateSentryException_Aggregate.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
Synthetic: false,
IsExceptionGroup: false,
ExceptionId: 2,
ParentId: 0
ParentId: 0,
Data: {
HResult: 0x80131500
}
}
},
{
Expand All @@ -20,7 +23,10 @@
Synthetic: false,
IsExceptionGroup: false,
ExceptionId: 1,
ParentId: 0
ParentId: 0,
Data: {
HResult: 0x80131500
}
}
},
{
Expand All @@ -33,7 +39,8 @@
IsExceptionGroup: true,
ExceptionId: 0,
Data: {
foo: bar
foo: bar,
HResult: 0x80131500
}
}
}
Expand Down
17 changes: 14 additions & 3 deletions test/Sentry.Tests/Internals/MainExceptionProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ private class Fixture
private readonly Fixture _fixture = new();

[Fact]
public void Process_ExceptionsWithoutData_MechanismDataIsEmpty()
public void Process_ExceptionsWithoutData_MechanismDataIsMinimal()
{
var sut = _fixture.GetSut();
var evt = new SentryEvent();
Expand All @@ -21,7 +21,12 @@ public void Process_ExceptionsWithoutData_MechanismDataIsEmpty()
sut.Process(ex, evt);

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

// All managed exceptions has an HResult, at a bare minimum (which we store in the Mechanism.Data)
sentryException.Mechanism!.Data.Should().BeEquivalentTo(new Dictionary<string, string>()
{
["HResult"] = "0x80131500" // The default value of HRESULT for managed exceptions (COR_E_EXCEPTION)
});
}

[Fact]
Expand Down Expand Up @@ -109,7 +114,13 @@ public void CreateSentryException_DataHasObjectAsKey_ItemIgnored()

var actual = sut.CreateSentryExceptions(ex).Single();

Assert.Null(actual.Mechanism);
// The custom data won't be added, but the mechanism data will still contain an HResult.
// We add the HResult for all exceptions
actual.Mechanism!.Data.Should().BeEquivalentTo(new Dictionary<string, string>()
{
["HResult"] = "0x80131500" // The default value of HRESULT for managed exceptions (COR_E_EXCEPTION)
});

}

[Fact]
Expand Down
Loading