Repository: ray-project/ray
Pull request: make latency buckets configurable
## Why are these changes needed?

The choice of bucket boundaries in Prometheus is an application-level decision. Well-defined boundaries lead to more accurate quantile calculations, while poor choices can result in less precise estimates. 

This PR exposes bucket boundary configuration to users via two environment variables:  
- `REQUEST_LATENCY_BUCKET_MS`  
- `MODEL_LOAD_LATENCY_BUCKET_MS`  

This allows users to fine-tune latency measurement based on their needs.

Change is backwards compatible

See #38223 for a more detailed rationale.  

## Related Issue

Fixes #38223  

## Checks

- Added unit tests.  


Problem statement:
[serve] Make latency buckets configurable
### Description

The latency buckets used for the latency histogram heavily affect how we calculate P50, P90 etc latency on Grafana. We should make this configurable so users who know roughly how long their requests will take can modify the buckets to match that.

### Use case

_No response_

Hints from issue discussion:
## Prometheus Histogram Boundaries  

The Prometheus histogram boundaries are fetched from `DEFAULT_LATENCY_BUCKET_MS` in `constants.py`.  

### Current Usage of `Histogram` Metrics  
The `Histogram` metric is used in the following scenarios:  

1. **Model Load/Unload Times** – Tracks the time taken to load/unload multiplexed models.  
2. **Proxy-Level Request Latency** – Measures end-to-end request/response latency at the proxy, including both model inference time and infrastructure routing cost.  
3. **Replica-Level Request Latency** – Measures end-to-end request/response latency at the replica, primarily reflecting the isolated cost of model inference.  

### Decisions  

- Metrics **2 and 3** share similar characteristics in terms of histogram boundary definitions. A single configuration should control both.  
- Metric **1** is significantly different from **2 and 3**, so it requires a separate configuration for its histogram boundaries.  
- To maintain **backward compatibility**, `DEFAULT_LATENCY_BUCKET_MS` should be preserved for existing histogram-based metrics.  
- Introduce two new properties:  
  - `REQUEST_LATENCY_BUCKET_MS` for **metrics 2 and 3**.  
  - `MODEL_LOAD_LATENCY_BUCKET_MS` for **metric 1**.  

### Question  

Should the new properties be exposed via **environment variables** or as part of the **runtime configuration**, accessible through the public `deployment` decorator?  
My take is that: since we want to discourage(also its unlikely they are going to change this) users from changing the boundaries, its better is these lived as environment variable or some static config file (if one already exists)

@abrarsheikh I agree that it should be configurable through environment variables instead of the deployment configuration. It's unlikely that users will often change this. Introducing two separate env vars `REQUEST_LATENCY_BUCKET_MS` and `MODEL_LOAD_LATENCY_BUCKET_MS` sgtm!

Commit message: make latency buckets configurable

Signed-off-by: Abrar Sheikh <abrar2002as@gmail.com>

Patch to review:
diff --git a/python/ray/serve/_private/constants.py b/python/ray/serve/_private/constants.py
index ff7c292a5d945..46b13b5cbc6b2 100644
--- a/python/ray/serve/_private/constants.py
+++ b/python/ray/serve/_private/constants.py
@@ -49,6 +49,11 @@
     os.environ.get("MAX_DEPLOYMENT_CONSTRUCTOR_RETRY_COUNT", "20")
 )
 
+# If you are wondering why we are using histogram buckets, please refer to
+# https://prometheus.io/docs/practices/histograms/
+# short answer is that its cheaper to calculate percentiles on the histogram
+# than to calculate them on raw data, both in terms of time and space.
+
 #: Default histogram buckets for latency tracker.
 DEFAULT_LATENCY_BUCKET_MS = [
     1,
@@ -78,6 +83,39 @@
     600000,
 ]
 
+
+def _parse_latency_buckets(bucket_str: str) -> list:
+    if not bucket_str.strip():
+        return DEFAULT_LATENCY_BUCKET_MS
+    try:
+        # Convert string to list of floats
+        buckets = [float(x.strip()) for x in bucket_str.split(",")]
+        if not buckets:
+            raise ValueError("Empty bucket list")
+        if any(x <= 0 for x in buckets):
+            raise ValueError("Bucket values must be positive")
+        if sorted(set(buckets)) != buckets:
+            raise ValueError("Bucket values must be in ascending order and unique")
+        return buckets
+    except Exception as e:
+        raise ValueError(
+            f"Invalid format for {bucket_str}. "
+            f"Expected comma-separated positive numbers in ascending order. Error: {str(e)}"
+        )
+
+
+# Example usage:
+# RAY_SERVE_REQUEST_LATENCY_BUCKET_MS="1,2,3,4"
+# RAY_SERVE_MODEL_LOAD_LATENCY_BUCKET_MS="1,2,3,4"
+#: Histogram buckets for request latency.
+REQUEST_LATENCY_BUCKET_MS = _parse_latency_buckets(
+    os.getenv("REQUEST_LATENCY_BUCKET_MS", "")
+)
+#: Histogram buckets for model load/unload latency.
+MODEL_LOAD_LATENCY_BUCKET_MS = _parse_latency_buckets(
+    os.getenv("MODEL_LOAD_LATENCY_BUCKET_MS", "")
+)
+
 #: Name of deployment health check method implemented by user.
 HEALTH_CHECK_METHOD = "check_health"
 
diff --git a/python/ray/serve/_private/proxy.py b/python/ray/serve/_private/proxy.py
index b6f9c23fc1177..4f81eec864343 100644
--- a/python/ray/serve/_private/proxy.py
+++ b/python/ray/serve/_private/proxy.py
@@ -27,11 +27,11 @@
     RequestProtocol,
 )
 from ray.serve._private.constants import (
-    DEFAULT_LATENCY_BUCKET_MS,
     PROXY_MIN_DRAINING_PERIOD_S,
     RAY_SERVE_ENABLE_PROXY_GC_OPTIMIZATIONS,
     RAY_SERVE_HTTP_PROXY_CALLBACK_IMPORT_PATH,
     RAY_SERVE_PROXY_GC_THRESHOLD,
+    REQUEST_LATENCY_BUCKET_MS,
     SERVE_CONTROLLER_NAME,
     SERVE_LOGGER_NAME,
     SERVE_MULTIPLEXED_MODEL_ID,
@@ -182,13 +182,15 @@ def __init__(
             ),
         )
 
+        # log REQUEST_LATENCY_BUCKET_MS
+        logger.debug(f"REQUEST_LATENCY_BUCKET_MS: {REQUEST_LATENCY_BUCKET_MS}")
         self.processing_latency_tracker = metrics.Histogram(
             f"serve_{self.protocol.lower()}_request_latency_ms",
             description=(
                 f"The end-to-end latency of {self.protocol} requests "
                 f"(measured from the Serve {self.protocol} proxy)."
             ),
-            boundaries=DEFAULT_LATENCY_BUCKET_MS,
+            boundaries=REQUEST_LATENCY_BUCKET_MS,
             tag_keys=(
                 "method",
                 "route",
diff --git a/python/ray/serve/_private/replica.py b/python/ray/serve/_private/replica.py
index c9320c6c77b4a..2fe8da44d079f 100644
--- a/python/ray/serve/_private/replica.py
+++ b/python/ray/serve/_private/replica.py
@@ -46,7 +46,6 @@
 )
 from ray.serve._private.config import DeploymentConfig
 from ray.serve._private.constants import (
-    DEFAULT_LATENCY_BUCKET_MS,
     GRPC_CONTEXT_ARG_NAME,
     HEALTH_CHECK_METHOD,
     RAY_SERVE_COLLECT_AUTOSCALING_METRICS_ON_HANDLE,
@@ -55,6 +54,7 @@
     RAY_SERVE_RUN_SYNC_IN_THREADPOOL,
     RAY_SERVE_RUN_SYNC_IN_THREADPOOL_WARNING,
     RECONFIGURE_METHOD,
+    REQUEST_LATENCY_BUCKET_MS,
     SERVE_CONTROLLER_NAME,
     SERVE_LOGGER_NAME,
     SERVE_NAMESPACE,
@@ -174,10 +174,12 @@ def __init__(
         if self._cached_metrics_enabled:
             self._cached_error_counter = defaultdict(int)
 
+        # log REQUEST_LATENCY_BUCKET_MS
+        logger.debug(f"REQUEST_LATENCY_BUCKET_MS: {REQUEST_LATENCY_BUCKET_MS}")
         self._processing_latency_tracker = metrics.Histogram(
             "serve_deployment_processing_latency_ms",
             description="The latency for queries to be processed.",
-            boundaries=DEFAULT_LATENCY_BUCKET_MS,
+            boundaries=REQUEST_LATENCY_BUCKET_MS,
             tag_keys=("route",),
         )
         if self._cached_metrics_enabled:
diff --git a/python/ray/serve/multiplex.py b/python/ray/serve/multiplex.py
index 410f627a08879..ca46d30b8f74e 100644
--- a/python/ray/serve/multiplex.py
+++ b/python/ray/serve/multiplex.py
@@ -8,7 +8,7 @@
 from ray.serve import metrics
 from ray.serve._private.common import MultiplexedReplicaInfo
 from ray.serve._private.constants import (
-    DEFAULT_LATENCY_BUCKET_MS,
+    MODEL_LOAD_LATENCY_BUCKET_MS,
     PUSH_MULTIPLEXED_MODEL_IDS_INTERVAL_S,
     SERVE_LOGGER_NAME,
 )
@@ -58,15 +58,17 @@ def __init__(
         self.self_arg: Any = self_arg
         self.max_num_models_per_replica: int = max_num_models_per_replica
 
+        # log MODEL_LOAD_LATENCY_BUCKET_MS
+        logger.debug(f"MODEL_LOAD_LATENCY_BUCKET_MS: {MODEL_LOAD_LATENCY_BUCKET_MS}")
         self.model_load_latency_ms = metrics.Histogram(
             "serve_multiplexed_model_load_latency_ms",
             description="The time it takes to load a model.",
-            boundaries=DEFAULT_LATENCY_BUCKET_MS,
+            boundaries=MODEL_LOAD_LATENCY_BUCKET_MS,
         )
         self.model_unload_latency_ms = metrics.Histogram(
             "serve_multiplexed_model_unload_latency_ms",
             description="The time it takes to unload a model.",
-            boundaries=DEFAULT_LATENCY_BUCKET_MS,
+            boundaries=MODEL_LOAD_LATENCY_BUCKET_MS,
         )
         self.num_models_gauge = metrics.Gauge(
             "serve_num_multiplexed_models",
diff --git a/python/ray/serve/tests/test_constants.py b/python/ray/serve/tests/test_constants.py
new file mode 100644
index 0000000000000..5e288fd434c9a
--- /dev/null
+++ b/python/ray/serve/tests/test_constants.py
@@ -0,0 +1,45 @@
+import pytest
+from ray.serve._private.constants import (
+    DEFAULT_LATENCY_BUCKET_MS,
+    _parse_latency_buckets,
+)
+
+
+def test_parse_latency_buckets():
+    # Test empty string returns default buckets
+    assert _parse_latency_buckets("") == DEFAULT_LATENCY_BUCKET_MS
+
+    # Test valid inputs with different formats
+    assert _parse_latency_buckets("1,2,3") == [1.0, 2.0, 3.0]
+    assert _parse_latency_buckets("1,2,3,4 ") == [1.0, 2.0, 3.0, 4.0]
+    assert _parse_latency_buckets("  1,2,3,4,5") == [1.0, 2.0, 3.0, 4.0, 5.0]
+    assert _parse_latency_buckets(" 1, 2,3  ,4,5 ,6 ") == [1.0, 2.0, 3.0, 4.0, 5.0, 6.0]
+
+    # Test decimal numbers
+    assert _parse_latency_buckets("0.5,1.5,2.5") == [0.5, 1.5, 2.5]
+
+
+def test_parse_latency_buckets_invalid():
+    # Test negative numbers
+    with pytest.raises(ValueError, match=".*must be positive.*"):
+        _parse_latency_buckets("-1,1,2,3,4")
+
+    # Test non-ascending order
+    with pytest.raises(ValueError, match=".*must be in ascending order.*"):
+        _parse_latency_buckets("4,3,2,1")
+
+    # Test duplicate values
+    with pytest.raises(ValueError, match=".*must be in ascending order.*"):
+        _parse_latency_buckets("1,2,2,3,4")
+
+    # Test invalid number format
+    with pytest.raises(ValueError, match=".*Invalid.*format.*"):
+        _parse_latency_buckets("1,2,3,4,a")
+
+    # Test empty list
+    with pytest.raises(ValueError, match=".*could not convert.*"):
+        _parse_latency_buckets(",,,")
+
+    # Test invalid separators
+    with pytest.raises(ValueError, match=".*could not convert.*"):
+        _parse_latency_buckets("1;2;3;4")


Code review comments:
diff_hunk: @@ -78,6 +83,39 @@
     600000,
 ]
 
+
+def _parse_latency_buckets(bucket_str: str) -> list:
+    if not bucket_str.strip():
+        return DEFAULT_LATENCY_BUCKET_MS
path: python/ray/serve/_private/constants.py
text: @user1:
```suggestion
def _parse_latency_buckets(bucket_str: Optional[str]) -> List[float]:
    if bucket_str is None:
        return DEFAULT_LATENCY_BUCKET_MS
```
nit: `None` typically best practice for defaulting behavior (would also need to make the default for env var `None`)

diff_hunk: @@ -78,6 +83,39 @@
     600000,
 ]
 
+
+def _parse_latency_buckets(bucket_str: str) -> list:
+    if not bucket_str.strip():
+        return DEFAULT_LATENCY_BUCKET_MS
+    try:
+        # Convert string to list of floats
+        buckets = [float(x.strip()) for x in bucket_str.split(",")]
+        if not buckets:
+            raise ValueError("Empty bucket list")
+        if any(x <= 0 for x in buckets):
+            raise ValueError("Bucket values must be positive")
+        if sorted(set(buckets)) != buckets:
+            raise ValueError("Bucket values must be in ascending order and unique")
+        return buckets
+    except Exception as e:
+        raise ValueError(
+            f"Invalid format for {bucket_str}. "
+            f"Expected comma-separated positive numbers in ascending order. Error: {str(e)}"
+        )
+
+
+# Example usage:
+# RAY_SERVE_REQUEST_LATENCY_BUCKET_MS="1,2,3,4"
+# RAY_SERVE_MODEL_LOAD_LATENCY_BUCKET_MS="1,2,3,4"
+#: Histogram buckets for request latency.
+REQUEST_LATENCY_BUCKET_MS = _parse_latency_buckets(
+    os.getenv("REQUEST_LATENCY_BUCKET_MS", "")
+)
+#: Histogram buckets for model load/unload latency.
+MODEL_LOAD_LATENCY_BUCKET_MS = _parse_latency_buckets(
+    os.getenv("MODEL_LOAD_LATENCY_BUCKET_MS", "")
+)
path: python/ray/serve/_private/constants.py
text: @user1:
```suggestion
REQUEST_LATENCY_BUCKETS_MS = _parse_latency_buckets(
    os.getenv("REQUEST_LATENCY_BUCKET_MS", "")
)
#: Histogram buckets for model load/unload latency.
MODEL_LOAD_LATENCY_BUCKETS_MS = _parse_latency_buckets(
    os.getenv("MODEL_LOAD_LATENCY_BUCKET_MS", "")
)
```