feat: Add ASGI root path parameter to Phoenix server #3186

sbachstein · 2024-05-13T21:29:11Z

Allow for a root path parameter so that Phoenix can be hosted behind a proxy with a path prefix.

Session code has not been adjusted because the adjustment of the root path is probably not needed in this scenario.
Client and Exporter code have not been adjusted because they provide explicit endpoint parameters where a full URL can be given.

github-actions · 2024-05-13T21:29:24Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Allow for a root path parameter so that Phoenix can be hosted behind a proxy with a path prefix

sbachstein · 2024-05-14T08:41:52Z

I have read the CLA Document and I hereby sign the CLA

anticorrelator · 2024-05-14T18:29:08Z

src/phoenix/config.py

@@ -10,6 +10,7 @@
 ENV_PHOENIX_PORT = "PHOENIX_PORT"
 ENV_PHOENIX_GRPC_PORT = "PHOENIX_GRPC_PORT"
 ENV_PHOENIX_HOST = "PHOENIX_HOST"
+ENV_PHOENIX_ROOT_PATH = "PHOENIX_ROOT_PATH"


I think the changing env var name PHOENIX_HOST_ROOT_PATH is a bit better? We manage all of our configuration via environment variables and PHOENIX_ROOT_PATH might sound a little too generic

Agreed, should we match the CLI parameter then to also be --host-root-path or should we stick with the --root-path parameter as it is known from uvicorn, for example?

Went with the matching CLI parameter for now.

anticorrelator · 2024-05-14T18:30:13Z

Thanks so much for the contribution @sbachstein! This looks great, small request for a more specific config name

RogerHYang · 2024-05-14T23:48:37Z

The client needs to be updated, otherwise it'll have the wrong url.

e.g.

phoenix/src/phoenix/session/client.py

Line 244 in 5e8bc3d

urljoin(self._base_url, "/v1/traces"),

sbachstein · 2024-05-15T08:48:05Z

The client needs to be updated, otherwise it'll have the wrong url.

e.g.

phoenix/src/phoenix/session/client.py

Line 244 in 5e8bc3d

urljoin(self._base_url, "/v1/traces"),

The client provides the option to take the base path from the endpoint parameter (or environment variable). Do you think this is not sufficient and we need to, additionally, offer the option to specify HOST, PORT and ROOT_PATH?

RogerHYang · 2024-05-15T14:43:31Z

The issue is that urljoin is currently absolute. See the four scenarios below

sbachstein · 2024-05-15T16:59:41Z

Very good catch, thank you for the hint. I adjusted all usages of urljoin to avoid the absolute paths.

RogerHYang · 2024-05-15T20:59:43Z

Thanks for the update. Can we make sure that base_url always ends with /, otherwise the urljoin is still not correct.

abc below is gone because it doesn't end with /

>>> from urllib.parse import urljoin
>>> urljoin("http://xyz.com/abc", "rst")
'http://xyz.com/rst'

While we're at it, we could also add the root path as shown below.

self._base_url = ( 
    endpoint
    or get_env_collector_endpoint()
    or urljoin(f"http://{host}:{get_env_port()}", get_env_host_root_path())
).rstrip("/") + "/"

phoenix/src/phoenix/session/client.py

Lines 58 to 60 in 5e8bc3d

    
           self._base_url = ( 
        
               endpoint or get_env_collector_endpoint() or f"http://{host}:{get_env_port()}" 
        
           )

axiomofjoy · 2024-05-15T22:18:07Z

src/phoenix/server/main.py

@@ -119,6 +120,7 @@ def _get_pid_file() -> Path:
    parser.add_argument("--export_path")
    parser.add_argument("--host", type=str, required=False)
    parser.add_argument("--port", type=int, required=False)
+    parser.add_argument("--host-root-path", type=str, required=False)


Not sure we need the command line argument here in addition to the environment variable. We've been deprecating command line arguments such as --enable-prometheus that have environment variable equivalents if setting them via environment variables feels more natural, e.g., in Kubernetes. I think host root path probably falls into that category.

Agreed, I removed the CLI parameter.

CLI parameters for settings that are naturally adjusted via environment variables are discouraged for Phoenix

sbachstein · 2024-05-16T07:43:43Z

@RogerHYang Thank you for the hint again, that was kind of unexpected behavior for me, good to know!

anticorrelator · 2024-05-16T18:43:12Z

tests/session/test_client.py

@@ -18,6 +18,29 @@
 from phoenix.trace.trace_dataset import TraceDataset


+def test_base_path(monkeypatch: pytest.MonkeyPatch):


thank you for the test!

RogerHYang

I just tried this and I don't think the uvicorn implementation is correct.

At the minimum the traces endpoint is not advertised accurately.

anticorrelator · 2024-05-16T21:18:15Z

@sbachstein looks like the starlette app needs to be made aware of the root path too, thanks for checking this @RogerHYang

sbachstein · 2024-05-17T08:21:29Z

To me that is expected behavior. If the application is mounted at a root path with prefix by a proxy then the proxy would also make sure to rewrite the HTTP path and remove the root path from the request. That is also how I understand and have experienced the mechanisms of uvicorn/starlette root paths.

Here is how FastAPI explains the behavior of the root path: https://fastapi.tiangolo.com/advanced/behind-a-proxy/

RogerHYang

Thanks, this is LGTM. Sorry for the hold up.

sbachstein · 2024-05-17T22:16:46Z

Thank you for the thorough review!

axiomofjoy · 2024-05-17T23:19:23Z

@sbachstein Thanks for making Phoenix better!

dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label May 13, 2024

sbachstein force-pushed the feature/asgi_root_path branch 3 times, most recently from d388c72 to 9535709 Compare May 14, 2024 08:39

Add ASGI root path parameter to Phoenix server

4779edd

Allow for a root path parameter so that Phoenix can be hosted behind a proxy with a path prefix

sbachstein force-pushed the feature/asgi_root_path branch from 9535709 to 4779edd Compare May 14, 2024 08:40

github-actions bot added a commit that referenced this pull request May 14, 2024

@sbachstein has signed the CLA in #3186

2bc21eb

sbachstein changed the title ~~Add ASGI root path parameter to Phoenix server~~ feat: Add ASGI root path parameter to Phoenix server May 14, 2024

mikeldking requested a review from anticorrelator May 14, 2024 18:25

anticorrelator reviewed May 14, 2024

View reviewed changes

Name root path parameter more specifically

cd781ce

sbachstein added 2 commits May 15, 2024 18:56

Fix welcome message

3d99fc0

Enable client code to handle endpoints with path prefix

4db37a3

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels May 15, 2024

axiomofjoy reviewed May 15, 2024

View reviewed changes

sbachstein added 3 commits May 16, 2024 09:35

Fix wrong endpoint in evaluation module

365cfe5

Guarantee training slashes in host base URLs

42aaa35

Remove host root path CLI parameter

f32ac35

CLI parameters for settings that are naturally adjusted via environment variables are discouraged for Phoenix

Apply pre-commit fixes

6890a90

sbachstein force-pushed the feature/asgi_root_path branch from 169f514 to 6890a90 Compare May 16, 2024 09:21

Fix and adjust unit tests

bbbae7a

anticorrelator reviewed May 16, 2024

View reviewed changes

anticorrelator approved these changes May 16, 2024

View reviewed changes

RogerHYang requested changes May 16, 2024

View reviewed changes

RogerHYang approved these changes May 17, 2024

View reviewed changes

RogerHYang merged commit e27cc5d into Arize-ai:main May 17, 2024
8 checks passed

github-actions bot mentioned this pull request May 17, 2024

chore(main): release arize-phoenix 4.1.0 #3194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ASGI root path parameter to Phoenix server #3186

feat: Add ASGI root path parameter to Phoenix server #3186

sbachstein commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

sbachstein commented May 14, 2024

anticorrelator May 14, 2024

sbachstein May 15, 2024 •

edited

sbachstein May 15, 2024

anticorrelator commented May 14, 2024

RogerHYang commented May 14, 2024

sbachstein commented May 15, 2024

RogerHYang commented May 15, 2024

sbachstein commented May 15, 2024

RogerHYang commented May 15, 2024 •

edited

axiomofjoy May 15, 2024

sbachstein May 16, 2024

sbachstein commented May 16, 2024

anticorrelator May 16, 2024

RogerHYang left a comment

anticorrelator commented May 16, 2024

sbachstein commented May 17, 2024 •

edited

RogerHYang left a comment

sbachstein commented May 17, 2024

axiomofjoy commented May 17, 2024

		@@ -18,6 +18,29 @@
		from phoenix.trace.trace_dataset import TraceDataset


		def test_base_path(monkeypatch: pytest.MonkeyPatch):

feat: Add ASGI root path parameter to Phoenix server #3186

feat: Add ASGI root path parameter to Phoenix server #3186

Conversation

sbachstein commented May 13, 2024 • edited

github-actions bot commented May 13, 2024 • edited

sbachstein commented May 14, 2024

anticorrelator May 14, 2024

Choose a reason for hiding this comment

sbachstein May 15, 2024 • edited

Choose a reason for hiding this comment

sbachstein May 15, 2024

Choose a reason for hiding this comment

anticorrelator commented May 14, 2024

RogerHYang commented May 14, 2024

sbachstein commented May 15, 2024

RogerHYang commented May 15, 2024

sbachstein commented May 15, 2024

RogerHYang commented May 15, 2024 • edited

axiomofjoy May 15, 2024

Choose a reason for hiding this comment

sbachstein May 16, 2024

Choose a reason for hiding this comment

sbachstein commented May 16, 2024

anticorrelator May 16, 2024

Choose a reason for hiding this comment

RogerHYang left a comment

Choose a reason for hiding this comment

anticorrelator commented May 16, 2024

sbachstein commented May 17, 2024 • edited

RogerHYang left a comment

Choose a reason for hiding this comment

sbachstein commented May 17, 2024

axiomofjoy commented May 17, 2024

sbachstein commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

sbachstein May 15, 2024 •

edited

RogerHYang commented May 15, 2024 •

edited

sbachstein commented May 17, 2024 •

edited