-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fxa fixes #321
Conversation
* Added more verbose `trace!` and `debug!` logging messages. * ignore padding errors for VAPID keys * bumped up default max bytes to handle base64 encoded 4096 block
We can't add the key to metrics (cardinality). We can log it locally and monitor a given server for a short period, though.
// The error (e.g. VapidErrorKind::InvalidKey(String)) might be too cardinal, | ||
// but we may need that information to debug a production issue. We can | ||
// add an info here, temporarily turn on info level debugging on a given server, | ||
// capture it, and then turn it off before we run out of money. |
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.
We really need to just add extras support, logged: https://mozilla-hub.atlassian.net/browse/CONSVC-1886
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.
Looks good but maybe the padding case could be a separate test case
@@ -292,7 +306,8 @@ mod tests { | |||
base64::STANDARD, | |||
) | |||
.unwrap(); | |||
let public_key = "BM3bVjW_wuZC54alIbqjTbaBNtthriVtdZlchOyOSdbVYeYQu2i5inJdft7jUWIAy4O9xHBbY196Gf-1odb8hds".to_owned(); | |||
// Specify a potentially invalid padding. |
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.
Might be worth it to make this a separate test case
No description provided.