Create a Sentry event for failed HTTP requests #2320
Conversation
|
Good approach. 👍 |
Suggestions from code review Co-authored-by: Matt Johnson-Pint <matt.johnson-pint@sentry.io>
…or when start > end... - Tidied whitespace - Enabled warnings when not using file scoped namespaces in the .editorconfig
- Moved Response to Sentry.Protocol - Fixed 2 tests that were failing on .NET 4.8
|
OK I think this is working now... @mattjohnsonpint see sample event. |
…o feat/http-client-errors
…d SubstringOrRegexPattern
mattjohnsonpint
changed the title
|
|
||
| // The ContentLength might be null (but that's ok). | ||
| // See https://github.com/dotnet/runtime/issues/16162 | ||
| var bodySize = response.Content?.Headers?.ContentLength; |
There was a problem hiding this comment.
@mattjohnsonpint this was the approach I'd originally tried. However it's not that it returns null - when running tests against the net43 target it sometimes throws a System.ObjectDisposedException... so I don't think this code is safe.
I also tried things like checking response.Content?.Headers.?.Contains("Content-Length") before reading this, which avoided the exception on net43 but would prevent us from capturing the BodySize in later versions of .NET where it could be calculated but wasn't stored in the header.
It looks like HttpContent.TryComputeLength (which I think is where they fixed all this) was introduced in .NET 4.5... so possibly NET5_0_OR_GREATER is the wrong cut-off point...
An alternative I toyed with was try... catch(ObjectDisposedException ex) {}. Possibly that's a better solution but it would definitely need to be accompanied by a code comment explaining why it was there... we'd need to keep using an #if directive in the unit tests, but it would mean we'd be capturing the BodySize in all circumstances where this was possible (failing only when an exception was thrown by older versions of the .NET Framework).
There was a problem hiding this comment.
It's safe now, as it's been moved before EnsureSuccessStatusCode which used to dispose the content (which is why you got ObjectDisposedException). See my comment just above this one.
As for specifically checking "Content-Length" - we could add that if bodySize == null, but I'd want to reproduce the issue first. I tried locally (on both NetFx and Mono) and the value was always present if it came from an actual HttpClient instance. And if it came just from ResponseMessage (like in the tests), then Headers was null, so there'd be nothing to check against.
Either way, I'm not too worried about it, if it happens to be absent. The disposed exception is the main thing, which is taken care of.
There was a problem hiding this comment.
Ah, interesting... How did you work out where Dispose was being called?
I also just reaslised that relying on EnsureSuccessStatusCode to throw an exception means it's not possible to configure FailedRequestStatusCodes in the 200-299 range... nothing in that range will result in an exception being thrown here that will be captured by Sentry.
I can't think why someone would want to do that... Something to be aware of though.
Captures failed HTTP Requests as a Sentry error event.
Closes #2217