Skip to content
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 Synapse Stats Exporter #140

Closed
wants to merge 27 commits into from

Conversation

amandahla
Copy link
Collaborator

@amandahla amandahla commented Jan 17, 2024

Overview

In order to collect and monitor the number of users and rooms in the Synapse instance, this PR install the synapse-stats-exporter so the metrics can be extracted by Prometheus.

Also, it refactors how the charm handle the admin_access_token.

Before the refactor, the admin access token was handled only by the charm and used by Mjolnir.

But since the Stats Exporter needs it too, it was complex to let this in the charm instead of letting the Synapse module handle it.

So what happens now?

  • start event is emitted meaning that (hopefully) the workload was started (not just the container)

  • If the admin access token already exists (as a secret or as peer data), start the Stats Exporter and return

  • If the admin access token doesn't exist, create it via create_admin_user and save it in the charm_state

  • Mjolnir and Stats Exporter will get the admin_access_token via charm_state

  • Also, Mjolnir will create the user using the create_user available in the synapse module instead of calling actions.register_user (that didn't make much sense)

This is how the modules depend on each other now:

image

Rationale

Collect Synapse statistics.

Juju Events Changes

N/A

Module Changes

Pebble module has a new layer.

Library Changes

N/A

Checklist

Documentation should be updated after the exporter being available.

@amandahla amandahla requested a review from a team as a code owner January 17, 2024 18:38
@amandahla amandahla marked this pull request as draft January 17, 2024 18:38
Copy link
Contributor

@merkata merkata left a comment

Choose a reason for hiding this comment

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

Good start, left some minor comments, interested to see the tests on this.

src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
def _on_start(self, _: ops.HookEvent) -> None:
"""Handle start event."""
if self.get_admin_access_token():
self._start_synapse_stats_exporter()
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn'g mjolnir require an access token as well? Should it be added here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, Mjolnir requires it too.

  • If Mjolnir is enabled, the charm will instantiate a Mjolnir class passing state as parameter
  • The class observes the collect status event that will check for the requirements and enable Mjolnir when everything is set

get_admin_access_token = getattr(self._charm, "get_admin_access_token", None)
if not get_admin_access_token:
access_token = self._charm_state.synapse_config.admin_access_token
if not access_token:
logging.error("Failed to get method get_admin_access_token.")
Copy link
Contributor

Choose a reason for hiding this comment

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

That error can be reworked now due to the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now if the admin access token is None, the charm will be put on MaintenanceStatus. Do you think it should be changed?

self._charm.unit.status = ops.MaintenanceStatus(
"Failed to get admin access token. Please, check the logs."
)

src/synapse/admin.py Outdated Show resolved Hide resolved
synapse_rock/rockcraft.yaml Outdated Show resolved Hide resolved
@amandahla amandahla force-pushed the ISD-1519-synapse-get-usage-statistics branch from 5beb98b to b13def7 Compare January 23, 2024 18:47
src-docs/charm.py.md Outdated Show resolved Hide resolved
src/pebble.py Show resolved Hide resolved
@amandahla amandahla changed the title WIP - Add Synapse Stats Exporter Add Synapse Stats Exporter Jan 30, 2024
@amandahla amandahla marked this pull request as ready for review January 30, 2024 21:45
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Test coverage for 53d1097

Name                            Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------------
src/actions/__init__.py             3      0      0      0   100%
src/actions/register_user.py       19      1      2      1    90%   70
src/actions/reset_instance.py      21      0      2      0   100%
src/backup.py                      48      0      4      0   100%
src/backup_observer.py             35      0      2      0   100%
src/charm.py                      235     12     56      5    94%   164-165, 216-217, 244-245, 249-250, 253-254, 310-311
src/charm_state.py                 64      1     10      1    97%   129
src/charm_types.py                 23      0      6      0   100%
src/database_client.py             53      1     10      3    94%   35, 47->exit, 69->exit
src/database_observer.py           48      4      4      0    92%   69-71, 87
src/exceptions.py                   4      1      0      0    75%   22
src/mjolnir.py                     78      8     22      2    88%   59-63, 72, 164-165
src/observability.py                9      0      0      0   100%
src/pebble.py                     108     13     18      7    84%   84, 107-108, 110-111, 119, 121, 123, 127, 148-149, 164-165
src/saml_observer.py               45      1      8      0    98%   64
src/smtp_observer.py               70      3     14      1    95%   70-74, 96->101
src/synapse/__init__.py             4      0      0      0   100%
src/synapse/admin.py               20      2      2      0    91%   39-40
src/synapse/api.py                161      2     22      2    98%   207, 376
src/synapse/workload.py           272      8     42      6    96%   405->exit, 434-435, 487-488, 524-525, 541, 590->593, 639, 647->649, 649->651
src/user.py                        24      0      4      0   100%
---------------------------------------------------------------------------
TOTAL                            1344     57    228     28    94%

Static code analysis report

Run started:2024-02-01 16:06:06.236358

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 7039
  Total lines skipped (#nosec): 4
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

nrobinaubertin
nrobinaubertin previously approved these changes Feb 1, 2024
renovate bot and others added 2 commits February 8, 2024 17:57
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
renovate bot and others added 10 commits February 8, 2024 17:57
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Mariyan Dimitrov <[email protected]>
* Change Mjolnir web address to fix abuse reports

* Try to deploy Prometheus and Grafana from edge

---------

Co-authored-by: Mariyan Dimitrov <[email protected]>
Co-authored-by: arturo-seijas <[email protected]>
* Create backup action

* Remove unused import

* Add missing src-docs

* Reorder functions

* Improve comment with suggestion to limit bandwidth

* Add mark.s2

* Fix src-docs

* Remeve unused backup_id parameter

* Improving func comment

* Replace string addition with f-string

* Add comment in pathlib

* Unneeded lines

* Remove code to protect against int casting as that should not happen

* Remove default expected_size as it is not used

* Use full path for bash command

* Improve formatting and set bash to full path

* Get the media directory dinamically from the homeserver.yaml

* Add missing how-to

---------

Co-authored-by: Amanda H. L. de Andrade Katz <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Christopher Bartz <[email protected]>
* Create list backups action

* Improve comments. Sent data in event.fail

* Allow more than 1000 objects in list-backups

* Remove unused fields in S3Backup

* Improve test comments

* Fix typo

* Improve comments in docstrings

* Rename function from iterate_objects to list_s3_objects

* Apply suggestions from code review

use list instead of typing.List

Co-authored-by: Christopher Bartz <[email protected]>

* Remove unused import and change typing.List to list

---------

Co-authored-by: Christopher Bartz <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: arturo-seijas <[email protected]>
Co-authored-by: Mariyan Dimitrov <[email protected]>
Co-authored-by: javierdelapuente <[email protected]>
Co-authored-by: Amanda H. L. de Andrade Katz <[email protected]>
* Add cron service and cleanup script

* Fix rockfile

* Fix restart_synapse()

* Remove unused fn

* Fix restart_synapse()

* Fix linting

* Fix rockfile

* Throttle cleanup operations

* Add missing headers

* Add some licensing exceptions

* Try to fix headers ignore once again

* Make scripts added to the rock executable

* Rerun CI

* Only modify rockcraft.yml where necessary

* Fix rockcraft scripts permissions

* Fix typo

---------

Co-authored-by: Amanda H. L. de Andrade Katz <[email protected]>
Co-authored-by: arturo-seijas <[email protected]>
@amandahla
Copy link
Collaborator Author

I'm closing this PR in favor of splitting the changes into smaller ones. This is already too messy.

@amandahla amandahla closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants