search

matrix-org / panopticon

panopticon records usage metrics from homeservers
Apache License 2.0
13 stars 6 forks source link

encrypted messages stats #20

Closed jcgruenhage closed 3 years ago

jcgruenhage commented 3 years ago

I wanted to write a DB migration for this, but it seems like panopticon doesn't handle migrations at all. How have you been dealing with this internally, are these migrations part of your secret ansible sauce, or are you doing the migrations by hand?

jcgruenhage commented 3 years ago

Side note: There's two commits in here, aside of the actual change I fixed a mistake in the descriptions of the daily_messages and daily_sent_messages stats.

clokep commented 3 years ago

are you doing the migrations by hand?

If memory serves me correct, we've been doing them by hand.

clokep commented 3 years ago

This might benefit from some tests added, see https://github.com/matrix-org/panopticon/tree/master/tests This would be adding a new file test_push_good_before_1.27.0.sh (or maybe 1.28.0 depending on when the synapse PR is merged) with the current test in test_push_good.sh and then modifying test_push_good.sh with the new data (#17 recently did this for an example).

jcgruenhage commented 3 years ago

This might benefit from some tests added, see https://github.com/matrix-org/panopticon/tree/master/tests This would be adding a new file test_push_good_before_1.27.0.sh (or maybe 1.28.0 depending on when the synapse PR is merged) with the current test in test_push_good.sh and then modifying test_push_good.sh with the new data (#17 recently did this for an example).

I was about to do that when I wrote the code, and then realized that the tests there already don't include most of the metrics. If you still think it's helpful, I can do it, but I'm not really convinced.

Aside of that, the test cases are not in great shape in general. The lines are freakishly long, most test cases are duplicated among the scripts anyway. Instead, it'd probably make more sense to unify those into a single script, and have a few json/csv files with the request body/expected result in them.

While I obviously have opinions about this, I don't really feel like fixing it in the current state. This whole repo is a bit messy, home-grown shell based tests, no db migrations, two different languages, etc. For those reasons, we're currently considering @ famedly.com to just rewrite panopticon in rust, having a single tool that does both collection and aggregation, including database migration and tests run inside cargo test directly. If we were to do that, and released it under a license you're happy with, would you consider using it as well and collaborating on it? I don't think it makes much sense to have two separate implementations for this, but at the same time, the current implementation of panopticon is really a pain to work on and upgrade. If it makes much of a difference to you, I'm relatively sure we could also donate the code to the matrix foundation and have it live here, as a replacement for the current implementation.

Updated state: We've started implementing this as an intro project for a new hire, so it'll likely take a bit longer than originally anticipated, but expect this to become available soon-ish.

neilisfragile commented 3 years ago

Hi @jcgruenhage thanks for the PR (sorry for the lag), this is really great. I would like to see tests, be we can handle that.

In terms of the codebase overall, it is true that it could do with some love, but I would prefer to evolve what we have rather than rewrite, not least because the team principally maintaining it has more experience in Go and Python than Rust.

Obviously, nothing stops you from rewriting anyway, but the matrix.org hosted panopticon will continue to run the existing implementation.

On the plus side, this might be the kick we need to do some basic modernising 😄

jcgruenhage commented 3 years ago

Hi @jcgruenhage thanks for the PR (sorry for the lag), this is really great. I would like to see tests, be we can handle that.

No worries, we need this too obviously, since we're still running panopticon ourselves

In terms of the codebase overall, it is true that it could do with some love, but I would prefer to evolve what we have rather than rewrite, not least because the team principally maintaining it has more experience in Go and Python than Rust.

I totally get that having a zoo of languages is problematic, which is one of the reasons we chose to go with Rust, because our backend team has more experience with Rust. As for maintainership tasks: We can handle all that, and we'll definitely add all metrics that end up being send by synapse, so there's basically nothing you'd need to do here, if you wanted to use it.

Obviously, nothing stops you from rewriting anyway, but the matrix.org hosted panopticon will continue to run the existing implementation.

:+1:

On the plus side, this might be the kick we need to do some basic modernising

And if not, we'll always be happy to have you on our implementation if you end up changing your mind.