feat: Add database tracking and report for Push Reliability #769

jrconlin · 2024-09-25T23:30:27Z

This PR introduces tracking throughput for the database.

It also introduces the PushReliability reporting skeleton. This will be
fleshed out with full reporting later.

This PR introduces tracking throughput for the database. It also introduces the PushReliability reporting skeleton. This will be fleshed out with full reporting later. Closes: #SYNC-4324

… feat/SYNC-4324_track_db

jrconlin · 2024-09-25T23:30:48Z

autoconnect/autoconnect-web/Cargo.toml

@@ -34,3 +34,5 @@ ctor.workspace = true
 tokio.workspace = true

 autoconnect_common = { workspace = true, features = ["test-support"] }
+


these features will be used in the next PR.

jrconlin · 2024-09-25T23:33:15Z

autoendpoint/src/settings.rs

@@ -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(),


Modified to check for padding stripping.

Should this one have the "==" added to end as well, with exclusion of vI?

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.

* This presumes that there's a redis like storage system that can record the state transitions of messages. * function is hidden behind `reliable_report` feature flag * Requires #769 Closes: SYNC-4419

taddes

👍 thanks for the work on this. Some thoughts on adjustments

taddes · 2024-09-26T17:35:44Z

autoendpoint/src/routers/common.rs

@@ -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: {:?}",


For clarity, I'd update this to

Suggested change

"🔍 Sending Reliability: {:?}",

"🔍 Sending Reliability ID: {:?}",

and perhaps the other instances this is logged

taddes · 2024-09-26T17:36:07Z

autoendpoint/src/extractors/subscription.rs

+                    .is_trackable(&v)
+                    .then(|| app_state.vapid_tracker.get_id(req.headers()))
+            });
+            debug!("🔍 Assigning Reliability: {:?}", reliability_id);


Suggested change

debug!("🔍 Assigning Reliability: {:?}", reliability_id);

debug!("🔍 Assigning Reliability ID: {:?}", reliability_id);

taddes · 2024-09-26T17:50:04Z

autoendpoint/src/extractors/notification.rs

+        if let Some(reliability_id) = &self.subscription.reliability_id {
+            map.insert(
+                "reliability_id",
+                serde_json::to_value(reliability_id).unwrap(),


I see the signature returns HashMap<&'static str, serde_json::Value> regardless of success/fail with the .unwrap() calls. Did we want to simplify this with the use of ? in a few places? Here's my thought:

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 .unwrap()

Fair. I didn't want to modify a bunch of extra things, but I think you're right. I'll update ApiErrorKind

taddes · 2024-09-26T17:51:01Z

autoconnect/autoconnect-common/src/protocol.rs

@@ -85,6 +85,8 @@ pub struct ClientAck {
    #[serde(rename = "channelID")]
    pub channel_id: Uuid,
    pub version: String,
+    #[serde(default)]


Perhaps a comment - and potentially comments for channel_id and version, even if it's self evident - to describe purpose of the reliability_id

taddes · 2024-10-01T18:42:58Z

autoendpoint/src/settings.rs

+        let reply = Self::read_list_from_str(keys, "Invalid AUTOEND_TRACKING_KEYS")
+            .map(|v| v.to_owned().replace("=", ""))
+            .collect();
+        trace!("🔍 keys: {:?}", reply);
+        reply


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

let reply = Self::read_list_from_str(keys, "Invalid AUTOEND_TRACKING_KEYS")

.map(|v| v.to_owned().replace("=", ""))

.collect();

trace!("🔍 keys: {:?}", reply);

reply

let result = Self::read_list_from_str(keys, "Invalid AUTOEND_TRACKING_KEYS")

.map(|v| v.to_owned().replace("=", ""))

.collect();

trace!("🔍 tracking_keys: {:?}", reply);

result

taddes · 2024-10-01T18:47:41Z

autoendpoint/src/settings.rs

@@ -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(),


Should this one have the "==" added to end as well, with exclusion of vI?

taddes · 2024-10-01T18:53:03Z

tests/integration/test_integration_all_rust.py

+    """Test delivery of a basic push message with a VAPID header."""
+    uuid_data: str = str(uuid.uuid4())
+    vapid_info = _get_vapid(key=TRACKING_KEY, payload=vapid_payload)
+    # quick sanity check to ensure that the keys match.


thanks for adding the useful comments here

taddes · 2024-10-01T18:55:18Z

tests/integration/test_integration_all_rust.py

+    log.debug(f"🔍 reliability {result["reliability_id"]}")
+    assert result["reliability_id"] is not None


Minor nit: switch order so after assertions and match debug string to reliability_id for clarity.

Suggested change

log.debug(f"🔍 reliability {result["reliability_id"]}")

assert result["reliability_id"] is not None

assert result["reliability_id"] is not None

log.debug(f"🔍 reliability_id {result["reliability_id"]}")

taddes · 2024-10-01T19:01:46Z

tests/integration/test_integration_all_rust.py

+    clean_header = registered_test_client._crypto_key.replace('"', "").rstrip("=")
+    assert result["headers"]["encryption"] == clean_header
+    assert result["data"] == base64url_encode(uuid_data)
+    assert result["messageType"] == "notification"


We have an enum of ClientMessageType in the async_push_test_client.py which you could add this as a variant, should you wish. Can alter this to simply be MessageType as well, since the "client" does double duty in these tests and isn't strictly the client...

taddes · 2024-10-01T19:10:21Z

tests/integration/test_integration_all_rust.py

+    tracking_keys="[{}]".format(
+        base64.urlsafe_b64encode((b"\4" + TRACKING_PUB_KEY.to_string())).decode()
+    ),


Suggested change

tracking_keys="[{}]".format(

base64.urlsafe_b64encode((b"\4" + TRACKING_PUB_KEY.to_string())).decode()

),

tracking_keys=f"[{base64.urlsafe_b64encode(b'\4' + TRACKING_PUB_KEY.to_string()).decode()}]",

… feat/SYNC-4324_track_db

jrconlin added 4 commits September 25, 2024 15:34

feat: Add database tracking and report for Push Reliability

a8b64b7

This PR introduces tracking throughput for the database. It also introduces the PushReliability reporting skeleton. This will be fleshed out with full reporting later. Closes: #SYNC-4324

Merge branch 'master' of github.com:mozilla-services/autopush-rs into…

dc1763a

… feat/SYNC-4324_track_db

f lint

14bd4fb

f remove extra from pending pr

e914b14

jrconlin commented Sep 25, 2024

View reviewed changes

jrconlin requested review from pjenvey and taddes September 25, 2024 23:33

jrconlin changed the title ~~Feat/sync 4324 track db~~ feat: Add database tracking and report for Push Reliability Sep 27, 2024

taddes requested changes Oct 1, 2024

View reviewed changes

jrconlin added 4 commits October 2, 2024 10:13

Merge branch 'master' of github.com:mozilla-services/autopush-rs into…

4f25172

… feat/SYNC-4324_track_db

f r's

e9ff9f5

f r's

c9a3512

f lint

f8c7ee9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add database tracking and report for Push Reliability #769

feat: Add database tracking and report for Push Reliability #769

jrconlin commented Sep 25, 2024 •

edited by jira bot

Loading

jrconlin Sep 25, 2024

jrconlin Sep 25, 2024

taddes Oct 1, 2024 •

edited

Loading

jrconlin Oct 2, 2024

taddes left a comment

taddes Sep 26, 2024

taddes Sep 26, 2024

taddes Sep 26, 2024

jrconlin Oct 2, 2024

taddes Sep 26, 2024

taddes Oct 1, 2024

taddes Oct 1, 2024 •

edited

Loading

taddes Oct 1, 2024

taddes Oct 1, 2024

taddes Oct 1, 2024

taddes Oct 1, 2024

		@@ -34,3 +34,5 @@ ctor.workspace = true
		tokio.workspace = true

		autoconnect_common = { workspace = true, features = ["test-support"] }

	"🔍 Sending Reliability: {:?}",
	"🔍 Sending Reliability ID: {:?}",

	debug!("🔍 Assigning Reliability: {:?}", reliability_id);
	debug!("🔍 Assigning Reliability ID: {:?}", reliability_id);

		log.debug(f"🔍 reliability {result["reliability_id"]}")
		assert result["reliability_id"] is not None

feat: Add database tracking and report for Push Reliability #769

Are you sure you want to change the base?

feat: Add database tracking and report for Push Reliability #769

Conversation

jrconlin commented Sep 25, 2024 • edited by jira bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taddes Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taddes left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taddes Oct 1, 2024 • 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

jrconlin commented Sep 25, 2024 •

edited by jira bot

Loading

taddes Oct 1, 2024 •

edited

Loading

taddes Oct 1, 2024 •

edited

Loading