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

inspect request in sentry context #3180

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

ruslandoga
Copy link
Contributor

Changes

Fixes #3166

Tests

  • This PR does not require tests

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@@ -12,7 +12,7 @@ defmodule PlausibleWeb.Api.ExternalController do

def event(conn, _params) do
with {:ok, request} <- Ingestion.Request.build(conn),
_ <- Sentry.Context.set_extra_context(%{request: request}) do
_ <- Sentry.Context.set_extra_context(%{request: inspect(request)}) do
Copy link
Contributor Author

@ruslandoga ruslandoga Jul 24, 2023

Choose a reason for hiding this comment

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

Nitpick: we can use _ = Sentry.Context.set_extra_context(%{request: inspect(request)}) here since the result doesn't matter (to avoid with expanding to an extra case). And then since with has only one case in pipeline and else is handled, we can switch to case to make it less magical:

  def event(conn, _params) do
    case Ingestion.Request.build(conn) do
      {:ok, request} ->
        Sentry.Context.set_extra_context(%{request: inspect(request)})

        case Ingestion.Event.build_and_buffer(request) do
          {:ok, %{dropped: [], buffered: _buffered}} ->
            conn
            |> put_status(202)
            |> text("ok")

          {:ok, %{dropped: dropped, buffered: _}} ->
            first_invalid_changeset = find_first_invalid_changeset(dropped)

            if first_invalid_changeset do
              conn
              |> put_resp_header("x-plausible-dropped", "#{Enum.count(dropped)}")
              |> put_status(400)
              |> json(%{
                errors: Plausible.ChangesetHelpers.traverse_errors(first_invalid_changeset)
              })
            else
              conn
              |> put_resp_header("x-plausible-dropped", "#{Enum.count(dropped)}")
              |> put_status(202)
              |> text("ok")
            end
        end

      {:error, %Ecto.Changeset{} = changeset} ->
        conn
        |> put_status(400)
        |> json(%{errors: Plausible.ChangesetHelpers.traverse_errors(changeset)})
    end
  end

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, case fits here much better. Would you like to include that change?

Copy link
Contributor Author

@ruslandoga ruslandoga Jul 24, 2023

Choose a reason for hiding this comment

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

I'd prefer to do it in a separate PR to make the fix clear to anyone who checks the changelog :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I forgot to update changelog!

@bundlemon
Copy link

bundlemon bot commented Jul 24, 2023

BundleMon

Unchanged files (7)
Status Path Size Limits
static/css/app.css
492.34KB -
static/js/dashboard.js
318.55KB -
static/js/app.js
40.11KB -
static/js/embed.host.js
5.58KB -
static/js/embed.content.js
5.08KB -
tracker/js/plausible.js
742B -
static/js/applyTheme.js
314B -

No change in files bundle size

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@aerosol aerosol merged commit a9603a4 into plausible:master Jul 24, 2023
1 of 4 checks passed
@aerosol
Copy link
Member

aerosol commented Jul 24, 2023

Welp, looks like we've not been getting much sentry reports.
However the requests are filtered:

image

@aerosol
Copy link
Member

aerosol commented Jul 24, 2023

I think we should JSON encode it for best results. @ruslandoga do you have capacity to look into it? I can pick it up as well.

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Jul 24, 2023

@ruslandoga do you have capacity to look into it?

Sure do! :) I'll do it right after plausible/ecto_ch#92

@ruslandoga ruslandoga deleted the fix-sentry-with-request branch July 24, 2023 09:02
@aerosol
Copy link
Member

aerosol commented Jul 24, 2023

Awesome, while at it could you make sure this: https://github.com/plausible/analytics/blob/master/lib/plausible/ingestion/event.ex#L376 is fingerprinted in Sentry.EventFilter please? There's quite a lot of it suddenly :)

@ruslandoga ruslandoga mentioned this pull request Jul 24, 2023
4 tasks
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