diff --git a/api/tests/views_tests.py b/api/tests/views_tests.py index 7444b8e27..c834d82c8 100644 --- a/api/tests/views_tests.py +++ b/api/tests/views_tests.py @@ -97,14 +97,12 @@ def test_post_domainaddress_success( assert ret_data["full_address"].startswith("my-new-mask@premium.") assert (event := get_glean_event(caplog)) is not None - address = premium_user.domainaddress_set.get() expected_event = create_expected_glean_event( category="email_mask", name="created", user=premium_user, extra_items={ "n_domain_masks": "1", - "mask_id": address.metrics_id, "is_random_mask": "false", "created_by_api": "true", "has_website": "false", @@ -446,7 +444,6 @@ def test_patch_domainaddress( user=premium_user, extra_items={ "n_domain_masks": "1", - "mask_id": existing.metrics_id, "is_random_mask": "false", }, event_time=event["timestamp"], @@ -605,7 +602,6 @@ def test_delete_domainaddress( prem_api_client: APIClient, premium_user: User, caplog: pytest.LogCaptureFixture ) -> None: existing = DomainAddress.objects.create(user=premium_user, address="my-doomed-mask") - existing_mask_id = existing.metrics_id url = reverse("domainaddress-detail", args=[existing.id]) response = prem_api_client.delete(url) assert response.status_code == 204 @@ -618,7 +614,6 @@ def test_delete_domainaddress( user=premium_user, extra_items={ "n_deleted_domain_masks": "1", - "mask_id": existing_mask_id, "is_random_mask": "false", }, event_time=event["timestamp"], @@ -639,14 +634,12 @@ def test_post_relayaddress_success( assert ret_data["enabled"] assert (event := get_glean_event(caplog)) is not None - address = free_user.relayaddress_set.get() expected_event = create_expected_glean_event( category="email_mask", name="created", user=free_user, extra_items={ "n_random_masks": "1", - "mask_id": address.metrics_id, "is_random_mask": "true", "created_by_api": "true", "has_website": "false", @@ -677,7 +670,6 @@ def test_post_relayaddress_with_generated_for_success( "n_random_masks": "1", "has_extension": "true", "date_got_extension": str(int(address.created_at.timestamp())), - "mask_id": address.metrics_id, "is_random_mask": "true", "created_by_api": "true", "has_website": "true", @@ -773,7 +765,6 @@ def test_patch_relayaddress( user=free_user, extra_items={ "n_random_masks": "1", - "mask_id": existing.metrics_id, "is_random_mask": "true", }, event_time=event["timestamp"], @@ -933,7 +924,6 @@ def test_delete_randomaddress( free_api_client: APIClient, free_user: User, caplog: pytest.LogCaptureFixture ) -> None: existing = RelayAddress.objects.create(user=free_user) - existing_mask_id = existing.metrics_id url = reverse("relayaddress-detail", args=[existing.id]) response = free_api_client.delete(url) assert response.status_code == 204 @@ -946,7 +936,6 @@ def test_delete_randomaddress( user=free_user, extra_items={ "n_deleted_random_masks": "1", - "mask_id": existing_mask_id, "is_random_mask": "true", }, event_time=event["timestamp"], diff --git a/api/views/__init__.py b/api/views/__init__.py index 4a1532e9e..744f4271b 100644 --- a/api/views/__init__.py +++ b/api/views/__init__.py @@ -140,14 +140,12 @@ class AddressViewSet(Generic[_Address], SaveToRequestUser, viewsets.ModelViewSet ) def perform_destroy(self, instance: _Address) -> None: - mask_id = instance.metrics_id user = instance.user is_random_mask = isinstance(instance, RelayAddress) super().perform_destroy(instance) glean_logger().log_email_mask_deleted( request=self.request, user=user, - mask_id=mask_id, is_random_mask=is_random_mask, ) diff --git a/emails/tests/views_tests.py b/emails/tests/views_tests.py index 717502993..079ec48ec 100644 --- a/emails/tests/views_tests.py +++ b/emails/tests/views_tests.py @@ -498,7 +498,6 @@ class SNSNotificationIncomingTest(SNSNotificationTestBase): assert (event := get_glean_event(caplog)) is not None assert event["category"] == "email" assert event["name"] == "forwarded" - assert event["extra"]["mask_id"] == self.ra.metrics_id @override_settings(STATSD_ENABLED=True) def test_spamVerdict_FAIL_auto_block_doesnt_relay(self) -> None: @@ -532,7 +531,6 @@ class SNSNotificationIncomingTest(SNSNotificationTestBase): assert mask_event is not None shared_extra_items = { "n_domain_masks": "1", - "mask_id": da.metrics_id, "is_random_mask": "false", } expected_mask_event = create_expected_glean_event( @@ -792,7 +790,6 @@ class SNSNotificationRepliesTest(SNSNotificationTestBase): assert (event := get_glean_event(caplog)) is not None assert event["category"] == "email" assert event["name"] == "forwarded" - assert event["extra"]["mask_id"] == self.relay_address.metrics_id assert event["extra"]["is_reply"] == "true" self.mock_remove_message_from_s3.assert_called_once() @@ -1366,7 +1363,6 @@ class SNSNotificationValidUserEmailsInS3Test(TestCase): ) -> dict[str, Any]: extra_items = { "n_random_masks": "1", - "mask_id": self.address.metrics_id, "is_random_mask": "true", "is_reply": "true" if is_reply else "false", } @@ -1773,7 +1769,6 @@ class GetAddressTest(TestCase): extra_items={ "n_random_masks": "1", "n_domain_masks": "2", - "mask_id": address.metrics_id, "is_random_mask": "false", "has_website": "false", "created_by_api": "false", @@ -1806,7 +1801,6 @@ class GetAddressTest(TestCase): extra_items={ "n_random_masks": "1", "n_domain_masks": "2", - "mask_id": address.metrics_id, "is_random_mask": "false", "has_website": "false", "created_by_api": "false", diff --git a/privaterelay/glean/server_events.py b/privaterelay/glean/server_events.py index 0f9ad1d5a..3bda977e5 100644 --- a/privaterelay/glean/server_events.py +++ b/privaterelay/glean/server_events.py @@ -104,7 +104,6 @@ class EventsServerEventLogger: date_joined_premium: int, has_extension: bool, date_got_extension: int, - mask_id: str, is_random_mask: bool, is_reply: bool, reason: str, @@ -128,7 +127,6 @@ class EventsServerEventLogger: :param int date_joined_premium: Timestamp for starting premium_status subscription, seconds since epoch, -1 if not subscribed :param bool has_extension: The user has the Relay Add-on :param int date_got_extension: Timestamp for adding Relay Add-on, seconds since epoch, -1 if not used - :param str mask_id: Mask ID, 'R' (random mask) or 'D' (domain mask) followed by a number. :param bool is_random_mask: The mask is a random mask, instead of a domain mask :param bool is_reply: The email is a reply from the Relay user :param str reason: Code describing why the email was blocked @@ -148,7 +146,6 @@ class EventsServerEventLogger: "date_joined_premium": str(date_joined_premium), "has_extension": str(has_extension).lower(), "date_got_extension": str(date_got_extension), - "mask_id": str(mask_id), "is_random_mask": str(is_random_mask).lower(), "is_reply": str(is_reply).lower(), "reason": str(reason), @@ -171,7 +168,6 @@ class EventsServerEventLogger: date_joined_premium: int, has_extension: bool, date_got_extension: int, - mask_id: str, is_random_mask: bool, is_reply: bool, ) -> None: @@ -194,7 +190,6 @@ class EventsServerEventLogger: :param int date_joined_premium: Timestamp for starting premium_status subscription, seconds since epoch, -1 if not subscribed :param bool has_extension: The user has the Relay Add-on :param int date_got_extension: Timestamp for adding Relay Add-on, seconds since epoch, -1 if not used - :param str mask_id: Mask ID, 'R' (random mask) or 'D' (domain mask) followed by a number. :param bool is_random_mask: The mask is a random mask, instead of a domain mask :param bool is_reply: The email is a reply from the Relay user """ @@ -213,7 +208,6 @@ class EventsServerEventLogger: "date_joined_premium": str(date_joined_premium), "has_extension": str(has_extension).lower(), "date_got_extension": str(date_got_extension), - "mask_id": str(mask_id), "is_random_mask": str(is_random_mask).lower(), "is_reply": str(is_reply).lower(), }, @@ -235,7 +229,6 @@ class EventsServerEventLogger: date_joined_premium: int, has_extension: bool, date_got_extension: int, - mask_id: str, is_random_mask: bool, created_by_api: bool, has_website: bool, @@ -259,7 +252,6 @@ class EventsServerEventLogger: :param int date_joined_premium: Timestamp for starting premium_status subscription, seconds since epoch, -1 if not subscribed :param bool has_extension: The user has the Relay Add-on :param int date_got_extension: Timestamp for adding Relay Add-on, seconds since epoch, -1 if not used - :param str mask_id: Mask ID, 'R' (random mask) or 'D' (domain mask) followed by a number. :param bool is_random_mask: The mask is a random mask, instead of a domain mask :param bool created_by_api: The mask was created via the API, rather than an incoming email :param bool has_website: The mask was created by the Add-on or integration on a website @@ -279,7 +271,6 @@ class EventsServerEventLogger: "date_joined_premium": str(date_joined_premium), "has_extension": str(has_extension).lower(), "date_got_extension": str(date_got_extension), - "mask_id": str(mask_id), "is_random_mask": str(is_random_mask).lower(), "created_by_api": str(created_by_api).lower(), "has_website": str(has_website).lower(), @@ -302,7 +293,6 @@ class EventsServerEventLogger: date_joined_premium: int, has_extension: bool, date_got_extension: int, - mask_id: str, is_random_mask: bool, ) -> None: """ @@ -324,7 +314,6 @@ class EventsServerEventLogger: :param int date_joined_premium: Timestamp for starting premium_status subscription, seconds since epoch, -1 if not subscribed :param bool has_extension: The user has the Relay Add-on :param int date_got_extension: Timestamp for adding Relay Add-on, seconds since epoch, -1 if not used - :param str mask_id: Mask ID, 'R' (random mask) or 'D' (domain mask) followed by a number. :param bool is_random_mask: The mask is a random mask, instead of a domain mask """ event = { @@ -342,7 +331,6 @@ class EventsServerEventLogger: "date_joined_premium": str(date_joined_premium), "has_extension": str(has_extension).lower(), "date_got_extension": str(date_got_extension), - "mask_id": str(mask_id), "is_random_mask": str(is_random_mask).lower(), }, } @@ -363,7 +351,6 @@ class EventsServerEventLogger: date_joined_premium: int, has_extension: bool, date_got_extension: int, - mask_id: str, is_random_mask: bool, ) -> None: """ @@ -385,7 +372,6 @@ class EventsServerEventLogger: :param int date_joined_premium: Timestamp for starting premium_status subscription, seconds since epoch, -1 if not subscribed :param bool has_extension: The user has the Relay Add-on :param int date_got_extension: Timestamp for adding Relay Add-on, seconds since epoch, -1 if not used - :param str mask_id: Mask ID, 'R' (random mask) or 'D' (domain mask) followed by a number. :param bool is_random_mask: The mask is a random mask, instead of a domain mask """ event = { @@ -403,7 +389,6 @@ class EventsServerEventLogger: "date_joined_premium": str(date_joined_premium), "has_extension": str(has_extension).lower(), "date_got_extension": str(date_got_extension), - "mask_id": str(mask_id), "is_random_mask": str(is_random_mask).lower(), }, } diff --git a/privaterelay/glean_interface.py b/privaterelay/glean_interface.py index 203af1bac..3855ace64 100644 --- a/privaterelay/glean_interface.py +++ b/privaterelay/glean_interface.py @@ -113,22 +113,18 @@ class UserData(NamedTuple): class EmailMaskData(NamedTuple): """Extract and store data from a Relay email mask.""" - mask_id: str is_random_mask: bool has_website: bool @classmethod def from_mask(cls, mask: RelayAddress | DomainAddress) -> EmailMaskData: - mask_id = mask.metrics_id if isinstance(mask, RelayAddress): is_random_mask = True has_website = bool(mask.generated_for) else: is_random_mask = False has_website = False - return EmailMaskData( - mask_id=mask_id, is_random_mask=is_random_mask, has_website=has_website - ) + return EmailMaskData(is_random_mask=is_random_mask, has_website=has_website) class RelayGleanLogger(EventsServerEventLogger): @@ -179,7 +175,6 @@ class RelayGleanLogger(EventsServerEventLogger): date_joined_premium=_opt_dt_to_glean(user_data.date_joined_premium), has_extension=user_data.has_extension, date_got_extension=_opt_dt_to_glean(user_data.date_got_extension), - mask_id=mask_data.mask_id, is_random_mask=mask_data.is_random_mask, created_by_api=created_by_api, has_website=mask_data.has_website, @@ -211,7 +206,6 @@ class RelayGleanLogger(EventsServerEventLogger): date_joined_premium=_opt_dt_to_glean(user_data.date_joined_premium), has_extension=user_data.has_extension, date_got_extension=_opt_dt_to_glean(user_data.date_got_extension), - mask_id=mask_data.mask_id, is_random_mask=mask_data.is_random_mask, ) @@ -220,7 +214,6 @@ class RelayGleanLogger(EventsServerEventLogger): *, request: HttpRequest, user: User, - mask_id: str, is_random_mask: bool, ) -> None: """Log that a Relay email mask was deleted.""" @@ -242,7 +235,6 @@ class RelayGleanLogger(EventsServerEventLogger): date_joined_premium=_opt_dt_to_glean(user_data.date_joined_premium), has_extension=user_data.has_extension, date_got_extension=_opt_dt_to_glean(user_data.date_got_extension), - mask_id=mask_id, is_random_mask=is_random_mask, ) @@ -272,7 +264,6 @@ class RelayGleanLogger(EventsServerEventLogger): date_joined_premium=_opt_dt_to_glean(user_data.date_joined_premium), has_extension=user_data.has_extension, date_got_extension=_opt_dt_to_glean(user_data.date_got_extension), - mask_id=mask_data.mask_id, is_random_mask=mask_data.is_random_mask, is_reply=is_reply, ) @@ -304,7 +295,6 @@ class RelayGleanLogger(EventsServerEventLogger): date_joined_premium=_opt_dt_to_glean(user_data.date_joined_premium), has_extension=user_data.has_extension, date_got_extension=_opt_dt_to_glean(user_data.date_got_extension), - mask_id=mask_data.mask_id, is_random_mask=mask_data.is_random_mask, is_reply=is_reply, reason=reason, diff --git a/privaterelay/tests/glean_tests.py b/privaterelay/tests/glean_tests.py index c8a0e0ab7..e81fe3e75 100644 --- a/privaterelay/tests/glean_tests.py +++ b/privaterelay/tests/glean_tests.py @@ -175,7 +175,6 @@ def test_email_mask_relay_address() -> None: mask_data = EmailMaskData.from_mask(mask) - assert mask_data.mask_id == mask.metrics_id assert mask_data.is_random_mask is True assert mask_data.has_website is False @@ -199,7 +198,6 @@ def test_email_mask_domain_address() -> None: mask_data = EmailMaskData.from_mask(mask) - assert mask_data.mask_id == mask.metrics_id assert mask_data.is_random_mask is False assert mask_data.has_website is False @@ -307,7 +305,6 @@ def test_log_email_mask_created( name="created", extra_items={ "n_random_masks": "1", - "mask_id": address.metrics_id, "is_random_mask": "true", "has_website": "false", "created_by_api": "true", @@ -367,7 +364,6 @@ def test_log_email_mask_label_updated( name="label_updated", extra_items={ "n_random_masks": "1", - "mask_id": address.metrics_id, "is_random_mask": "true", }, user=user, @@ -408,13 +404,10 @@ def test_log_email_mask_deleted( """Check that log_email_mask_deleted results in a Glean server-side log.""" user = make_free_test_user() address = baker.make(RelayAddress, user=user) - mask_id = address.metrics_id request = rf.delete(f"/api/v1/relayaddresses/{address.id}/") address.delete() # Real request will delete the mask before glean event - glean_logger.log_email_mask_deleted( - user=user, mask_id=mask_id, is_random_mask=True, request=request - ) + glean_logger.log_email_mask_deleted(user=user, is_random_mask=True, request=request) # Check the one glean-server-event log assert len(caplog.records) == 1 record = caplog.records[0] @@ -428,7 +421,6 @@ def test_log_email_mask_deleted( name="deleted", extra_items={ "n_random_masks": "0", - "mask_id": mask_id, "is_random_mask": "true", }, user=user, @@ -448,14 +440,11 @@ def test_log_email_mask_deleted_with_opt_out( ) -> None: """A log is not emitted for mask deletion when the user has opted-out of metrics""" address = baker.make(RelayAddress, user=optout_user) - mask_id = address.metrics_id user = address.user request = rf.delete(f"/api/v1/relayaddresses/{address.id}/") address.delete() # Real request will delete the mask before glean event - glean_logger.log_email_mask_deleted( - user=user, mask_id=mask_id, is_random_mask=True, request=request - ) + glean_logger.log_email_mask_deleted(user=user, is_random_mask=True, request=request) assert len(caplog.records) == 0 @@ -485,7 +474,6 @@ def test_log_email_forwarded( name="forwarded", extra_items={ "n_random_masks": "1", - "mask_id": address.metrics_id, "is_random_mask": "true", "is_reply": "true" if is_reply else "false", }, @@ -537,7 +525,6 @@ def test_log_email_blocked( name="blocked", extra_items={ "n_random_masks": "1", - "mask_id": address.metrics_id, "is_random_mask": "true", "is_reply": "true" if is_reply else "false", "reason": reason, diff --git a/telemetry/glean/relay-server-metrics.yaml b/telemetry/glean/relay-server-metrics.yaml index 73abe9da2..ea1cfdc6b 100644 --- a/telemetry/glean/relay-server-metrics.yaml +++ b/telemetry/glean/relay-server-metrics.yaml @@ -59,9 +59,6 @@ email_mask: date_got_extension: description: Timestamp for adding Relay Add-on, seconds since epoch, -1 if not used type: quantity - mask_id: - description: Mask ID, 'R' (random mask) or 'D' (domain mask) followed by a number. - type: string is_random_mask: description: The mask is a random mask, instead of a domain mask type: boolean