From 4e9f77a6c4b851fbfc719e8e65f00aadc27bae6a Mon Sep 17 00:00:00 2001
From: Mateusz Mandera <mateusz.mandera@protonmail.com>
Date: Fri, 6 Mar 2020 10:56:36 +0100
Subject: [PATCH] rate_limit: Adjust keys() of some RateLimitedObjects.

type().__name__ is sufficient, and much readable than type(), so it's
better to use the former for keys.
We also make the classes consistent in forming the keys in the format
type(self).__name__:identifier and adjust logger.warning and statsd to
take advantage of that and simply log the key().
---
 zerver/forms.py                   | 2 +-
 zerver/lib/email_mirror.py        | 2 +-
 zerver/lib/rate_limiter.py        | 7 +++----
 zerver/tests/test_external.py     | 2 +-
 zerver/tests/test_queue_worker.py | 3 ++-
 zproject/backends.py              | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/zerver/forms.py b/zerver/forms.py
index cfeb99eb80..6584493772 100644
--- a/zerver/forms.py
+++ b/zerver/forms.py
@@ -309,7 +309,7 @@ class RateLimitedPasswordResetByEmail(RateLimitedObject):
         return "Email: {}".format(self.email)
 
     def key(self) -> str:
-        return "{}:{}".format(type(self), self.email)
+        return "{}:{}".format(type(self).__name__, self.email)
 
     def rules(self) -> List[Tuple[int, int]]:
         return settings.RATE_LIMITING_RULES['password_reset_form_by_email']
diff --git a/zerver/lib/email_mirror.py b/zerver/lib/email_mirror.py
index 92e859b587..f978cbf58e 100644
--- a/zerver/lib/email_mirror.py
+++ b/zerver/lib/email_mirror.py
@@ -445,7 +445,7 @@ class RateLimitedRealmMirror(RateLimitedObject):
         super().__init__()
 
     def key(self) -> str:
-        return "emailmirror:{}:{}".format(type(self.realm), self.realm.id)
+        return "{}:{}".format(type(self).__name__, self.realm.string_id)
 
     def rules(self) -> List[Tuple[int, int]]:
         return settings.RATE_LIMITING_MIRROR_REALM_RULES
diff --git a/zerver/lib/rate_limiter.py b/zerver/lib/rate_limiter.py
index 53c235c966..73961e8819 100644
--- a/zerver/lib/rate_limiter.py
+++ b/zerver/lib/rate_limiter.py
@@ -113,7 +113,7 @@ class RateLimitedUser(RateLimitedObject):
         return "Id: {}".format(self.user.id)
 
     def key(self) -> str:
-        return "{}:{}:{}".format(type(self.user), self.user.id, self.domain)
+        return "{}:{}:{}".format(type(self).__name__, self.user.id, self.domain)
 
     def rules(self) -> List[Tuple[int, int]]:
         # user.rate_limits are general limits, applicable to the domain 'api_by_user'
@@ -342,14 +342,13 @@ class RedisRateLimiterBackend(RateLimiterBackend):
         ratelimited, time = cls.is_ratelimited(entity)
 
         if ratelimited:
-            statsd.incr("ratelimiter.limited.%s.%s" % (type(entity), str(entity)))
+            statsd.incr("ratelimiter.limited.%s" % (entity.key(),))
 
         else:
             try:
                 cls.incr_ratelimit(entity)
             except RateLimiterLockingException:
-                logger.warning("Deadlock trying to incr_ratelimit for %s:%s" % (
-                               type(entity).__name__, str(entity)))
+                logger.warning("Deadlock trying to incr_ratelimit for %s" % (entity.key(),))
                 # rate-limit users who are hitting the API so hard we can't update our stats.
                 ratelimited = True
 
diff --git a/zerver/tests/test_external.py b/zerver/tests/test_external.py
index c2e29d10ff..03348392c9 100644
--- a/zerver/tests/test_external.py
+++ b/zerver/tests/test_external.py
@@ -121,5 +121,5 @@ class RateLimitTests(ZulipTestCase):
                         side_effect=RateLimiterLockingException):
             result = self.send_api_message(user, "some stuff")
             self.assertEqual(result.status_code, 429)
-            mock_warn.assert_called_with("Deadlock trying to incr_ratelimit for RateLimitedUser:Id: %s"
+            mock_warn.assert_called_with("Deadlock trying to incr_ratelimit for RateLimitedUser:%s:api_by_user"
                                          % (user.id,))
diff --git a/zerver/tests/test_queue_worker.py b/zerver/tests/test_queue_worker.py
index 1715210494..3264b43624 100644
--- a/zerver/tests/test_queue_worker.py
+++ b/zerver/tests/test_queue_worker.py
@@ -411,7 +411,8 @@ class WorkerTest(ZulipTestCase):
                     fake_client.queue.append(('email_mirror', data[0]))
                     worker.start()
                     self.assertEqual(mock_mirror_email.call_count, 4)
-                    expected_warn = "Deadlock trying to incr_ratelimit for RateLimitedRealmMirror:zulip"
+                    expected_warn = "Deadlock trying to incr_ratelimit for RateLimitedRealmMirror:%s" % (
+                        realm.string_id,)
                     mock_warn.assert_called_with(expected_warn)
 
     def test_email_sending_worker_retries(self) -> None:
diff --git a/zproject/backends.py b/zproject/backends.py
index 15cf8b2273..2d23daa662 100644
--- a/zproject/backends.py
+++ b/zproject/backends.py
@@ -186,7 +186,7 @@ class RateLimitedAuthenticationByUsername(RateLimitedObject):
         return "Username: {}".format(self.username)
 
     def key(self) -> str:
-        return "{}:{}".format(type(self), self.username)
+        return "{}:{}".format(type(self).__name__, self.username)
 
     def rules(self) -> List[Tuple[int, int]]:
         return rate_limiting_rules