-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Add database tracking and report for Push Reliability #769
base: master
Are you sure you want to change the base?
Changes from 4 commits
a8b64b7
dc1763a
14bd4fb
e914b14
4f25172
e9ff9f5
c9a3512
f8c7ee9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,3 +34,5 @@ ctor.workspace = true | |
tokio.workspace = true | ||
|
||
autoconnect_common = { workspace = true, features = ["test-support"] } | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these features will be used in the next PR. |
||
[features] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,6 +103,7 @@ impl From<Notification> for autopush_common::notification::Notification { | |
timestamp: notification.timestamp, | ||
data: notification.data, | ||
sortkey_timestamp, | ||
reliability_id: notification.subscription.reliability_id, | ||
headers: { | ||
let headers: HashMap<String, String> = notification.headers.into(); | ||
if headers.is_empty() { | ||
|
@@ -171,6 +172,12 @@ impl Notification { | |
map.insert("ttl", serde_json::to_value(self.headers.ttl).unwrap()); | ||
map.insert("topic", serde_json::to_value(&self.headers.topic).unwrap()); | ||
map.insert("timestamp", serde_json::to_value(self.timestamp).unwrap()); | ||
if let Some(reliability_id) = &self.subscription.reliability_id { | ||
map.insert( | ||
"reliability_id", | ||
serde_json::to_value(reliability_id).unwrap(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the signature returns Eg. /// Serialize the notification for delivery to the connection server.
pub fn serialize_for_delivery(&self) -> Result<HashMap<&'static str, serde_json::Value>, serde_json::Error> {
let mut map = HashMap::new();
map.insert(
"channelID",
serde_json::to_value(self.subscription.channel_id)?
);
map.insert("version", serde_json::to_value(&self.message_id)?);
map.insert("ttl", serde_json::to_value(self.headers.ttl)?);
map.insert("topic", serde_json::to_value(&self.headers.topic)?);
map.insert("timestamp", serde_json::to_value(self.timestamp)?);
if let Some(reliability_id) = &self.subscription.reliability_id {
map.insert("reliability_id", serde_json::to_value(reliability_id)?);
}
if let Some(data) = &self.data {
map.insert("data", serde_json::to_value(data)?);
let headers: HashMap<_, _> = self.headers.clone().into();
map.insert("headers", serde_json::to_value(headers)?);
}
Ok(map)
} Therefore the single call to this function in autoendpoint/src/routers/webpush.rs has a surrounding context to returning a Result type, so it'd probably be minimal adjustment with just calling a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair. I didn't want to modify a bunch of extra things, but I think you're right. I'll update |
||
); | ||
} | ||
|
||
if let Some(data) = &self.data { | ||
map.insert("data", serde_json::to_value(data).unwrap()); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -40,7 +40,7 @@ pub struct Subscription { | |||||
/// (This should ONLY be applied for messages that match known | ||||||
/// Mozilla provided VAPID public keys.) | ||||||
/// | ||||||
pub tracking_id: Option<String>, | ||||||
pub reliability_id: Option<String>, | ||||||
} | ||||||
|
||||||
impl FromRequest for Subscription { | ||||||
|
@@ -75,11 +75,13 @@ impl FromRequest for Subscription { | |||||
.transpose()?; | ||||||
|
||||||
trace!("raw vapid: {:?}", &vapid); | ||||||
let trackable = if let Some(vapid) = &vapid { | ||||||
app_state.reliability.is_trackable(vapid) | ||||||
} else { | ||||||
false | ||||||
}; | ||||||
let reliability_id: Option<String> = vapid.clone().and_then(|v| { | ||||||
app_state | ||||||
.vapid_tracker | ||||||
.is_trackable(&v) | ||||||
.then(|| app_state.vapid_tracker.get_id(req.headers())) | ||||||
}); | ||||||
debug!("🔍 Assigning Reliability: {:?}", reliability_id); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
// Capturing the vapid sub right now will cause too much cardinality. Instead, | ||||||
// let's just capture if we have a valid VAPID, as well as what sort of bad sub | ||||||
|
@@ -134,14 +136,11 @@ impl FromRequest for Subscription { | |||||
.incr(&format!("updates.vapid.draft{:02}", vapid.vapid.version()))?; | ||||||
} | ||||||
|
||||||
let tracking_id = | ||||||
trackable.then(|| app_state.reliability.get_tracking_id(req.headers())); | ||||||
|
||||||
Ok(Subscription { | ||||||
user, | ||||||
channel_id, | ||||||
vapid, | ||||||
tracking_id, | ||||||
reliability_id, | ||||||
}) | ||||||
} | ||||||
.boxed_local() | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,6 +21,12 @@ pub fn build_message_data(notification: &Notification) -> ApiResult<HashMap<&'st | |||||
message_data.insert_opt("enc", notification.headers.encryption.as_ref()); | ||||||
message_data.insert_opt("cryptokey", notification.headers.crypto_key.as_ref()); | ||||||
message_data.insert_opt("enckey", notification.headers.encryption_key.as_ref()); | ||||||
// Report the data to the UA. How this value is reported back is still a work in progress. | ||||||
trace!( | ||||||
"🔍 Sending Reliability: {:?}", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity, I'd update this to
Suggested change
and perhaps the other instances this is logged |
||||||
notification.subscription.reliability_id | ||||||
); | ||||||
message_data.insert_opt("rid", notification.subscription.reliability_id.as_ref()); | ||||||
} | ||||||
|
||||||
Ok(message_data) | ||||||
|
@@ -239,7 +245,7 @@ pub mod tests { | |||||
user, | ||||||
channel_id: channel_id(), | ||||||
vapid: None, | ||||||
tracking_id: None, | ||||||
reliability_id: None, | ||||||
}, | ||||||
headers: NotificationHeaders { | ||||||
ttl: 0, | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -175,9 +175,11 @@ impl Settings { | |||||||||||||||||||||
// public key, but that may not always be true. | ||||||||||||||||||||||
pub fn tracking_keys(&self) -> Vec<String> { | ||||||||||||||||||||||
let keys = &self.tracking_keys.replace(['"', ' '], ""); | ||||||||||||||||||||||
Self::read_list_from_str(keys, "Invalid AUTOEND_TRACKING_KEYS") | ||||||||||||||||||||||
.map(|v| v.to_owned()) | ||||||||||||||||||||||
.collect() | ||||||||||||||||||||||
let reply = Self::read_list_from_str(keys, "Invalid AUTOEND_TRACKING_KEYS") | ||||||||||||||||||||||
.map(|v| v.to_owned().replace("=", "")) | ||||||||||||||||||||||
.collect(); | ||||||||||||||||||||||
trace!("🔍 keys: {:?}", reply); | ||||||||||||||||||||||
reply | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor nit, but thought result is better than reply since it's not calling out to anywhere? And making the trace output linked be a little more explicit, given keys might be a bit vague.
Suggested change
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Get the URL for this endpoint server | ||||||||||||||||||||||
|
@@ -199,11 +201,20 @@ impl VapidTracker { | |||||||||||||||||||||
pub fn is_trackable(&self, vapid: &VapidHeaderWithKey) -> bool { | ||||||||||||||||||||||
// ideally, [Settings.with_env_and_config_file()] does the work of pre-populating | ||||||||||||||||||||||
// the Settings.tracking_vapid_pubs cache, but we can't rely on that. | ||||||||||||||||||||||
self.0.contains(&vapid.public_key) | ||||||||||||||||||||||
let key = vapid.public_key.replace('=', ""); | ||||||||||||||||||||||
let result = self.0.contains(&key); | ||||||||||||||||||||||
debug!("🔍 Checking {key} {}", { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super useful, good idea 👍 |
||||||||||||||||||||||
if result { | ||||||||||||||||||||||
"Match!" | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
"no match" | ||||||||||||||||||||||
} | ||||||||||||||||||||||
}); | ||||||||||||||||||||||
result | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
/// Extract the message Id from the headers (if present), otherwise just make one up. | ||||||||||||||||||||||
pub fn get_tracking_id(&self, headers: &HeaderMap) -> String { | ||||||||||||||||||||||
pub fn get_id(&self, headers: &HeaderMap) -> String { | ||||||||||||||||||||||
headers | ||||||||||||||||||||||
.get("X-MessageId") | ||||||||||||||||||||||
.and_then(|v| | ||||||||||||||||||||||
|
@@ -310,7 +321,7 @@ mod tests { | |||||||||||||||||||||
#[test] | ||||||||||||||||||||||
fn test_tracking_keys() -> ApiResult<()> { | ||||||||||||||||||||||
let settings = Settings{ | ||||||||||||||||||||||
tracking_keys: r#"["BLMymkOqvT6OZ1o9etCqV4jGPkvOXNz5FdBjsAR9zR5oeCV1x5CBKuSLTlHon-H_boHTzMtMoNHsAGDlDB6X7vI"]"#.to_owned(), | ||||||||||||||||||||||
tracking_keys: r#"["BLMymkOqvT6OZ1o9etCqV4jGPkvOXNz5FdBjsAR9zR5oeCV1x5CBKuSLTlHon-H_boHTzMtMoNHsAGDlDB6X7"]"#.to_owned(), | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modified to check for padding stripping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this one have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we were decoding the key from base64, yes. We're currently not doing that so just tweaking the string values to ensure that they match (with and without the padding) should be fine. If we ever decide to decode these strings and do a byte comparison of the decoded pairs, then we would have to revisit this test, but that's out of scope for this PR. |
||||||||||||||||||||||
..Default::default() | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -320,7 +331,7 @@ mod tests { | |||||||||||||||||||||
token: "".to_owned(), | ||||||||||||||||||||||
version_data: crate::headers::vapid::VapidVersionData::Version1, | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
public_key: "BLMymkOqvT6OZ1o9etCqV4jGPkvOXNz5FdBjsAR9zR5oeCV1x5CBKuSLTlHon-H_boHTzMtMoNHsAGDlDB6X7vI".to_owned() | ||||||||||||||||||||||
public_key: "BLMymkOqvT6OZ1o9etCqV4jGPkvOXNz5FdBjsAR9zR5oeCV1x5CBKuSLTlHon-H_boHTzMtMoNHsAGDlDB6X7==".to_owned() | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
let key_set = settings.tracking_keys(); | ||||||||||||||||||||||
|
@@ -333,20 +344,20 @@ mod tests { | |||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
#[test] | ||||||||||||||||||||||
fn test_tracking_id() -> ApiResult<()> { | ||||||||||||||||||||||
fn test_reliability_id() -> ApiResult<()> { | ||||||||||||||||||||||
let mut headers = HeaderMap::new(); | ||||||||||||||||||||||
let keys = Vec::new(); | ||||||||||||||||||||||
let reliability = VapidTracker(keys); | ||||||||||||||||||||||
|
||||||||||||||||||||||
let key = reliability.get_tracking_id(&headers); | ||||||||||||||||||||||
let key = reliability.get_id(&headers); | ||||||||||||||||||||||
assert!(!key.is_empty()); | ||||||||||||||||||||||
|
||||||||||||||||||||||
headers.insert( | ||||||||||||||||||||||
HeaderName::from_lowercase(b"x-messageid").unwrap(), | ||||||||||||||||||||||
HeaderValue::from_static("123foobar456"), | ||||||||||||||||||||||
); | ||||||||||||||||||||||
|
||||||||||||||||||||||
let key = reliability.get_tracking_id(&headers); | ||||||||||||||||||||||
let key = reliability.get_id(&headers); | ||||||||||||||||||||||
assert_eq!(key, "123foobar456".to_owned()); | ||||||||||||||||||||||
|
||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||
|
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.
Perhaps a comment - and potentially comments for channel_id and version, even if it's self evident - to describe purpose of the
reliability_id