Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(traces): configurable endpoint for the exporter #1795

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

mikeldking
Copy link
Contributor

@mikeldking mikeldking commented Nov 21, 2023

resolves #1793

This PR introduces an endpoint config for the exporter so that we don't assume that the phoenix server is running locally. This is needed for various use-cases:

Phoenix can now be running on an EC2 instance or other
Phoenix might have to be running in Sagemaker studio in a separate process (Networking failing right now but in theory this might be the configuration)

@mikeldking mikeldking changed the title feat(traces): configurable endpoint for the exporter feat(traces): configurable endpoint for the trace exporter Nov 21, 2023
Copy link
Contributor

@axiomofjoy axiomofjoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring for HttpExporter should specify that endpoint supersedes host and port, so there is no need to specify a host and port if you've already provided an endpoint.



def get_env_trace_endpoint() -> Optional[str]:
return os.getenv(ENV_TRACE_ENDPOINT) or None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return os.getenv(ENV_TRACE_ENDPOINT) or None
return os.getenv(ENV_TRACE_ENDPOINT)

os.getenv returns None if the environment variable can't be found.

@@ -7,6 +7,7 @@
ENV_PHOENIX_PORT = "PHOENIX_PORT"
ENV_PHOENIX_HOST = "PHOENIX_HOST"
ENV_NOTEBOOK_ENV = "PHOENIX_NOTEBOOK_ENV"
ENV_TRACE_ENDPOINT = "PHOENIX_TRACE_ENDPOINT"
Copy link
Contributor

@RogerHYang RogerHYang Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep it generic?

Suggested change
ENV_TRACE_ENDPOINT = "PHOENIX_TRACE_ENDPOINT"
ENV_PHOENIX_RECEIVER_ENDPOINT = "PHOENIX_RECEIVER_ENDPOINT"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah evals. Was copying OTEL naming but maybe I'll just call it collector

@mikeldking mikeldking changed the title feat(traces): configurable endpoint for the trace exporter feat(traces): configurable endpoint for the exporter Nov 21, 2023
@mikeldking mikeldking merged commit 8515763 into main Nov 21, 2023
9 checks passed
@mikeldking mikeldking deleted the 1793-exporter-endpoint-config branch November 21, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[ENHANCEMENT] Add the ability to export spans to a remote collector
3 participants