CityBaseInc / airbrake_client

Airbrake client to report errors and exceptions to Airbrake.io.
Other
0 stars 1 forks source link

Better support for Jason library #13

Closed jdfrens closed 3 years ago

jdfrens commented 3 years ago

If you use jason instead of poison for JSON encoding, you have to derive a serialization for Airbrake.Payload. This is not enough:

Protocol.derive(Jason.Encoder, Airbrake.Payload)

This is because Airbrake.Payload defines only a few fields in its struct:

https://github.com/CityBaseInc/airbrake_client/blob/master/lib/airbrake/payload.ex#L10

However, the "enhanced options" for Airbrake.report/2 include optional fields :context, :params, :session, and :env. Since they are not part of the Airbrake.Payload struct, deriving Jason.Encoder ignores those fields, and you get anemic reports in Airbrake.io.

Here's how I verified the problem in an iex session:

iex(6)> payload = Airbrake.Payload.new([type: "TestingError", message: "Testing Airbrake.report/2"], [], context: %{
...(6)>   foo: "bar",
...(6)>   now: "America/Chicago" |> DateTime.now!() |> inspect()
...(6)> })
%{
  __struct__: Airbrake.Payload,
  apiKey: nil,
  context: %{
    environment: "dev",
    foo: "bar",
    hostname: "000045",
    now: "#DateTime<2021-06-04 11:43:51.448695-05:00 CDT America/Chicago>"
  },
  errors: [
    %{backtrace: [], message: "Testing Airbrake.report/2", type: "TestingError"}
  ],
  notifier: %{
    name: "Airbrake Client",
    url: "https://github.com/CityBaseInc/airbrake_client",
    version: "0.8.2"
  }
}
iex(8)> Jason.encode!(payload) |> Jason.decode()
{:ok,
 %{
   "apiKey" => nil,
   "errors" => [
     %{
       "backtrace" => [],
       "message" => "Testing Airbrake.report/2",
       "type" => "TestingError"
     }
   ],
   "notifier" => %{
     "name" => "Airbrake Client",
     "url" => "https://github.com/CityBaseInc/airbrake_client",
     "version" => "0.8.2"
   }
 }}

Adding these extra fields to the struct seems like a good idea, but I suspect then the fields will always be sent to Airbrake.io, set to nil if a field hasn't been assigned a value. This seems like it could lead to a lot of noise in the reports. So just adding the fields to the struct will not be sufficient. (This might be a very easy problem to solve with options provided by jason and poison.)

Somewhat separately, it would be great if airbrake_client derived an encoding for Airbrake.Payload itself. This was a good idea to begin with (rather than forcing anyone using the library to come up with their own encoded). But if the encoding is more involved than just calling Protocol.derive/2, then for easy-of-use this library must define the encoding, and it might as well be done fixing the missing struct fields.

jdfrens commented 3 years ago

I was able to confirm that the missing fields from the Airbrake.Payload struct is the problem when deriving the encoders. It was easiest for me to tweak the encoder, but I suspect the better solution is to add the fields to the struct.

I ran some experiments with an app that uses airbrake_client version 0.8.0. I ended up with this hacked encoder:

impl Jason.Encoder, for: Airbrake.Payload do
  def encode(value, opts) do
    Jason.Encode.map(Map.take(value, [:apiKey, :notifier, :errors, :context, :params, :session, :environment]), opts)
  end
end

From the iex console, I generated this report:

Airbrake.report(
  [type: "TestingError", message: "Testing Airbrake.report/2"],
  context: %{
    foo: "bar",
    now: "America/Chicago" |> DateTime.now!() |> inspect()
  },
  params: %{foo: 6},
  session: %{foo: 9},
  env: %{foo: 10}
)

Now I see the extended options in Airbrake.io:

Screen Shot 2021-06-04 at 2 12 39 PM