[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus #49353

rueian · 2024-12-19T04:13:25Z

Why are these changes needed?

Some users from Slack channels have an authentication proxy in front of their Prometheus server and that proxy requires requests to have specific HTTP headers to pass. Currently, we don't have a way for them to customize headers for those requests.

This PR introduces a new RAY_PROMETHEUS_HEADERS environment variable, next to the existing RAY_PROMETHEUS_HOST, to allow users to add custom headers for requests to Prometheus from Ray Dashboard and Grafana (ref)

The format of RAY_PROMETHEUS_HEADERS should be a JSON string in one of the following:

{"H1": "V1", "H2": "V2"}
[["H1", "V1"], ["H2", "V2"], ["H2", "V3"]]

The second format supports multiple headers with the same name (which is valid by HTTP spec) while the first format doesn't due to the aiohttp limitation. i.e. the format like{"H2": ["V2", "V3"]} is not supported.

A Grafana datasource.yml example of the first format

A Grafana datasource.yml example of the second format

A request dump example from Ray Dashboard to Prometheus

A request dump example from Grafana to Prometheus

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

rueian · 2024-12-21T03:25:24Z

python/ray/dashboard/modules/metrics/templates.py

-GRAFANA_DATASOURCE_TEMPLATE = """apiVersion: 1

-datasources:
-  - name: {prometheus_name}
-    url: {prometheus_host}
-    type: prometheus
-    isDefault: true
-    access: proxy
-"""
+def GRAFANA_DATASOURCE_TEMPLATE(
+    prometheus_name, prometheus_host, jsonData, secureJsonData
+):
+    return yaml.safe_dump(
+        {
+            "apiVersion": 1,
+            "datasources": [
+                {
+                    "name": prometheus_name,
+                    "url": prometheus_host,
+                    "type": "prometheus",
+                    "isDefault": True,
+                    "access": "proxy",
+                    "jsonData": jsonData,
+                    "secureJsonData": secureJsonData,
+                }
+            ],
+        }
+    )


I think using yaml.safe_dump is better because a simple string template is now not enough for complex values in jsonData and secureJsonData and there might also be some values that require escaping to fit into a yaml file.

kevin85421

I am not familiar with our Prometheus setup, but the Ray head node has several metric endpoints, including 8080, Autoscaler metrics, and Dashboard metrics. Are they in separate codepaths?

https://github.com/ray-project/kuberay/blob/a788963f36bee9e0c5b7b58a20be0c9ae10f3b04/config/prometheus/podMonitor.yaml#L46-L63

kevin85421 · 2024-12-27T22:43:02Z

release/ray_release/command_runner/_prometheus_metrics.py

@@ -13,6 +13,8 @@

 DEFAULT_PROMETHEUS_HOST = "http://localhost:9090"
 PROMETHEUS_HOST_ENV_VAR = "RAY_PROMETHEUS_HOST"
+DEFAULT_PROMETHEUS_HEADERS = "{}"
+PROMETHEUS_HEADERS_ENV_VAR = "RAY_PROMETHEUS_HEADERS"


Can you also add the comments about the format here?

I have added the comment below and next to the parse_prom_headers function.

I am not familiar with our Prometheus setup, but the Ray head node has several metric endpoints, including 8080, Autoscaler metrics, and Dashboard metrics. Are they in separate codepaths?

Yes, they are in the different code paths. Prometheus will periodically scrape metrics from these metrics endpoints, while this PR targets the endpoints of the Prometheus server.

alanwguo · 2024-12-27T23:50:50Z

python/ray/dashboard/modules/metrics/metrics_head.py

@@ -73,6 +76,10 @@ def __init__(self, dashboard_head):
        self.prometheus_host = os.environ.get(
            PROMETHEUS_HOST_ENV_VAR, DEFAULT_PROMETHEUS_HOST
        )
+        self.prometheus_headers = os.environ.get(


can we do some basic validation of the format before using the env var?

A new parse_prom_headers function is added for both validation and parsing. This is what it looks like when the validation fails:

▶ export RAY_PROMETHEUS_HEADERS='[["XAuth"],["XAuth2"]]' ▶ ray start --head --include-dashboard=true --metrics-export-port=8080 Local node IP: 127.0.0.1 2024-12-28 13:32:56,919 ERROR services.py:1353 -- Failed to start the dashboard , return code 1 2024-12-28 13:32:56,920 ERROR services.py:1378 -- Error should be written to 'dashboard.log' or 'dashboard.err'. We are printing the last 20 lines for you. See 'https://docs.ray.io/en/master/ray-observability/user-guides/configure-logging.html#logging-directory-structure' to find where the log file is. Traceback (most recent call last): ... File "/Users/ruian/Code/python/ray/python/ray/dashboard/modules/metrics/metrics_head.py", line 97, in __init__ self.prometheus_headers = parse_prom_headers( ^^^^^^^^^^^^^^^^^^^ File "/Users/ruian/Code/python/ray/python/ray/dashboard/modules/metrics/metrics_head.py", line 74, in parse_prom_headers raise ValueError( ValueError: RAY_PROMETHEUS_HEADERS should be a JSON string in one of the formats: 1) An object with string keys and string values. 2) an array of string arrays with 2 string elements each.

…aders to Prometheus Signed-off-by: Rueian <[email protected]>

Signed-off-by: Rueian <[email protected]>

kevin85421

Should we add a test?

kevin85421 · 2025-01-02T22:17:43Z

python/ray/dashboard/modules/metrics/metrics_head.py

+            return parsed
+    raise ValueError(
+        f"{PROMETHEUS_HEADERS_ENV_VAR} should be a JSON string in one of the formats:\n"
+        + "1) An object with string keys and string values.\n"


can you add the example to the ValueError?

kevin85421 · 2025-01-02T22:24:27Z

release/ray_release/command_runner/_prometheus_metrics.py

+# parse_prom_headers will make sure the input is in one of the following formats:
+# 1. {"H1": "V1", "H2": "V2"}
+# 2. [["H1", "V1"], ["H2", "V2"], ["H2", "V3"]]
+def parse_prom_headers(prometheus_headers):


This file seems to be highly similar to metrics_head.py. What are the differences between them? Perhaps @alanwguo knows the difference.

As far as I can tell, this file is used at:

ray/release/ray_release/command_runner/_anyscale_job_wrapper.py

Lines 120 to 130 in b25c72c

subprocess.run(

[

"python",

"prometheus_metrics.py",

str(time_taken),

"--path",

os.environ["METRICS_OUTPUT_JSON"],

],

timeout=metrics_timeout,

check=True,

)

and

ray/release/ray_release/command_runner/job_runner.py

Lines 81 to 84 in b25c72c

def save_metrics(self, start_time: float, timeout: float = 900):

self.run_prepare_command(

f"python prometheus_metrics.py {start_time}", timeout=timeout

)

According to their commit messages, these usages are related to CI procedures.

It also looks like the script intentionally does not import Ray, therefore, I copied the parse_prom_headers function from metrics_head.py to here instead of importing it. But if we don't need this in the CI procedures, I will just remove the change. Do we need this in the CI procedures?

I have removed the change from the _prometheus_metrics.py. It seems we will never need this feature in the release procedures.

…sage Signed-off-by: Rueian <[email protected]>

rueian · 2025-01-03T04:58:50Z

Should we add a test?

Sure! How about adding the test to python/ray/dashboard/tests/test_dashboard.py? I have added one there and used the DashboardHead._load_modules(), which is responsible for loading the MetricsHead and the DataHead, to test the validation.

Signed-off-by: Rueian <[email protected]>

…trics.py release tool Signed-off-by: Rueian <[email protected]>

kevin85421 · 2025-01-03T19:29:02Z

python/ray/dashboard/tests/test_dashboard.py

+
+    # Test the unsupported case.
+    with pytest.raises(ValueError):
+        os.environ[PROMETHEUS_HEADERS_ENV_VAR] = '{"H1": "V1", "H2": ["V1", "V2"]}'


maybe use monkeypatch.setenv to avoid env var leak

Thanks for the hint. Updated.

Signed-off-by: Rueian <[email protected]>

kevin85421 · 2025-01-04T05:32:20Z

Trigger the whole CI tests.

…in minimal tests Signed-off-by: Rueian <[email protected]>

rueian · 2025-01-04T19:05:05Z

Trigger the whole CI tests.

Hi @kevin85421, the previous premerge check failed on minimal installation tests and I pushed a fix for that. Could you help trigger the whole CI test again? Thank you.

kevin85421 · 2025-01-05T04:59:30Z

@rueian With the go label on the PR, all tests will be executed.

…aders to Prometheus (ray-project#49353) Signed-off-by: Rueian <[email protected]>

…aders to Prometheus (ray-project#49353) Signed-off-by: Rueian <[email protected]> Signed-off-by: Roshan Kathawate <[email protected]>

…aders to Prometheus (ray-project#49353) Signed-off-by: Rueian <[email protected]> Signed-off-by: Puyuan Yao <[email protected]>

rueian force-pushed the dashboard-prom-headers branch 2 times, most recently from b667fec to 4b9357a Compare December 19, 2024 13:52

kevin85421 self-assigned this Dec 19, 2024

rueian force-pushed the dashboard-prom-headers branch 2 times, most recently from a1b293a to 8e42e75 Compare December 20, 2024 06:20

rueian changed the title ~~[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus (#2669)~~ [Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus Dec 21, 2024

rueian force-pushed the dashboard-prom-headers branch from 8e42e75 to 2b39527 Compare December 21, 2024 02:25

rueian commented Dec 21, 2024

View reviewed changes

rueian marked this pull request as ready for review December 21, 2024 03:25

rueian requested a review from a team as a code owner December 21, 2024 03:25

rueian mentioned this pull request Dec 21, 2024

[Prometheus][Ray dashboard] Make the headers of requests from Ray to RAY_PROMETHEUS_HOST configurable ray-project/kuberay#2669

Closed

2 tasks

jcotant1 added observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling dashboard Issues specific to the Ray Dashboard labels Dec 23, 2024

kevin85421 assigned MengjinYan and alanwguo Dec 27, 2024

kevin85421 reviewed Dec 27, 2024

View reviewed changes

alanwguo reviewed Dec 27, 2024

View reviewed changes

rueian added 2 commits December 28, 2024 11:30

[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional he…

a027ac4

…aders to Prometheus Signed-off-by: Rueian <[email protected]>

[Dashboard] validate RAY_PROMETHEUS_HEADERS env

7a654e6

Signed-off-by: Rueian <[email protected]>

rueian force-pushed the dashboard-prom-headers branch from 2b39527 to 7a654e6 Compare December 28, 2024 05:35

rueian requested a review from alanwguo December 28, 2024 13:44

kevin85421 reviewed Jan 2, 2025

View reviewed changes

[Dashboard] add examples to the RAY_PROMETHEUS_HEADERS validation mes…

af1946c

…sage Signed-off-by: Rueian <[email protected]>

rueian force-pushed the dashboard-prom-headers branch from b630a53 to d8d1e53 Compare January 3, 2025 04:59

[Dashboard] Test the validation of the RAY_PROMETHEUS_HEADERS env

be3b9e5

Signed-off-by: Rueian <[email protected]>

rueian force-pushed the dashboard-prom-headers branch from d8d1e53 to be3b9e5 Compare January 3, 2025 05:27

[Dashboard] Remove the RAY_PROMETHEUS_HEADERS env from _prometheus_me…

cf036ee

…trics.py release tool Signed-off-by: Rueian <[email protected]>

alanwguo approved these changes Jan 3, 2025

View reviewed changes

kevin85421 approved these changes Jan 3, 2025

View reviewed changes

[Dashboard] Test the validation of the RAY_PROMETHEUS_HEADERS env

626e098

Signed-off-by: Rueian <[email protected]>

rueian force-pushed the dashboard-prom-headers branch from 1c19a57 to 626e098 Compare January 4, 2025 02:56

kevin85421 added the go add ONLY when ready to merge, run all tests label Jan 4, 2025

[Dashboard] do not import ray.dashboard.modules.metrics.metrics_head …

17724ad

…in minimal tests Signed-off-by: Rueian <[email protected]>

jjyao merged commit d37edf9 into ray-project:master Jan 5, 2025
5 checks passed

roshankathawate pushed a commit to roshankathawate/ray that referenced this pull request Jan 7, 2025

[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional he…

756d0ea

…aders to Prometheus (ray-project#49353) Signed-off-by: Rueian <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus #49353

[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus #49353

rueian commented Dec 19, 2024 •

edited

Loading

rueian Dec 21, 2024 •

edited

Loading

kevin85421 left a comment

kevin85421 Dec 27, 2024

rueian Dec 28, 2024

rueian Dec 28, 2024

alanwguo Dec 27, 2024

rueian Dec 28, 2024

kevin85421 left a comment

kevin85421 Jan 2, 2025

rueian Jan 3, 2025

kevin85421 Jan 2, 2025

rueian Jan 3, 2025

rueian Jan 3, 2025

rueian commented Jan 3, 2025 •

edited

Loading

kevin85421 Jan 3, 2025

rueian Jan 4, 2025

kevin85421 commented Jan 4, 2025

rueian commented Jan 4, 2025

kevin85421 commented Jan 5, 2025

	subprocess.run(
	[
	"python",
	"prometheus_metrics.py",
	str(time_taken),
	"--path",
	os.environ["METRICS_OUTPUT_JSON"],
	],
	timeout=metrics_timeout,
	check=True,
	)

	def save_metrics(self, start_time: float, timeout: float = 900):
	self.run_prepare_command(
	f"python prometheus_metrics.py {start_time}", timeout=timeout
	)

[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus #49353

[Dashboard] add RAY_PROMETHEUS_HEADERS env for carrying additional headers to Prometheus #49353

Conversation

rueian commented Dec 19, 2024 • edited Loading

Why are these changes needed?

A Grafana datasource.yml example of the first format

A Grafana datasource.yml example of the second format

A request dump example from Ray Dashboard to Prometheus

A request dump example from Grafana to Prometheus

Related issue number

Checks

rueian Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Jan 4, 2025

rueian commented Jan 4, 2025

kevin85421 commented Jan 5, 2025

rueian commented Dec 19, 2024 •

edited

Loading

rueian Dec 21, 2024 •

edited

Loading

rueian commented Jan 3, 2025 •

edited

Loading