Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add tests to characterise the current behaviour of R30 phone-home metrics #10315

Merged
merged 10 commits into from
Jul 15, 2021
1 change: 1 addition & 0 deletions changelog.d/10315.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add tests to characterise the current behaviour of R30 phone-home metrics.
99 changes: 99 additions & 0 deletions tests/app/test_phone_stats_home.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import synapse
from synapse.rest.client.v1 import login, room

from tests import unittest
from tests.unittest import HomeserverTestCase

DAY = 86400
reivilibre marked this conversation as resolved.
Show resolved Hide resolved


class PhoneHomeTestCase(HomeserverTestCase):
servlets = [
synapse.rest.admin.register_servlets_for_client_rest_resource,
room.register_servlets,
login.register_servlets,
]

# Override the retention time for the user_ips table because otherwise it
# gets pruned too aggressively for our R30 test.
@unittest.override_config({"user_ips_max_age": "365d"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that R30 doesn't work out of the box?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is 28d, so out of the box, it does seem like it's slightly broken, yes...

Out of the box, it can only count users who had activity in the last 28 days, where that activity occurred at least 30 days after registration.

The intention would be to count users who had activity in the last 30 days, where that activity occurred at least 30 days after registration, so it's not a MASSIVE difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just wary of testing a feature by first changing the default config? Can we update the test to work with the default, or do we need to update the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, the default should be changed. (However, I'm not sure how worthwhile that is since the idea is to add R30v2 and eventually remove R30 [I expect]...).
Updating the test to work with the default wouldn't really show what R30 actually means, it'd instead be R28 or something ;p.
I was hoping to basically just add a test to characterise the current behaviour and wasn't intending to change anything, though...

Suppose I could make a copy of the test that uses the default settings? What do you think is preferable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess its fair to merge as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes, I think we should have a test for the default, since this is for phone home stats.

def test_r30_minimum_usage(self):
"""
Tests the minimum amount of interaction necessary for the R30 metric
to consider a user 'retained'.
"""

# Register a user, log it in, create a room and send a message
user_id = self.register_user("u1", "secret!")
access_token = self.login("u1", "secret!")
room_id = self.helper.create_room_as(room_creator=user_id, tok=access_token)
self.helper.send(room_id, "message", tok=access_token)

# Check the R30 results do not count that user.
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 0})

# Advance 30 days (+ 1 second, because strict inequality causes issues if we are
# bang on 30 days later).
self.reactor.advance(30 * DAY + 1)

# (Make sure the user isn't somehow counted by this point.)
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 0})

# Send a message (this counts as activity)
self.helper.send(room_id, "message2", tok=access_token)

# We have to wait up to 5 seconds for _update_client_ips_batch to get
# called and update the user_ips table in a batch.
self.reactor.advance(5.1)
reivilibre marked this conversation as resolved.
Show resolved Hide resolved

# *Now* the user is counted.
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 1, "unknown": 1})

# Advance 29 days. The user has now not posted for 29 days.
self.reactor.advance(29 * DAY)

# The user is still counted.
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 1, "unknown": 1})

# Advance another day. The user has now not posted for 30 days.
self.reactor.advance(DAY)

# The user is now no longer counted in R30.
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 0})

def test_r30_user_must_be_retained_for_at_least_a_month(self):
"""
Tests that a newly-registered user must be retained for a whole month
before appearing in the R30 statistic, even if they post every day
during that time!
"""
# Register a user and send a message
user_id = self.register_user("u1", "secret!")
access_token = self.login("u1", "secret!")
room_id = self.helper.create_room_as(room_creator=user_id, tok=access_token)
self.helper.send(room_id, "message", tok=access_token)

# Check the user does not contribute to R30 yet.
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 0})

for _ in range(30):
# This loop posts a message every day for 30 days
self.reactor.advance(DAY)
self.helper.send(room_id, "I'm still here", tok=access_token)

# Notice that the user *still* does not contribute to R30!
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 0})

self.reactor.advance(DAY)
self.helper.send(room_id, "Still here!", tok=access_token)

# *Now* the user appears in R30.
r30_results = self.get_success(self.hs.get_datastore().count_r30_users())
self.assertEqual(r30_results, {"all": 1, "unknown": 1})