Skip to content

Commit 356f8e9

Browse files
ericsampsonericsampson
authored
Improve SentryHttpFailedRequestHandler issue grouping (#4724)
1 parent 71f982a commit 356f8e9

File tree

7 files changed

+431
-143
lines changed

7 files changed

+431
-143
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
### Fixes
66

7+
- Captured [Http Client Errors](https://docs.sentry.io/platforms/dotnet/guides/aspnet/configuration/http-client-errors/) on .NET 5+ now include a full stack trace in order to improve Issue grouping ([#4724](https://github.com/getsentry/sentry-dotnet/pull/4724))
78
- Sentry Tracing middleware crashed ASP.NET Core in .NET 10 in 6.0.0-rc.1 and earlier ([#4747](https://github.com/getsentry/sentry-dotnet/pull/4747))
89

910
### Dependencies

‎src/Sentry/HttpStatusCodeRangeExtensions.cs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
namespace Sentry;
2+
3+
/// <summary>
4+
/// Extension methods for collections of <see cref="HttpStatusCodeRange"/>.
5+
/// </summary>
6+
internal static class HttpStatusCodeRangeExtensions
7+
{
8+
/// <summary>
9+
/// Checks if any range in the collection contains the given status code.
10+
/// </summary>
11+
/// <param name="ranges">Collection of ranges to check.</param>
12+
/// <param name="statusCode">Status code to check.</param>
13+
/// <returns>True if any range contains the given status code.</returns>
14+
internal static bool ContainsStatusCode(this IEnumerable<HttpStatusCodeRange> ranges, int statusCode)
15+
=> ranges.Any(range => range.Contains(statusCode));
16+
17+
/// <summary>
18+
/// Checks if any range in the collection contains the given status code.
19+
/// </summary>
20+
/// <param name="ranges">Collection of ranges to check.</param>
21+
/// <param name="statusCode">Status code to check.</param>
22+
/// <returns>True if any range contains the given status code.</returns>
23+
internal static bool ContainsStatusCode(this IEnumerable<HttpStatusCodeRange> ranges, HttpStatusCode statusCode)
24+
=> ranges.ContainsStatusCode((int)statusCode);
25+
}

‎src/Sentry/Internal/Extensions/HttpStatusExtensions.cs‎

Lines changed: 0 additions & 28 deletions
This file was deleted.

‎src/Sentry/SentryHttpFailedRequestHandler.cs‎

Lines changed: 61 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -15,67 +15,85 @@ internal SentryHttpFailedRequestHandler(IHub hub, SentryOptions options)
1515
protected internal override void DoEnsureSuccessfulResponse([NotNull] HttpRequestMessage request, [NotNull] HttpResponseMessage response)
1616
{
1717
// Don't capture events for successful requests
18-
if (!Options.FailedRequestStatusCodes.Any(range => range.Contains(response.StatusCode)))
18+
if (!Options.FailedRequestStatusCodes.ContainsStatusCode(response.StatusCode))
1919
{
2020
return;
2121
}
2222

2323
// Capture the event
24-
try
24+
25+
var statusCode = (int)response.StatusCode;
26+
// Match behavior of HttpResponseMessage.EnsureSuccessStatusCode
27+
// See https://github.com/getsentry/sentry-dotnet/issues/2684
28+
if (statusCode >= 200 && statusCode <= 299)
2529
{
30+
return;
31+
}
32+
33+
var exception = new HttpRequestException(
34+
string.Format(
35+
System.Globalization.CultureInfo.InvariantCulture,
36+
"Response status code does not indicate success: {0}",
37+
statusCode)
38+
);
39+
2640
#if NET5_0_OR_GREATER
27-
response.EnsureSuccessStatusCode();
41+
// Add a full stack trace into the exception to improve Issue grouping,
42+
// see https://github.com/getsentry/sentry-dotnet/issues/3582
43+
ExceptionDispatchInfo.SetCurrentStackTrace(exception);
2844
#else
29-
// Use our own implementation of EnsureSuccessStatusCode because older implementations of
30-
// EnsureSuccessStatusCode disposes the content.
31-
// See https://github.com/dotnet/runtime/issues/24845
32-
response.StatusCode.EnsureSuccessStatusCode();
33-
#endif
45+
// Where SetRemoteStackTrace is not available, throw and catch to get a basic stack trace
46+
try
47+
{
48+
throw exception;
3449
}
35-
catch (HttpRequestException exception)
50+
catch (HttpRequestException ex)
3651
{
37-
exception.SetSentryMechanism(MechanismType);
52+
exception = ex;
53+
}
54+
#endif
55+
56+
exception.SetSentryMechanism(MechanismType);
3857

39-
var @event = new SentryEvent(exception);
40-
var hint = new SentryHint(HintTypes.HttpResponseMessage, response);
58+
var @event = new SentryEvent(exception);
59+
var hint = new SentryHint(HintTypes.HttpResponseMessage, response);
4160

42-
var uri = response.RequestMessage?.RequestUri;
43-
var sentryRequest = new SentryRequest
44-
{
45-
QueryString = uri?.Query,
46-
Method = response.RequestMessage?.Method.Method.ToUpperInvariant()
47-
};
61+
var uri = response.RequestMessage?.RequestUri;
62+
var sentryRequest = new SentryRequest
63+
{
64+
QueryString = uri?.Query,
65+
Method = response.RequestMessage?.Method.Method.ToUpperInvariant()
66+
};
4867

49-
var responseContext = new Response
50-
{
51-
StatusCode = (short)response.StatusCode,
68+
var responseContext = new Response
69+
{
70+
StatusCode = (short)response.StatusCode,
5271
#if NET5_0_OR_GREATER
53-
// Starting with .NET 5, the content and headers are guaranteed to not be null.
54-
BodySize = response.Content.Headers.ContentLength
72+
// Starting with .NET 5, the content and headers are guaranteed to not be null.
73+
BodySize = response.Content.Headers.ContentLength
5574
#else
56-
// The ContentLength might be null (but that's ok).
57-
// See https://github.com/dotnet/runtime/issues/16162
58-
BodySize = response.Content?.Headers?.ContentLength
75+
// The ContentLength might be null (but that's ok).
76+
// See https://github.com/dotnet/runtime/issues/16162
77+
BodySize = response.Content?.Headers?.ContentLength
5978
#endif
60-
};
79+
};
6180

62-
if (!Options.SendDefaultPii)
63-
{
64-
sentryRequest.Url = uri?.HttpRequestUrl();
65-
}
66-
else
67-
{
68-
sentryRequest.Url = uri?.AbsoluteUri;
69-
sentryRequest.Cookies = request.Headers.GetCookies();
70-
sentryRequest.AddHeaders(request.Headers);
71-
responseContext.Cookies = response.Headers.GetCookies();
72-
responseContext.AddHeaders(response.Headers);
73-
}
81+
if (!Options.SendDefaultPii)
82+
{
83+
sentryRequest.Url = uri?.HttpRequestUrl();
84+
}
85+
else
86+
{
87+
sentryRequest.Url = uri?.AbsoluteUri;
88+
sentryRequest.Cookies = request.Headers.GetCookies();
89+
sentryRequest.AddHeaders(request.Headers);
90+
responseContext.Cookies = response.Headers.GetCookies();
91+
responseContext.AddHeaders(response.Headers);
92+
}
7493

75-
@event.Request = sentryRequest;
76-
@event.Contexts[Response.Type] = responseContext;
94+
@event.Request = sentryRequest;
95+
@event.Contexts[Response.Type] = responseContext;
7796

78-
Hub.CaptureEvent(@event, hint: hint);
79-
}
97+
Hub.CaptureEvent(@event, hint: hint);
8098
}
8199
}

‎test/Sentry.Tests/HttpStatusCodeRangeExtensionsTests.cs‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
namespace Sentry.Tests;
2+
3+
public class HttpStatusCodeRangeExtensionsTests
4+
{
5+
[Fact]
6+
public void ContainsStatusCode_EmptyList_ReturnsFalse()
7+
{
8+
// Arrange
9+
var ranges = new List<HttpStatusCodeRange>();
10+
11+
// Act
12+
var result = ranges.ContainsStatusCode(404);
13+
14+
// Assert
15+
result.Should().BeFalse();
16+
}
17+
18+
[Theory]
19+
[InlineData(400)]
20+
[InlineData(450)]
21+
[InlineData(499)]
22+
public void ContainsStatusCode_SingleRangeInRange_ReturnsTrue(int statusCode)
23+
{
24+
// Arrange
25+
var ranges = new List<HttpStatusCodeRange> { (400, 499) };
26+
27+
// Act
28+
var result = ranges.ContainsStatusCode(statusCode);
29+
30+
// Assert
31+
result.Should().BeTrue();
32+
}
33+
34+
[Theory]
35+
[InlineData(200)]
36+
[InlineData(399)]
37+
[InlineData(500)]
38+
public void ContainsStatusCode_SingleRangeOutOfRange_ReturnsFalse(int statusCode)
39+
{
40+
// Arrange
41+
var ranges = new List<HttpStatusCodeRange> { (400, 499) };
42+
43+
// Act
44+
var result = ranges.ContainsStatusCode(statusCode);
45+
46+
// Assert
47+
result.Should().BeFalse();
48+
}
49+
50+
[Theory]
51+
[InlineData(400)] // In first range
52+
[InlineData(404)] // In first range
53+
[InlineData(500)] // In second range
54+
[InlineData(503)] // In second range
55+
public void ContainsStatusCode_MultipleRangesInAnyRange_ReturnsTrue(int statusCode)
56+
{
57+
// Arrange
58+
var ranges = new List<HttpStatusCodeRange> { (400, 404), (500, 503) };
59+
60+
// Act
61+
var result = ranges.ContainsStatusCode(statusCode);
62+
63+
// Assert
64+
result.Should().BeTrue();
65+
}
66+
67+
[Theory]
68+
[InlineData(200)] // Below ranges
69+
[InlineData(405)] // Between ranges
70+
[InlineData(499)] // Between ranges
71+
[InlineData(504)] // Above ranges
72+
public void ContainsStatusCode_MultipleRangesNotInAnyRange_ReturnsFalse(int statusCode)
73+
{
74+
// Arrange
75+
var ranges = new List<HttpStatusCodeRange> { (400, 404), (500, 503) };
76+
77+
// Act
78+
var result = ranges.ContainsStatusCode(statusCode);
79+
80+
// Assert
81+
result.Should().BeFalse();
82+
}
83+
84+
[Theory]
85+
[InlineData(400)] // In first range only
86+
[InlineData(425)] // In overlap
87+
[InlineData(450)] // In overlap
88+
[InlineData(475)] // In second range only
89+
public void ContainsStatusCode_OverlappingRangesInUnion_ReturnsTrue(int statusCode)
90+
{
91+
// Arrange
92+
var ranges = new List<HttpStatusCodeRange> { (400, 450), (425, 475) };
93+
94+
// Act
95+
var result = ranges.ContainsStatusCode(statusCode);
96+
97+
// Assert
98+
result.Should().BeTrue();
99+
}
100+
101+
[Theory]
102+
[InlineData(200)] // Below first range
103+
[InlineData(399)] // Below first range
104+
[InlineData(476)] // Above second range
105+
[InlineData(500)] // Above second range
106+
public void ContainsStatusCode_OverlappingRangesOutsideUnion_ReturnsFalse(int statusCode)
107+
{
108+
// Arrange
109+
var ranges = new List<HttpStatusCodeRange> { (400, 450), (425, 475) };
110+
111+
// Act
112+
var result = ranges.ContainsStatusCode(statusCode);
113+
114+
// Assert
115+
result.Should().BeFalse();
116+
}
117+
118+
[Fact]
119+
public void ContainsStatusCode_SingleValueRangeExactMatch_ReturnsTrue()
120+
{
121+
// Arrange
122+
var ranges = new List<HttpStatusCodeRange> { 404 };
123+
124+
// Act
125+
var result = ranges.ContainsStatusCode(404);
126+
127+
// Assert
128+
result.Should().BeTrue();
129+
}
130+
131+
[Theory]
132+
[InlineData(403)]
133+
[InlineData(405)]
134+
public void ContainsStatusCode_SingleValueRangeNoMatch_ReturnsFalse(int statusCode)
135+
{
136+
// Arrange
137+
var ranges = new List<HttpStatusCodeRange> { 404 };
138+
139+
// Act
140+
var result = ranges.ContainsStatusCode(statusCode);
141+
142+
// Assert
143+
result.Should().BeFalse();
144+
}
145+
146+
[Fact]
147+
public void ContainsStatusCode_HttpStatusCodeEnumInRange_ReturnsTrue()
148+
{
149+
// Arrange
150+
var ranges = new List<HttpStatusCodeRange> { (400, 499) };
151+
152+
// Act
153+
var result = ranges.ContainsStatusCode(HttpStatusCode.NotFound); // 404
154+
155+
// Assert
156+
result.Should().BeTrue();
157+
}
158+
159+
[Fact]
160+
public void ContainsStatusCode_HttpStatusCodeEnumOutOfRange_ReturnsFalse()
161+
{
162+
// Arrange
163+
var ranges = new List<HttpStatusCodeRange> { (400, 499) };
164+
165+
// Act
166+
var result = ranges.ContainsStatusCode(HttpStatusCode.OK); // 200
167+
168+
// Assert
169+
result.Should().BeFalse();
170+
}
171+
}

0 commit comments

Comments
 (0)