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

Allow logging error response body #21

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SteffiPeTaffy
Copy link

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?

@hamvocke
Copy link

hamvocke commented Jun 1, 2021

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! :)

@WouterDeKort
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants