Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Improve logging and error messages #116

Closed
brizental opened this issue Jul 3, 2018 · 1 comment
Closed

Improve logging and error messages #116

brizental opened this issue Jul 3, 2018 · 1 comment

Comments

@brizental
Copy link
Contributor

brizental commented Jul 3, 2018

This needs a little bit more digging, but there are some log/error messages that could be improved.

Like, if we try and send in a request with missing params, it will return the following log:

{"Logger":"fxa_email_service-1.115.0","Type":"fxa_email_service:log","Pid":91132,"Severity":3,"Timestamp":1530658150217212000,"Fields":{"msg":"{\"code\":\"400\",\"errno\":\"101\",\"error\":\"Bad Request\",\"message\":\"Missing email params.\"}","agent":"curl/7.54.0","remoteAddressChain":"127.0.0.1","path":"/send","method":"POST"}}

Here, it would be nicer to know, for example, which are the missing params. In auth server we just add more properties to some of our error messages when they need more info (https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format), that could be a solution here.

Anyways, probably not something urgent, but I just noticed this and thought would be best to document it.

@philbooth
Copy link
Contributor

Ah, yeah, there used to be a TODO comment about this but I can't find it now, so thanks for raising this!

Anyway, for the validation errors, I think it depends on a resolution to rwf2/Rocket#596 happening first.

vladikoff pushed a commit that referenced this issue Aug 9, 2018
…dikoff,@philbooth

Fixes #10
Fixes #99
Connects to #116

This was my first experience messing with the queues part of the service, I thought dealing with these issues first would make the experience smoother. I added the standard logging and error handling with the failure crate just like the rest of the lib.

Since our errors were very much centered around requests and rocket, we have some fields that don't really make sense for the queues process, like the http code, so let me know what you think about that and if the queues process should have a different error format. I also did another minor change to the errors: now the response JSON will return a number for code and errno, instead of a string.

For logging, I implemented an AppErrorFields so that we get better structured logs for our AppErrors. I used slog_scope to make it easier to have a global logger for the queues process, let me know what you think about that... In the README.md for that crate, they have some warnings about it not being the best idea all the time, but anyways, I thought it was neat and worked well for our case.

Finally, when I started working in the queues they were not really working due to parsing errors, you will see that I changed a little bit the SQS Notification struct, that was for parsing to work, also I created a notification-dev queue in the AWS Console, because it didn't exist yet, I think everything is working fine now.
@philbooth philbooth self-assigned this Nov 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants