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 metric to count lazy member sync requests #4022

Merged
merged 6 commits into from Oct 10, 2018

Conversation

Projects
None yet
2 participants
@erikjohnston
Member

erikjohnston commented Oct 9, 2018

No description provided.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 9, 2018

@richvdh

richvdh approved these changes Oct 9, 2018

# Counts the number of times we got asked for a lazy loaded sync. Type is one of
# initial_sync, full_sate_sync or incremental_sync
lazy_member_sync_counter = Counter(
"synapse_handlers_sync_lazy_member_sync", "", ["type"],

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

"apparently" best practice is for these to be called xyz_total ...

@@ -36,6 +38,13 @@
logger = logging.getLogger(__name__)
# Counts the number of times we got asked for a lazy loaded sync. Type is one of
# initial_sync, full_sate_sync or incremental_sync

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

full_state_sync

context.tag = sync_type
if sync_config.filter_collection.lazy_load_members():
lazy_member_sync_counter.labels(sync_type).inc()

This comment has been minimized.

@richvdh

richvdh Oct 9, 2018

Member

I'm sort-of wondering if we should only bump this when the result is non-empty (ie, do it in current_sync_for_user rather than here). Not bothered though.

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 10, 2018

I'm sort-of wondering if we should only bump this when the result is non-empty (ie, do it in current_sync_for_user rather than here). Not bothered though.

I was sort of planning on comparing it to the total number of sync requests we received, but that doesn't actually work. I've changed it so that we have a count of number of non-empty sync responses, and have made whether it was a lazy loaded request or not a label.

Which I think makes more sense?

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 10, 2018

@@ -0,0 +1 @@
Add metric to count lazy member sync requests

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

this could do with some expansion

# "true" or "false" depending on if the request asked for lazy loaded members or
# not.
non_empty_sync_counter = Counter(
"synapse_handlers_sync_nonempty_total", "", ["type", "lazy_loaded"],

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

the second param is a description. why not use it!

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 10, 2018

I've added a description. I'm not sure whether its too much or too little, as we generally haven't used description. (I think to keep the metric size down?)

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 10, 2018

@richvdh

This comment has been minimized.

Member

richvdh commented Oct 10, 2018

I think to keep the metric size down?

more likely because the predecessor to prometheus-client made them hard

@richvdh

lgtm

@erikjohnston erikjohnston merged commit e97d939 into develop Oct 10, 2018

6 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
continuous-integration/travis-ci/push The Travis CI build passed
Details

michaelkaye added a commit that referenced this pull request Oct 22, 2018

Merge tag 'v0.33.7' into dinsic
**Warning**: This release removes the example email notification templates from
`res/templates` (they are now internal to the python package). This should only
affect you if you (a) deploy your Synapse instance from a git checkout or a
github snapshot URL, and (b) have email notifications enabled.

If you have email notifications enabled, you should ensure that
`email.template_dir` is either configured to point at a directory where you
have installed customised templates, or leave it unset to use the default
templates.

The configuration parser will try to detect the situation where
`email.template_dir` is incorrectly set to `res/templates` and do the right
thing, but will warn about this.

Features
--------

- Ship the example email templates as part of the package ([\#4052](#4052))
- Add support for end-to-end key backup (MSC1687) ([\#4019](#4019))

Bugfixes
--------

- Fix bug which made get_missing_events return too few events ([\#4045](#4045))
- Fix bug in event persistence logic which caused 'NoneType is not iterable' ([\#3995](#3995))
- Fix exception in background metrics collection ([\#3996](#3996))
- Fix exception handling in fetching remote profiles ([\#3997](#3997))
- Fix handling of rejected threepid invites ([\#3999](#3999))
- Workers now start on Python 3. ([\#4027](#4027))
- Synapse now starts on Python 3.7. ([\#4033](#4033))

Internal Changes
----------------

- Log exceptions in looping calls ([\#4008](#4008))
- Optimisation for serving federation requests ([\#4017](#4017))
- Add metric to count number of non-empty sync responses ([\#4022](#4022))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment