-
Notifications
You must be signed in to change notification settings - Fork 6
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
Report log level. #17
Conversation
Wasn't sure who to request review from, hopefully one of you is correct. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to merge as is.
While you are in the area could you also check if other datapoints are transmitted and not currently handled?
For instance I think count_monthly_users is an example.
For double points, if you could also add aggregation support for count_monthly_users that would be extremely helpful. https://github.com/matrix-org/panopticon/blob/master/scripts/aggregate.py it should be quite easy to crib.
No need to aggregate log level though.
I cross-referenced and it seems I'll add that and take a look at aggregating it. |
@neilisfragile I think I made the proper changes, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm afraid that the testing coverage is pretty poor though changes are generally rare - the aggregate table will also need the mau column adding manually.
This was deployed in release v0.1.3 and I confirmed that both new columns are having data added. |
This matches the changes from matrix-org/synapse#8477.