liefery-it-legacy / bugsnex

API client and logger for Bugsnag
MIT License
10 stars 2 forks source link

Erlang error %Phoenix.Router.NoRouteError reported on 404 after upgrade to plug_cowboy 2 #36

Closed adamniedzielski closed 5 years ago

adamniedzielski commented 5 years ago

After upgrade to plug_cowboy 2, 404 responses started to appear as errors in Bugsnag:

Erlang error: {{%Phoenix.Router.NoRouteError{conn: %Plug.Conn{adapter: {Plug.Cowboy.Conn, :...}, assigns: %{}, before_send: [#Function<1.1812729/1 in Plug.Logger.call/2>], body_params: %{}, cookies: %Plug.Conn.Unfetched{aspect: :cookies}, halted: false, host: "fulfillment-staging.liefery.com", method: "GET", owner: #PID<0.2309.0>, params: %{}, path_info: ["css", "vendor.css"], path_params: %{}, port: 80, private: %{Fulfillment.Router => {[], %{}}, :phoenix_endpoint => Fulfillment.Endpoint, :phoenix_router => Fulfillment.Router, :plug_session_fetch => #Function<1.58261320/1 in Plug.Session.fetch_session/1>}, query_params: %{}, query_string: "", remote_ip: {10, 10, 1, 5}, req_cookies: %Plug.Conn.Unfetched{aspect: :cookies}, req_headers: [{"accept", "text/css,*/*;q=0.1"}, {"accept-encoding", "gzip, deflate, br"}, {"accept-language", "en-US,en;q=0.9,de;q=0.8"}

[redacted]

This didn't happen when using plug_cowboy 1.

It looks like multiple other libraries for error tracking services experienced the similar problem:

I found the content in https://github.com/appsignal/appsignal-elixir/issues/417 particularly informative. Here is the explanation and here is a suggestion to switch from listening to Erlang's error_logger(which we do here) to listening to Elixir's Logger.

We should investigate if the suggestion fixes the problem.

bitboxer commented 5 years ago

I also would love to see this one fixed. If you need any help doing so, please ping here. I would love to chip in a bit of my time for this.

adamniedzielski commented 5 years ago

@bitboxer we reverted to plug_cowboy 1 so that's not a pressing bug for us 😆Feel free to submit a PR

bitboxer commented 5 years ago

So after looking into it, changing the logger like here won't help that much. We just need to filter for the status code. Cowboy 2 changed the behaviour what is logged as an error.

For example like here or here inside of the plug.

What do you think? Would the first approach with everything < 500 should not be reported to bugsnag be okay?

irmela commented 5 years ago

@bitboxer Thanks for looking into it. I think the approach with not reporting < 500 is good enough. Would you be willing to submit a PR?

adamniedzielski commented 5 years ago

👍 sounds good to me.

Thanks for pointing us to https://github.com/appsignal/appsignal-elixir/pull/454. I tried to understand it, but I still have some unanswered questions:

  1. what's the advantage of using :logger over :error_logger?
  2. why Elixir Logger doesn't have a way to extract the error object and stacktrace? Is it a bug / missing important feature in Elixir Logger?
bitboxer commented 5 years ago

I have no answer to your :logger questions. I digged trough the code to see where they actually tried to change the error logger after joses hint to use another logger that you linked above.

From what I see the we need to handle this on the handle_errors inside of the plug and not in the error loggers.

bitboxer commented 5 years ago

If it is still okay, I am happy to provide a PR for this.

adamniedzielski commented 5 years ago

Sure! I posted the questions to show my thinking processes, not necessarily to get the answers 😄