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.

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

ref: Handle non json error response messages. #690

Merged
merged 4 commits into from
Dec 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# Changelog

## vNext
## Unreleased

- Handle non-json error response messages on HttpTransport. (#690) @lucas-zimerman
- Fix deadlock on missing ConfigureAwait into foreach loops. (#694) @lucas-zimerman

## 3.0.0-alpha.8
Expand Down
54 changes: 35 additions & 19 deletions src/Sentry/Internal/Http/HttpTransport.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,25 +111,41 @@ public async Task SendEnvelopeAsync(Envelope envelope, CancellationToken cancell
}
else if (_options.DiagnosticLogger?.IsEnabled(SentryLevel.Error) == true)
{
var responseJson = await response.Content.ReadAsJsonAsync(cancellationToken).ConfigureAwait(false);

var errorMessage =
responseJson.GetPropertyOrNull("detail")?.GetString()
?? DefaultErrorMessage;

var errorCauses =
responseJson.GetPropertyOrNull("causes")?.EnumerateArray().Select(j => j.GetString()).ToArray()
?? Array.Empty<string>();

_options.DiagnosticLogger?.Log(
SentryLevel.Error,
"Sentry rejected the envelope {0}. Status code: {1}. Error detail: {2}. Error causes: {3}.",
null,
processedEnvelope.TryGetEventId(),
response.StatusCode,
errorMessage,
string.Join(", ", errorCauses)
);
if (response.Content.Headers.ContentType?.MediaType is "application/json")
{
var responseJson = await response.Content.ReadAsJsonAsync(cancellationToken).ConfigureAwait(false);

var errorMessage =
responseJson.GetPropertyOrNull("detail")?.GetString()
?? DefaultErrorMessage;

var errorCauses =
responseJson.GetPropertyOrNull("causes")?.EnumerateArray().Select(j => j.GetString()).ToArray()
?? Array.Empty<string>();

_options.DiagnosticLogger?.Log(
SentryLevel.Error,
"Sentry rejected the envelope {0}. Status code: {1}. Error detail: {2}. Error causes: {3}.",
null,
Copy link
Member

Choose a reason for hiding this comment

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

Lets have an overload that doesn't take null

processedEnvelope.TryGetEventId(),
response.StatusCode,
errorMessage,
string.Join(", ", errorCauses)
);
}
else
{
var responseString = await response.Content.ReadAsStringAsync().ConfigureAwait(false);

_options.DiagnosticLogger?.Log(
SentryLevel.Error,
"Sentry rejected the envelope {0}. Status code: {1}. Error detail: {2}.",
null,
processedEnvelope.TryGetEventId(),
response.StatusCode,
responseString
);
}
}
}

Expand Down
11 changes: 9 additions & 2 deletions test/Sentry.Testing/SentryResponses.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Net;
using System.Net.Http;
using System.Text;
using System.Text.Json;

namespace Sentry.Testing
Expand All @@ -20,15 +21,21 @@ public static HttpResponseMessage GetOkResponse()
Content = GetOkContent()
};

public static HttpResponseMessage GetErrorResponse(HttpStatusCode code, string detail, string[] causes = null)
public static HttpResponseMessage GetJsonErrorResponse(HttpStatusCode code, string detail, string[] causes = null)
{
var responseContent = causes != null
? JsonSerializer.Serialize(new {detail, causes})
: JsonSerializer.Serialize(new {detail});

return new HttpResponseMessage(code) {Content = new StringContent(responseContent)};
return new HttpResponseMessage(code) {Content = new StringContent(responseContent, Encoding.UTF8, "application/json") };
}

public static HttpResponseMessage GetTextErrorResponse(HttpStatusCode code, string detail)
=> new HttpResponseMessage(code)
{
Content = new StringContent(detail)
};

public static HttpResponseMessage GetRateLimitResponse(string rateLimitHeaderValue)
{
return new((HttpStatusCode)429)
Expand Down
46 changes: 43 additions & 3 deletions test/Sentry.Tests/Internals/Http/HttpTransportTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ await httpHandler
}

[Fact]
public async Task SendEnvelopeAsync_ResponseNotOkWithMessage_LogsError()
public async Task SendEnvelopeAsync_ResponseNotOkWithJsonMessage_LogsError()
{
// Arrange
const HttpStatusCode expectedCode = HttpStatusCode.BadGateway;
Expand All @@ -63,7 +63,7 @@ public async Task SendEnvelopeAsync_ResponseNotOkWithMessage_LogsError()
var httpHandler = Substitute.For<MockableHttpMessageHandler>();

httpHandler.VerifiableSendAsync(Arg.Any<HttpRequestMessage>(), Arg.Any<CancellationToken>())
.Returns(_ => SentryResponses.GetErrorResponse(expectedCode, expectedMessage, expectedCauses));
.Returns(_ => SentryResponses.GetJsonErrorResponse(expectedCode, expectedMessage, expectedCauses));

var logger = new AccumulativeDiagnosticLogger();

Expand Down Expand Up @@ -94,6 +94,46 @@ public async Task SendEnvelopeAsync_ResponseNotOkWithMessage_LogsError()
).Should().BeTrue();
}

[Fact]
public async Task SendEnvelopeAsync_ResponseNotOkWithStringMessage_LogsError()
{
// Arrange
const HttpStatusCode expectedCode = HttpStatusCode.RequestEntityTooLarge;
const string expectedMessage = "413 Request Entity Too Large";

var httpHandler = Substitute.For<MockableHttpMessageHandler>();

_ = httpHandler.VerifiableSendAsync(Arg.Any<HttpRequestMessage>(), Arg.Any<CancellationToken>())
.Returns(_ => SentryResponses.GetTextErrorResponse(expectedCode, expectedMessage));

var logger = new AccumulativeDiagnosticLogger();

var httpTransport = new HttpTransport(
new SentryOptions
{
Dsn = DsnSamples.ValidDsnWithSecret,
Debug = true,
DiagnosticLogger = logger
},
new HttpClient(httpHandler)
);

var envelope = Envelope.FromEvent(new SentryEvent());

// Act
await httpTransport.SendEnvelopeAsync(envelope);

// Assert
_ = logger.Entries.Any(e =>
e.Level == SentryLevel.Error &&
e.Message == "Sentry rejected the envelope {0}. Status code: {1}. Error detail: {2}." &&
e.Exception == null &&
e.Args[0].ToString() == envelope.TryGetEventId().ToString() &&
e.Args[1].ToString() == expectedCode.ToString() &&
e.Args[2].ToString() == expectedMessage
).Should().BeTrue();
}

[Fact]
public async Task SendEnvelopeAsync_ResponseNotOkNoMessage_LogsError()
{
Expand All @@ -103,7 +143,7 @@ public async Task SendEnvelopeAsync_ResponseNotOkNoMessage_LogsError()
var httpHandler = Substitute.For<MockableHttpMessageHandler>();

httpHandler.VerifiableSendAsync(Arg.Any<HttpRequestMessage>(), Arg.Any<CancellationToken>())
.Returns(_ => SentryResponses.GetErrorResponse(expectedCode, null));
.Returns(_ => SentryResponses.GetJsonErrorResponse(expectedCode, null));

var logger = new AccumulativeDiagnosticLogger();

Expand Down