Skip to content
This repository has been archived by the owner on Jun 21, 2024. It is now read-only.

feat: Track response in error details #29

Merged
merged 8 commits into from
May 3, 2024

Conversation

tomasfarias
Copy link
Contributor

@tomasfarias tomasfarias commented Apr 26, 2024

When debugging apps, it's useful to have error messages. The problem is that we are only producing to app_metrics the message generated by reqwest (specifically, the message generated by this impl: https://github.com/seanmonstar/reqwest/blob/master/src/error.rs#L185).

This error message is useful, but to a limited degree: only status code and url are included in the message. Usually, the server's response can contain more information about what failed. As an example, see how the Django API includes more details on what failed in its default response:

curl -X POST -H "Content-Type: application/json" \ 
--data '{"api_key": "<redacted>", "batch": []}' https://app.posthog.com/batch
{"type": "validation_error", "code": "invalid_payload", "detail": "Invalid payload: All events must have the event name field \"event\"!", "attr": null}

This is valuable information we are currently suppressing in our handling of webhook errors. We should record this error message to be displayed in PostHog to assist with debugging.

Changes:

  • WebhookJobError now is initialized directly from a WebhookRequestError, which is a new enum of errors that contains the response text.
  • The display implementation of WebhookRequestError appends the response contents, if any.

Tests:

Included a unit test to check the error string contains the response body.

Comment on lines +400 to +402
match response.error_for_status_ref() {
Ok(_) => Ok(response),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice we are using a reference to response here when matching as error_for_status would have taken ownership of the response, and we need to keep ownership if we intend to return it.

@tiina303 tiina303 requested a review from a team April 26, 2024 14:19
hook-worker/src/worker.rs Outdated Show resolved Hide resolved
@tomasfarias tomasfarias force-pushed the feat/track-response-in-error-details branch from c47f652 to 6743384 Compare May 2, 2024 10:09
@tomasfarias
Copy link
Contributor Author

Had to rebase to pull in fix for broken dependency in #32

Comment on lines +44 to +52
#[derive(Error, Debug)]
pub enum WebhookResponseError {
#[error("failed to parse a response as UTF8")]
ParseUTF8StringError(#[from] std::str::Utf8Error),
#[error("error while iterating over response body chunks")]
StreamIterationError(#[from] reqwest::Error),
#[error("attempted to slice a chunk of length {0} with an out of bounds index of {1}")]
ChunkOutOfBoundsError(usize, usize),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not handling these at the moment. hook-worker just defaults to an empty response on error.

@tomasfarias tomasfarias merged commit f7e02cc into main May 3, 2024
4 checks passed
@tomasfarias tomasfarias deleted the feat/track-response-in-error-details branch May 3, 2024 08:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants