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

Add option to track MAU stats (but not limit people) #3830

Merged
merged 11 commits into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@turt2live
Copy link
Member

turt2live commented Sep 9, 2018

No description provided.

turt2live added some commits Sep 9, 2018

@turt2live turt2live referenced this pull request Sep 12, 2018

Closed

MAU transaction errors #3854

@richvdh
Copy link
Member

richvdh left a comment

@turt2live please can you fix the conflict and the test failure?

@turt2live turt2live removed their assignment Sep 18, 2018

@turt2live turt2live requested a review from richvdh Sep 18, 2018

@richvdh richvdh requested review from matrix-org/synapse-core and removed request for richvdh Sep 19, 2018

test fixed

@richvdh
Copy link
Member

richvdh left a comment

looks plausible to me, though since @neilisfragile wrote most of this code it might be sensible for him to take a look.

@richvdh richvdh requested a review from neilisfragile Sep 19, 2018

@neilisfragile
Copy link
Contributor

neilisfragile left a comment

Looks good, my only ask is some tests to prove the logic - either in test/store/test_monthly_active_users or in the more integration-y test like test/test_mau.py

@@ -235,8 +235,12 @@ def populate_monthly_active_users(self, user_id):
# but only update if we have not previously seen the user for
# LAST_SEEN_GRANULARITY ms
if last_seen_timestamp is None:
count = yield self.get_monthly_active_count()
if count < self.hs.config.max_mau_value:
# Optimize the db usage when not limiting usage

This comment has been minimized.

@neilisfragile

neilisfragile Sep 20, 2018

Contributor

I had to look at it a few times to understand the intent - I'm not sure I can suggest a more expressive way to do via code, but a more descriptive comment would be helpful.

I'm not sure it will necessarily help a great deal with db load since get_monthly_active_count is cached but the logic expects max_mau_value to be set and so I agree that it needs the if check.

Perhaps something like

"In the case where mau_stats_only is True and limit_usage_by_mau is False, there is no point in checking get_monthly_active_count - it adds no value and will break the logic if max_mau_value is exceeded "

This comment has been minimized.

@richvdh

richvdh Sep 21, 2018

Member

I had to look at it a few times to understand the intent

(I had the same reaction tbh)

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 2, 2018

(fixed a conflict, haven't addressed anything else - will poke it Soon)

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 16, 2018

This was done with an unpaid hat on:

Signed-off-by: Travis Ralston travpc@gmail.com

@turt2live turt2live removed their assignment Oct 16, 2018

@neilisfragile

This comment has been minimized.

Copy link
Contributor

neilisfragile commented Oct 16, 2018

LGTM you happy @richvdh?

turt2live added some commits Oct 16, 2018

@Valodim

This comment has been minimized.

Copy link
Contributor

Valodim commented Nov 13, 2018

I'd love to have this functionality, can we get this merged?

@richvdh richvdh self-requested a review Nov 15, 2018

@richvdh
Copy link
Member

richvdh left a comment

lgtm

@richvdh richvdh merged commit 835779f into matrix-org:develop Nov 15, 2018

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@turt2live turt2live deleted the turt2live:travis/mau-stats branch Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment