Skip to content

Conversation

@cgarvis
Copy link

@cgarvis cgarvis commented Oct 1, 2025

This module implements the Sentry.HTTPClient behaviour using the Req HTTP client.

This module implements the Sentry.HTTPClient behaviour using the Req HTTP client.
Comment on lines +16 to +18
case Req.post(url, headers: headers, body: body) do
{:ok, %{status: status, headers: headers, body: body}} ->
{:ok, status, headers, body}
Copy link

Choose a reason for hiding this comment

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

Potential bug: The success case for Req.post incorrectly pattern matches on a plain map (%{}) instead of the returned %Req.Response{} struct, which will cause a MatchError.
  • Description: The case statement in Sentry.ReqClient incorrectly pattern matches on a plain map, %{status: status, ...}, for the success case of a Req.post call. According to the Req library's documentation, successful calls return a struct, %Req.Response{...}. In Elixir, a struct will not match a plain map pattern. This mismatch will cause a MatchError to be raised for every successful HTTP request sent via this client, rendering it non-functional for its primary purpose of sending events to Sentry.

  • Suggested fix: Update the pattern match in the case statement to expect a %Req.Response{} struct. Specifically, change the line {:ok, %{status: status, headers: headers, body: body}} to {:ok, %Req.Response{status: status, headers: headers, body: body}}.
    severity: 0.9, confidence: 1.0

Did we get this right? 👍 / 👎 to inform future reviews.

@whatyouhide
Copy link
Collaborator

@cgarvis we already have a PR that replaces Hackney with Finch. I think that's a preferred route—this is not exposed to users and using Req means using one more dependency, but mostly for the sake of the Sentry SDK maintainers and not of end users. Any particular reasons you wanted to go with this over Finch and the existing PR?

@cgarvis
Copy link
Author

cgarvis commented Oct 14, 2025

Req doesn’t supersede the Finch PR, it actually builds on it. Req uses Finch under the hood, so this adapter still benefits from that work. The goal here is to support multiple adapters since the Elixir ecosystem tends to favor flexibility. With Phoenix and other libraries standardizing on Req, offering an adapter makes it easier for projects to configure and align their stack without additional boilerplate.

@whatyouhide
Copy link
Collaborator

@cgarvis I think I’m missing the point of having another adapter then. Why would someone care about the adapter that the SDK uses unless it's for dependencies reasons?

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.

2 participants