StackExchange.Utils icon indicating copy to clipboard operation
StackExchange.Utils copied to clipboard

Allow logging error response body

Open SteffiPeTaffy opened this issue 4 years ago • 2 comments

Current limitation

Right now the library doesn't support logging the response body of an error response. This makes it difficult for monitoring/debugging/troubleshooting.

Current workaround

var strResponse = await Http
      .Request("http://example.com")
      // don't log 403's as errors so we can get the details out of the response body
      .WithoutLogging(HttpStatusCode.Forbidden)
      .ExpectString()
      .GetAsync();

if (!strResponse.Success)
{
 // log 
}
else
{
// deserialize
}

Proposed solution

Add extension modifier method that allows you to log error response bodies for given status codes as exception data.

  var response = await Http
      .Request("http://example.com")
      .WithErrorResponseBodyLogging(HttpStatusCode.Forbidden)
      .ExpectJson<SomeResponseObject>()
      .GetAsync();

Open questions

  • I am reading the response content stream outside of a handler method. Is there a possibility where this could happen twice?
  • Is the httpmock dep okay to use for unit testing?

SteffiPeTaffy avatar Jun 01 '21 08:06 SteffiPeTaffy

For context:

We had this discussion in Bonfire a few days ago where we discussed this change. I talked about this issue with Steffi and she was eager to tackle this challenge.

Initially, we thought we'd introduce a new extension that allows us to return a success type and an error type (something like .ExpectJson<TSuccess, TError>()) but we abandoned that idea. All usages where we're currently using the workaround code that Steffi outlined above are not using the parsed error response type for anything but throwing it into the exception log. As a consequence we thought it might be easier to just attach the error response body to the exception data directly and not expose the error type in any way.

Happy to discuss and find the best way forward! :)

hamvocke avatar Jun 01 '21 08:06 hamvocke

I like it!

This PR would give us more info in OpServer and really help us on escalation duty. I think the implementation with the automated tests is nice. Not sure how we deal with merging PRs on this repo but I +1 this.

WouterDeKort avatar Aug 16 '21 12:08 WouterDeKort