-
-
Notifications
You must be signed in to change notification settings - Fork 207
Elixir 1.7/OTP 21 #302
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
Elixir 1.7/OTP 21 #302
Conversation
lib/sentry/client.ex
Outdated
| defp encode_and_send(event, result) do | ||
| render_event(event) | ||
| |> Poison.encode() | ||
| |> Jason.encode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make this configurable a-la ecto/phoenix. "json_library"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would take some more thought as it complicates the installation. If an app adds Sentry, and :jason is the default (but still an optional dep), and they don't have Jason as a dependency or another library configured, it'll break Sentry. Phoenix solves this with the generation of a project and including a default dependency in mix.exs.
We'd have to do something like: https://github.com/phoenixframework/phoenix/blob/18871c898fee3a91034cd23a4b86444449daa991/lib/phoenix.ex#L79-L90 as well
It also seems semi-scary that we can't enforce that the JSON library implements a protocol or interface we expect? I don't see how Ecto/Phoenix catch that a bad configuration like :sentry, json_library: :ranch wouldn't work, and you don't find out until runtime?
I dunno how I feel for cost/benefit on it.
Created #303 to track this
5286bbe to
92eb3bb
Compare
This PR will bundle significant changes, including:
Sentry.Logger:error_loggerbackend in favor ofSentry.LoggerBackendwhich leverages the new changes in Elixir 1.7 and OTP 21