chore(queues): use mozlog and failure in queues process #161

brizental · 2018-08-08T06:52:44Z

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.

r? @philbooth @vladikoff

philbooth · 2018-08-08T07:28:58Z

src/app_errors/mod.rs

-            fields.insert(String::from("errno"), Value::String(format!("{}", errno)));
+            fields.insert(
+                String::from("errno"),
+                Value::Number(errno.to_owned().into()),


Ref: mozilla/fxa-auth-server#2569

philbooth · 2018-08-08T07:36:03Z

src/app_errors/mod.rs

+    QueueError(String),
+    /// An error for when we get an invalid notification type in the queues
+    /// process.
+    #[fail(display = "Invalid notification type.")]


Tiny nit: I'm not a fan of including a terminating period in our error strings fwiw. None of our other repos do it and it may be incorrect in the context of the receiving code, if it wants to display the message as part of a broader error string.

philbooth · 2018-08-08T07:43:55Z

src/logging/mod.rs

@@ -59,6 +60,46 @@ impl KV for RequestMozlogFields {
    }
 }

+#[derive(Clone)]
+struct AppErrorFields {
+    code: String,


Doesn't really matter tbh, it's only logging, but this one could/should be an i32 as well, right?

(actually u16 is probably more correct for both if I'm feeling pedantic, which I usually am)

philbooth · 2018-08-08T07:57:18Z

src/queues/sqs/notification/mod.rs

    headers: Option<Vec<Header>>,
    #[serde(rename = "commonHeaders")]
-    common_headers: Option<Vec<Header>>,
+    common_headers: Option<HashMap<String, HeaderValue>>,


~~Should the Header struct circa line 160 be deleted too?~~

Oh, wait, no we shouldn't, it's still used for the headers property on the line above.

(apologies for my poor reading comprehension! 😄)

I thought about writing a deserializing function for the common_headers so that we can keep it as a Option<Vec<Header>>, it makes sense to me that both headers are the same type... But then I thought that maybe we might want the struct to mimic exactly the message we get. What do you think, @philbooth ?

I reckon keep it like you've got here, it's probably less confusing if we match the SES format as closely as possible.

philbooth · 2018-08-08T08:17:43Z

src/queues/sqs/notification/mod.rs

@@ -137,10 +140,10 @@ pub struct Mail {
    sending_account_id: Option<String>,
    destination: Option<Vec<String>>,
    #[serde(rename = "headersTruncated")]
-    headers_truncated: Option<String>,
+    headers_truncated: Option<bool>,


philbooth · 2018-08-08T09:53:57Z

src/app_errors/mod.rs

+                    .map_err(|_| Status::InternalServerError)?;
+                slog_error!(log, "{}", "Request errored.");
+            }
+            _ => println!("Internal error: No managed MozlogLogger"),


~~How come this isn't a slog_error!?~~

Duh, because haven't got a logger you div.

In that case, should we panic here? If logging isn't working we probably want to abort don't we?

Yes, panicking sounds reasonable.

philbooth

This looks good! There's a couple of tiny nits in the inline comments, but I'm cool with you ignoring them if you disagree or whatever.

philbooth · 2018-08-08T18:04:50Z

I'm happy, we can merge this as soon as the conflict is resolved!

- Also fix formatting errors.

brizental · 2018-08-08T19:31:22Z

Would you like me to squash these commits before merging also, @philbooth ?

brizental force-pushed the brizental/queue-logs-and-errors branch 2 times, most recently from 556e7e1 to 880455d Compare August 8, 2018 07:10

philbooth reviewed Aug 8, 2018

View reviewed changes

philbooth mentioned this pull request Aug 8, 2018

Test message-parsing in the queues process #162

Closed

philbooth reviewed Aug 8, 2018

View reviewed changes

philbooth approved these changes Aug 8, 2018

View reviewed changes

brizental force-pushed the brizental/queue-logs-and-errors branch from cb02bb3 to 0da0f5b Compare August 8, 2018 17:14

brizental added 7 commits August 8, 2018 11:23

chore(errors): use failure crate for queue errors

38816c9

refactor(logging): create custom logger for AppError

0a5aeb1

feat(logging): use mozlog logger for queues process

502c872

fix(queues): get queues to parse sqs messages properly

e665a81

- Also fix formatting errors.

chore(errors): remove terminating period from error strings

b9688b1

chore(errors): change errno and code fields to u16

205e66e

chore(logging): panic! if no mozlogger is found in managed state

79e204e

brizental force-pushed the brizental/queue-logs-and-errors branch from 0da0f5b to 79e204e Compare August 8, 2018 18:25

brizental added 2 commits August 8, 2018 11:34

fix(queues): fixes for updated rusoto version

2453445

chore(formatting): fix formatting errors for new fmt version

23c56f4

vladikoff merged commit 501a591 into mozilla:master Aug 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(queues): use mozlog and failure in queues process #161

chore(queues): use mozlog and failure in queues process #161

brizental commented Aug 8, 2018

philbooth Aug 8, 2018

philbooth Aug 8, 2018

philbooth Aug 8, 2018

philbooth Aug 8, 2018

philbooth Aug 8, 2018 •

edited

Loading

philbooth Aug 8, 2018

brizental Aug 8, 2018

philbooth Aug 8, 2018

philbooth Aug 8, 2018

philbooth Aug 8, 2018 •

edited

Loading

philbooth Aug 8, 2018

brizental Aug 8, 2018

philbooth left a comment

philbooth commented Aug 8, 2018

brizental commented Aug 8, 2018

chore(queues): use mozlog and failure in queues process #161

chore(queues): use mozlog and failure in queues process #161

Conversation

brizental commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbooth left a comment

Choose a reason for hiding this comment

philbooth commented Aug 8, 2018

brizental commented Aug 8, 2018

philbooth Aug 8, 2018 •

edited

Loading

philbooth Aug 8, 2018 •

edited

Loading