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

Use a separate client process #93

Merged
merged 12 commits into from Oct 10, 2017

Conversation

Projects
None yet
2 participants
@sorentwo
Contributor

sorentwo commented Sep 25, 2017

This PR changes the how the client works, replacing the use of tasks for a supervised client. It ended up being larger than intended because the changes were difficult to isolate—on the plus side, it covers at least three open issues.

  • Drops HTTPoison in favor of directly using Hackney, which gives us access to a connection pool
  • Drops Meck and stubbing in favor of a local cowboy server (#7)
  • Changes notify from a macro to a function
  • Stops spawning new tasks for every error, instead relying on async handling in the client (#88)
  • Increases the logging around client activity (#20)
  • Starts a supervision tree with the client as a child

It is based off of #89, largely to prevent merge conflicts and simplify the changes.

@joshuap

This comment has been minimized.

Show comment
Hide comment
@joshuap

joshuap Oct 3, 2017

Member

@sorentwo I can't wait to go through this, it sounds great. I'm planning to dig in later this week. Any ideas on the build failures? I can pull this down and run the suite locally--I assume they are related to the changes around dropping Meck. Assuming the build is passing locally I can probably fix Travis.

By the way, if anyone else feels like giving this a code review, feel free.

Member

joshuap commented Oct 3, 2017

@sorentwo I can't wait to go through this, it sounds great. I'm planning to dig in later this week. Any ideas on the build failures? I can pull this down and run the suite locally--I assume they are related to the changes around dropping Meck. Assuming the build is passing locally I can probably fix Travis.

By the way, if anyone else feels like giving this a code review, feel free.

sorentwo added some commits Sep 18, 2017

Consistently use get_env for all config access
By using `Honeybadger.get_env/1` throughout we can universally use
standard values or `:system` tuples.
Refactor client module into a GenServer
This changes the way notifications are enqueued and emitted. Rather than
kicking off one task for every error notification we ask a common server
to do it instead. This has numerous benefits, including:

* A stacktrace that doesn't come from a one off task
* The client will be restarted if it crashes
* Centralized state management
* Simpler testing with a mocked http client
Test notifications using a fake api server
This begins the process of removing `meck` based testing, instead
relying on a local api server that sends messages back for verification.
Remove mock testing from logger tests
The server restructuring causes some trouble with recursive logging, so
that is being re-worked.
@sorentwo

This comment has been minimized.

Show comment
Hide comment
@sorentwo

sorentwo Oct 3, 2017

Contributor

@joshuap The build is passing again. I forgot that left not in right wasn't [added until recently[(https://github.com/elixir-lang/elixir/pull/5620), so the older versions were getting syntax errors.

Everything is fixed up now and this has been rebased off master, mostly to get the additional builds from the matrix update.

Contributor

sorentwo commented Oct 3, 2017

@joshuap The build is passing again. I forgot that left not in right wasn't [added until recently[(https://github.com/elixir-lang/elixir/pull/5620), so the older versions were getting syntax errors.

Everything is fixed up now and this has been rebased off master, mostly to get the additional builds from the matrix update.

@joshuap

Hi @sorentwo, thanks again for this, it's looking great overall. I added a few comments for you to look at. Once you take a look at those I'll do some more testing.

Show outdated Hide outdated lib/honeybadger/client.ex
Show outdated Hide outdated lib/honeybadger.ex
Show outdated Hide outdated lib/honeybadger.ex
|> Enum.into(Keyword.new)
|> Keyword.put(:pool, __MODULE__)
case :hackney.post(state.url, state.headers, payload, opts) do

This comment has been minimized.

@joshuap

joshuap Oct 4, 2017

Member

In a test app I'm trying to configure honeybadger via the HONEYBADGER_API_KEY environment variable. In a phoneix app, the app boots up fine (it determines the environment variable is present), but when it reports an error the Client GenServer is crashing with a bunch of errors like this (eventually the application shuts down instead of looping forever, which is nice):

14:37:52.948 request_id=b5a0okef3ctq36ktl0skaa4jafbreqb8 [info] GET /runtime_error
14:37:52.973 request_id=b5a0okef3ctq36ktl0skaa4jafbreqb8 [info] Sent 500 in 25ms
14:37:52.981 [error] #PID<0.458.0> running CrywolfWeb.Endpoint terminated
Server: localhost:4000 (http)
Request: GET /runtime_error
** (exit) an exception was raised:
    ** (RuntimeError) This is a runtime error generated by the crywolf app.
        (crywolf) lib/crywolf_web/controllers/page_controller.ex:23: CrywolfWeb.PageController.runtime_error/2
        (crywolf) lib/crywolf_web/controllers/page_controller.ex:1: CrywolfWeb.PageController.action/2
        (crywolf) lib/crywolf_web/controllers/page_controller.ex:1: CrywolfWeb.PageController.phoenix_controller_pipeline/2
        (crywolf) lib/crywolf_web/endpoint.ex:1: CrywolfWeb.Endpoint.instrument/4
        (phoenix) lib/phoenix/router.ex:278: Phoenix.Router.__call__/1
        (crywolf) lib/crywolf_web/router.ex:3: CrywolfWeb.Router.call/2
        (crywolf) lib/crywolf_web/endpoint.ex:1: CrywolfWeb.Endpoint.plug_builder_call/2
        (crywolf) lib/crywolf_web/endpoint.ex:1: CrywolfWeb.Endpoint.call/2
14:37:53.440 [error] GenServer Honeybadger.Client terminating
** (FunctionClauseError) no function clause matching in :hackney_headers_new.params_to_iolist/2
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:201: :hackney_headers_new.params_to_iolist("HONEYBADGER_API_KEY", [])
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:190: anonymous fn/3 in :hackney_headers_new.to_iolist/1
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:148: :hackney_headers_new.do_fold/3
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:184: :hackney_headers_new.to_iolist/1
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_request.erl:95: :hackney_request.perform/2
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney.erl:358: :hackney.send_request/2
    (honeybadger) lib/honeybadger/client.ex:71: Honeybadger.Client.handle_cast/2
    (stdlib) gen_server.erl:616: :gen_server.try_dispatch/4
Last message: {:"$gen_cast", {:notice, %Honeybadger.Notice{ ... }}}

...

14:37:54.528 [warn] [Honeybadger] Unable to notify, the :honeybadger client isn't running
14:37:54.528 [info] Application honeybadger exited: shutdown
{"Kernel pid terminated",application_controller,"{application_terminated,honeybadger,shutdown}"}
Kernel pid terminated (application_controller) ({application_terminated,honeybadger,shutdown})

Crash dump is being written to: erl_crash.dump...done

(note that I omitted the contents of %Honeybadger.Notice{} in the "Last message:" line for brevity.)

@joshuap

joshuap Oct 4, 2017

Member

In a test app I'm trying to configure honeybadger via the HONEYBADGER_API_KEY environment variable. In a phoneix app, the app boots up fine (it determines the environment variable is present), but when it reports an error the Client GenServer is crashing with a bunch of errors like this (eventually the application shuts down instead of looping forever, which is nice):

14:37:52.948 request_id=b5a0okef3ctq36ktl0skaa4jafbreqb8 [info] GET /runtime_error
14:37:52.973 request_id=b5a0okef3ctq36ktl0skaa4jafbreqb8 [info] Sent 500 in 25ms
14:37:52.981 [error] #PID<0.458.0> running CrywolfWeb.Endpoint terminated
Server: localhost:4000 (http)
Request: GET /runtime_error
** (exit) an exception was raised:
    ** (RuntimeError) This is a runtime error generated by the crywolf app.
        (crywolf) lib/crywolf_web/controllers/page_controller.ex:23: CrywolfWeb.PageController.runtime_error/2
        (crywolf) lib/crywolf_web/controllers/page_controller.ex:1: CrywolfWeb.PageController.action/2
        (crywolf) lib/crywolf_web/controllers/page_controller.ex:1: CrywolfWeb.PageController.phoenix_controller_pipeline/2
        (crywolf) lib/crywolf_web/endpoint.ex:1: CrywolfWeb.Endpoint.instrument/4
        (phoenix) lib/phoenix/router.ex:278: Phoenix.Router.__call__/1
        (crywolf) lib/crywolf_web/router.ex:3: CrywolfWeb.Router.call/2
        (crywolf) lib/crywolf_web/endpoint.ex:1: CrywolfWeb.Endpoint.plug_builder_call/2
        (crywolf) lib/crywolf_web/endpoint.ex:1: CrywolfWeb.Endpoint.call/2
14:37:53.440 [error] GenServer Honeybadger.Client terminating
** (FunctionClauseError) no function clause matching in :hackney_headers_new.params_to_iolist/2
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:201: :hackney_headers_new.params_to_iolist("HONEYBADGER_API_KEY", [])
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:190: anonymous fn/3 in :hackney_headers_new.to_iolist/1
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:148: :hackney_headers_new.do_fold/3
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_headers_new.erl:184: :hackney_headers_new.to_iolist/1
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney_request.erl:95: :hackney_request.perform/2
    (hackney) /home/josh/code/honeybadger-io/crywolf-elixir-new/deps/hackney/src/hackney.erl:358: :hackney.send_request/2
    (honeybadger) lib/honeybadger/client.ex:71: Honeybadger.Client.handle_cast/2
    (stdlib) gen_server.erl:616: :gen_server.try_dispatch/4
Last message: {:"$gen_cast", {:notice, %Honeybadger.Notice{ ... }}}

...

14:37:54.528 [warn] [Honeybadger] Unable to notify, the :honeybadger client isn't running
14:37:54.528 [info] Application honeybadger exited: shutdown
{"Kernel pid terminated",application_controller,"{application_terminated,honeybadger,shutdown}"}
Kernel pid terminated (application_controller) ({application_terminated,honeybadger,shutdown})

Crash dump is being written to: erl_crash.dump...done

(note that I omitted the contents of %Honeybadger.Notice{} in the "Last message:" line for brevity.)

This comment has been minimized.

@joshuap

joshuap Oct 4, 2017

Member

It looks like this may be an issue with how we're merging the environment variables. This is what the config looks like:

[api_key: {:system, "HONEYBADGER_API_KEY"}, app: nil, environment_name: {:system, "MIX_ENV"}, exclude_envs: [:dev, :test], hostname: "zoltan", origin: "https://api.honeybadger.io", proxy: nil, proxy_auth: {nil, nil}, project_root: "/home/josh/code/honeybadger-io/crywolf-elixir-new", use_logger: true, notice_filter: Honeybadger.DefaultNoticeFilter, filter: Honeybadger.DefaultFilter, filter_keys: [:password, :credit_card], filter_disable_url: false, filter_disable_params: false, filter_disable_session: false, included_applications: []]

Edit: Since the tests didn't cover this, it would be awesome if you can add a test case!

@joshuap

joshuap Oct 4, 2017

Member

It looks like this may be an issue with how we're merging the environment variables. This is what the config looks like:

[api_key: {:system, "HONEYBADGER_API_KEY"}, app: nil, environment_name: {:system, "MIX_ENV"}, exclude_envs: [:dev, :test], hostname: "zoltan", origin: "https://api.honeybadger.io", proxy: nil, proxy_auth: {nil, nil}, project_root: "/home/josh/code/honeybadger-io/crywolf-elixir-new", use_logger: true, notice_filter: Honeybadger.DefaultNoticeFilter, filter: Honeybadger.DefaultFilter, filter_keys: [:password, :credit_card], filter_disable_url: false, filter_disable_params: false, filter_disable_session: false, included_applications: []]

Edit: Since the tests didn't cover this, it would be awesome if you can add a test case!

This comment has been minimized.

@sorentwo

sorentwo Oct 5, 2017

Contributor

It was due to the use of Application.get_all_env, which didn't make any use of Honeybadger.get_env/1. I've added a new function, Honeybadger.get_all_env/0 that takes care of this automatically—and is tested.

@sorentwo

sorentwo Oct 5, 2017

Contributor

It was due to the use of Application.get_all_env, which didn't make any use of Honeybadger.get_env/1. I've added a new function, Honeybadger.get_all_env/0 that takes care of this automatically—and is tested.

sorentwo added some commits Oct 5, 2017

Fetch the response body before logging it
The value is a reference and hasn't been decoded, which makes it useless
for logging.
@sorentwo

This comment has been minimized.

Show comment
Hide comment
@sorentwo

sorentwo Oct 5, 2017

Contributor

@joshuap All of your feedback should have been addressed. Thanks for looking it over.

Contributor

sorentwo commented Oct 5, 2017

@joshuap All of your feedback should have been addressed. Thanks for looking it over.

@joshuap

@sorentwo thanks, those changes look good. I'm still getting errors when I configure Honeybadger with environment variables, though. I think the issue is that you should be calling System.get_env("VARIABLE_NAME") instead of {:system, "VARIABLE_NAME"} in default_config (see my comment).

Show outdated Hide outdated lib/honeybadger.ex
Define all static env as app defaults
To ensure app env is configured correctly, and to have the settings
available as soon as possible we put static configuration values in
the application itself. This helps simplify the startup and env merging
flow.
@sorentwo

This comment has been minimized.

Show comment
Hide comment
@sorentwo

sorentwo Oct 10, 2017

Contributor

@joshuap Alright, shuffled the config a bit and refined how the defaults are merged or preserved. It is cleaner now, and I've verified that none of the {:system, _} tuples make it through to the client:

Honeybadger.Client.new([])
%Honeybadger.Client{enabled: false,
 headers: [{"X-API-Key", "asdfghjkl"}, {"Accept", "application/json"},
  {"Content-Type", "application/json"}, {"User-Agent", "Honeybadger Elixir"}],
 proxy: nil, proxy_auth: {nil, nil},
 url: "https://api.honeybadger.io/v1/notices"}
Contributor

sorentwo commented Oct 10, 2017

@joshuap Alright, shuffled the config a bit and refined how the defaults are merged or preserved. It is cleaner now, and I've verified that none of the {:system, _} tuples make it through to the client:

Honeybadger.Client.new([])
%Honeybadger.Client{enabled: false,
 headers: [{"X-API-Key", "asdfghjkl"}, {"Accept", "application/json"},
  {"Content-Type", "application/json"}, {"User-Agent", "Honeybadger Elixir"}],
 proxy: nil, proxy_auth: {nil, nil},
 url: "https://api.honeybadger.io/v1/notices"}
@joshuap

This comment has been minimized.

Show comment
Hide comment
@joshuap

joshuap Oct 10, 2017

Member

@sorentwo thanks, this looks good to me! 👍 I'm going to merge now and release a beta.

Member

joshuap commented Oct 10, 2017

@sorentwo thanks, this looks good to me! 👍 I'm going to merge now and release a beta.

@joshuap joshuap merged commit c228c2b into honeybadger-io:master Oct 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sorentwo sorentwo deleted the sorentwo:chore/use-hackney-directly branch Oct 10, 2017

@sorentwo

This comment has been minimized.

Show comment
Hide comment
@sorentwo

sorentwo Oct 10, 2017

Contributor

@joshuap Awesome! Thanks for all the feedback and troubleshooting.

Contributor

sorentwo commented Oct 10, 2017

@joshuap Awesome! Thanks for all the feedback and troubleshooting.

@sorentwo sorentwo referenced this pull request Nov 6, 2017

Closed

Infinite errors logged #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment