CityBaseInc / airbrake_client

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

Issue 13 :: derive Jason.Encoder #16

Closed jdfrens closed 3 years ago

jdfrens commented 3 years ago

This PR fixes the Airbrake.Payload struct and derives Jason.Encoder for Airbrake.Payload`. Closes #13.

This Problem

Airbrake.io accepts a lot of fields in the payload for an error report. However, Airbrake.Payload defined only three of them:

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

Airbrake.io also accepts "context", "params", "session", and "environment" in the payload. Airbrake.Payload adds values for those fields anyway, and poison was more than happy to encode them. jason is more precise, and deriving the Jason.Encoder for Airbrake.Payload left those non-struct fields out.

Leaving out those fields also made it inevitable to get "field not defined on Airbrake.Payload" errors with recent versions of Elixir.

The Fixes

This PR adds the missing fields to the Airbrake.Payload struct.

The only drawback is that if a field is not given a value, it is still included in the encoding, set to null. Airbrake.io seems to ignore those fields, so null is not a problem for them. It consumes a little more bandwidth, but not that much. (And devs should be encouraged to use all of the fields anyway.)

I greatly improved the testing around Airbrake.Payload. I tried to keep each commit to one theme, and refactoring commits should have few (if any) test changes.

Manual Tesing

I ran some tests, reporting "errors" into Airbrake.io. You can see the errors I generated from my laptop (named 000045):

More Testing

I added an integration_test_apps to make sure that the encodings work even when the app depends on just one app. In particular, I wanted to make sure that deriving of Jason.Encoder happened only when jason was a dependency.

I don't expect these apps to fail, so they're probably not important to run locally all the time, but I did include them in the CI tests just in case.