Switch to the Python Prometheus library by hawkowl · Pull Request #3256 · matrix-org/synapse

hawkowl · 2018-05-22T00:50:53Z

erikjohnston · 2018-05-22T08:40:32Z

Awesome! I maybe should have mentioned that ideally the changes would be backwards compatible?

erikjohnston · 2018-05-22T14:45:59Z

(I also added some metrics in #3252 I'm afraid)

erikjohnston

Woo, this looks great! I haven't gone through it to carefully, but I did run it on jki.re which picked up a few typos in the names.

The main thing that I think we're missing is keeping the user/system CPU metrics. We've had problems in the past where we see spikes in system CPU, so we'd really want to keep that I think.

erikjohnston · 2018-05-23T08:14:17Z

-        metrics.register_callback(
-            "pending_destinations",
+        LaterGauge(
+            "synapse_federation_client_pending_destinations",


We're in synapse_federation_transaction_queue_

erikjohnston · 2018-05-23T08:18:59Z

+push_rules_invalidation_counter = Counter(
+    "synapse_push_bulk_push_role_evaluator_push_rules_invalidation_counter", "")
+push_rules_state_size_counter = Counter(
+    "synapse_push_bulk_push_role_evaluator_push_rules_state_size_counter", "")


_rule_ not _role_

erikjohnston · 2018-05-23T08:20:55Z

-response_size = metrics.register_counter(
-    "response_size", labels=["method", "servlet", "tag"]
+response_size = Counter(
+    "synapse_http_request_response_size", "", ["method", "servlet", "tag"]


http_server rather than http_request

erikjohnston · 2018-05-23T08:44:10Z

+    "python_gc_time",
+    "Time taken to GC (ms)",
+    ["gen"],
+    buckets=[1, 2, 5, 10, 25, 50, 100, 250, 500, 1000],


Gen 2 can sometimes take 30s, so probably worth adding a few more buckets. Getting an idea of large GC times gives us an insight into why things might occasionally be pausing for tens of seconds ,which is useful.

Added 2.5, 5, 7.5, 15, 30, 45, and 60 second buckets.

erikjohnston · 2018-05-23T08:46:05Z

+    # Check if the metric is already registered. Unregister it, if so.
+    metric_name = "cache_%s_%s" % (cache_type, cache_name)
+    if metric_name in collectors_by_name.keys():
+        REGISTRY.unregister(collectors_by_name[metric_name])


Why would they already be registered?

Tests, because otherwise prom throws an error. We could get around this by making them top level and putting the names as labels always, as prom_client will throw an exception if you register something with the same name twice (like in tests).

Ah right, might be worth just putting a comment that its for tests.

hawkowl · 2018-05-23T16:53:36Z

Thanks for the review @erikjohnston.

I'm going to add back the CPU metrics, and then I'll put it back up.

fixes made

erikjohnston

Running this on jki.re and it seems like the only issue is milliseconds vs seconds for the histogram metrics.

erikjohnston · 2018-05-24T11:10:27Z

-sql_query_timer = metrics.register_distribution("query_time", labels=["verb"])
-sql_txn_timer = metrics.register_distribution("transaction_time", labels=["desc"])
+sql_query_timer = Histogram("synapse_storage_query_time", "", ["verb"])
+sql_txn_timer = Histogram("synapse_storage_transaction_time", "", ["desc"])


The default histogram buckets expect them to be observed in seconds rather than milliseconds? I think we'll need to go through and change all the observations to use seconds instead.

erikjohnston

(That last approval was meant to be request changes... though once we have corrected the histogram units I think it'll be ready to merge!)

erikjohnston · 2018-05-28T10:23:31Z

We probably want to add a note to the changelog and docs about which metrics have changed and how to update existing graphs (e.g use histogram_quantile(0.5, ...) to get at the averages)

richvdh · 2018-05-29T10:45:48Z

Some follow-up work (including the changelog?) is happening in #3274

fix bug introduced in #3256

@thegcat

Changes in synapse v0.31.0 (2018-06-06) ====================================== Most notable change from v0.30.0 is to switch to python prometheus library to improve system stats reporting. WARNING this changes a number of prometheus metrics in a backwards-incompatible manner. For more details, see `docs/metrics-howto.rst <docs/metrics-howto.rst#removal-of-deprecated-metrics--time-based-counters-becoming-histograms-in-0310>`_. Bug Fixes: * Fix metric documentation tables (PR #3341) * Fix LaterGuage error handling (694968f) * Fix replication metrics (b7e7fd2) Changes in synapse v0.31.0-rc1 (2018-06-04) ========================================== Features: * Switch to the Python Prometheus library (PR #3256, #3274) * Let users leave the server notice room after joining (PR #3287) Changes: * daily user type phone home stats (PR #3264) * Use iter* methods for _filter_events_for_server (PR #3267) * Docs on consent bits (PR #3268) * Remove users from user directory on deactivate (PR #3277) * Avoid sending consent notice to guest users (PR #3288) * disable CPUMetrics if no /proc/self/stat (PR #3299) * Add local and loopback IPv6 addresses to url_preview_ip_range_blacklist (PR #3312) Thanks to @thegcat! * Consistently use six's iteritems and wrap lazy keys/values in list() if they're not meant to be lazy (PR #3307) * Add private IPv6 addresses to example config for url preview blacklist (PR #3317) Thanks to @thegcat! * Reduce stuck read-receipts: ignore depth when updating (PR #3318) * Put python's logs into Trial when running unit tests (PR #3319) Changes, python 3 migration: * Replace some more comparisons with six (PR #3243) Thanks to @NotAFile! * replace some iteritems with six (PR #3244) Thanks to @NotAFile! * Add batch_iter to utils (PR #3245) Thanks to @NotAFile! * use repr, not str (PR #3246) Thanks to @NotAFile! * Misc Python3 fixes (PR #3247) Thanks to @NotAFile! * Py3 storage/_base.py (PR #3278) Thanks to @NotAFile! * more six iteritems (PR #3279) Thanks to @NotAFile! * More Misc. py3 fixes (PR #3280) Thanks to @NotAFile! * remaining isintance fixes (PR #3281) Thanks to @NotAFile! * py3-ize state.py (PR #3283) Thanks to @NotAFile! * extend tox testing for py3 to avoid regressions (PR #3302) Thanks to @krombel! * use memoryview in py3 (PR #3303) Thanks to @NotAFile! Bugs: * Fix federation backfill bugs (PR #3261) * federation: fix LaterGauge usage (PR #3328) Thanks to @intelfx!

hawkowl added 5 commits May 21, 2018 17:01

remove old metrics libs

f258def

look at the Prometheus metrics instead

02c1d29

don't need the resource portion

c60e0d5

replacing portions

df9f72d

rest of the changes

fcc525b

hawkowl added 3 commits May 22, 2018 10:50

Merge remote-tracking branch 'origin/develop' into 3218-official-prom

a8990fa

cleanups, self-registration

8f5a688

update gitignore to remove some files that got put on my machine

07bb9bd

hawkowl changed the base branch from master to develop May 22, 2018 15:58

hawkowl added 6 commits May 22, 2018 15:02

fix the test failures

228f1f5

fixes

85ba83e

tests/metrics is gone now

4abeaed

cleanup pep8 errors

0712063

cleanup

53cc2cd

more cleanup

b606363

hawkowl changed the title ~~[WIP] Switch to the Python Prometheus library~~ Switch to the Python Prometheus library May 23, 2018

hawkowl requested a review from erikjohnston May 23, 2018 01:35

ara4n assigned erikjohnston May 23, 2018

erikjohnston previously requested changes May 23, 2018

View reviewed changes

erikjohnston assigned hawkowl and unassigned erikjohnston May 23, 2018

hawkowl added 3 commits May 23, 2018 13:03

fixes

e987079

add back CPU metrics

472a5ec

pepeightttt

389dac2

hawkowl assigned erikjohnston and unassigned hawkowl May 23, 2018

erikjohnston approved these changes May 24, 2018

View reviewed changes

erikjohnston assigned hawkowl and unassigned erikjohnston May 24, 2018

erikjohnston suggested changes May 24, 2018

View reviewed changes

hawkowl added 6 commits May 28, 2018 18:57

Merge remote-tracking branch 'origin/develop' into 3218-official-prom

754826a

update metrics to be in seconds

a2eb5db

add comment about why unreg

357c74a

invalid syntax :(

5c40ce3

update to more consistently use seconds in any metrics or logging

3ef5cd7

fix up tests

57ad76f

erikjohnston approved these changes May 28, 2018

View reviewed changes

hawkowl merged commit 81717e8 into develop May 28, 2018

hawkowl deleted the 3218-official-prom branch May 28, 2018 13:30

richvdh added a commit that referenced this pull request Jun 4, 2018

Fix replication metrics

b7e7fd2

fix bug introduced in #3256

richvdh mentioned this pull request Jul 13, 2018

joining a large room completely hosed a homeserver #3527

Open

Conversation

hawkowl commented May 22, 2018

Uh oh!

erikjohnston commented May 22, 2018

Uh oh!

erikjohnston commented May 22, 2018

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkowl commented May 23, 2018

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

erikjohnston commented May 28, 2018

Uh oh!

richvdh commented May 29, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants