mirror of
https://github.com/neondatabase/neon.git
synced 2026-01-17 10:22:56 +00:00
pageserver: only throttle pagestream requests & bring back throttling deduction for smgr latency metrics (#9962)
## Problem In the batching PR - https://github.com/neondatabase/neon/pull/9870 I stopped deducting the time-spent-in-throttle fro latency metrics, i.e., - smgr latency metrics (`SmgrOpTimer`) - basebackup latency (+scan latency, which I think is part of basebackup). The reason for stopping the deduction was that with the introduction of batching, the trick with tracking time-spent-in-throttle inside RequestContext and swap-replacing it from the `impl Drop for SmgrOpTimer` no longer worked with >1 requests in a batch. However, deducting time-spent-in-throttle is desirable because our internal latency SLO definition does not account for throttling. ## Summary of changes - Redefine throttling to be a page_service pagestream request throttle instead of a throttle for repository `Key` reads through `Timeline::get` / `Timeline::get_vectored`. - This means reads done by `basebackup` are no longer subject to any throttle. - The throttle applies after batching, before handling of the request. - Drive-by fix: make throttle sensitive to cancellation. - Rename metric label `kind` from `timeline_get` to `pagestream` to reflect the new scope of throttling. To avoid config format breakage, we leave the config field named `timeline_get_throttle` and ignore the `task_kinds` field. This will be cleaned up in a future PR. ## Trade-Offs Ideally, we would apply the throttle before reading a request off the connection, so that we queue the minimal amount of work inside the process. However, that's not possible because we need to do shard routing. The redefinition of the throttle to limit pagestream request rate instead of repository `Key` rate comes with several downsides: - We're no longer able to use the throttle mechanism for other other tasks, e.g. image layer creation. However, in practice, we never used that capability anyways. - We no longer throttle basebackup.
This commit is contained in:
committed by
Ivan Efremov
parent
907e4aa3c4
commit
63cb8ce975
@@ -33,7 +33,9 @@ def test_pageserver_getpage_throttle(neon_env_builder: NeonEnvBuilder, pg_bin: P
|
||||
conf={
|
||||
"compaction_period": f"{compaction_period}s",
|
||||
"timeline_get_throttle": {
|
||||
"task_kinds": ["PageRequestHandler"],
|
||||
"task_kinds": [
|
||||
"PageRequestHandler"
|
||||
], # any non-empty array will do here https://github.com/neondatabase/neon/pull/9962
|
||||
"initial": 0,
|
||||
"refill_interval": "100ms",
|
||||
"refill_amount": int(rate_limit_rps / 10),
|
||||
@@ -116,7 +118,6 @@ def test_pageserver_getpage_throttle(neon_env_builder: NeonEnvBuilder, pg_bin: P
|
||||
timeout=compaction_period,
|
||||
)
|
||||
|
||||
log.info("the smgr metric includes throttle time")
|
||||
smgr_query_seconds_post = ps_http.get_metric_value(smgr_metric_name, smgr_metrics_query)
|
||||
assert smgr_query_seconds_post is not None
|
||||
throttled_usecs_post = ps_http.get_metric_value(throttle_metric_name, throttle_metrics_query)
|
||||
@@ -125,13 +126,14 @@ def test_pageserver_getpage_throttle(neon_env_builder: NeonEnvBuilder, pg_bin: P
|
||||
actual_throttled_usecs = throttled_usecs_post - throttled_usecs_pre
|
||||
actual_throttled_secs = actual_throttled_usecs / 1_000_000
|
||||
|
||||
log.info("validate that the metric doesn't include throttle wait time")
|
||||
assert (
|
||||
pytest.approx(duration_secs, 0.1) == actual_smgr_query_seconds
|
||||
), "smgr metrics include throttle wait time"
|
||||
smgr_ex_throttle = actual_smgr_query_seconds - actual_throttled_secs
|
||||
assert smgr_ex_throttle > 0
|
||||
duration_secs >= 10 * actual_smgr_query_seconds
|
||||
), "smgr metrics should not include throttle wait time"
|
||||
|
||||
log.info("validate that the throttling wait time metrics is correct")
|
||||
assert (
|
||||
duration_secs > 10 * smgr_ex_throttle
|
||||
pytest.approx(actual_throttled_secs + actual_smgr_query_seconds, 0.1) == duration_secs
|
||||
), "most of the time in this test is spent throttled because the rate-limit's contribution to latency dominates"
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user