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: Improve OTel wrapper ergonomics #4295

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

anticorrelator
Copy link
Contributor

  • The high-level OTel wrappers will now print a summary of the TracerProvider that has been configured
  • Defaults Phoenix endpoint to: http://localhost:6006
  • Better handling of endpoints set via environment variables, defaults to using GRPC transport for spans
  • Fixes small bugs

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 20, 2024
Comment on lines +140 to +143
if os.name == "nt":
details_header = "OpenTelemetry Tracing Details"
else:
details_header = "🔭 OpenTelemetry Tracing Details 🔭"
Copy link
Contributor

Choose a reason for hiding this comment

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

should the guard be just for windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current guard is only for Windows, are we worried about anything else? (cc @RogerHYang)

Copy link
Contributor

@mikeldking mikeldking left a comment

Choose a reason for hiding this comment

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

LGTM - will try it out tomorrow.


def register(
*,
endpoint: Optional[str] = None,
project_name: Optional[str] = None,
batch: bool = False,
set_global=True,
headers=None,
set_global_tracer: bool = True,
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
set_global_tracer: bool = True,
set_global_tracer_provider: bool = True,

I guess more explicit maybe

set_global=True,
headers=None,
set_global_tracer: bool = True,
headers: Optional[Dict[str, str]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should verbose be plumbed through here too?

return exporter.__class__.__name__


def _normalize_headers(headers: Union[List[Tuple[str, str]], Dict[str, str]]) -> Dict[str, str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Normalize is probably not the right word. It's just for printing so maybe something like

def get_printable_db_url(connection_str: str) -> str:

@anticorrelator anticorrelator merged commit ef533cf into main Aug 21, 2024
9 checks passed
@anticorrelator anticorrelator deleted the dustin/improve-otel-ergonomics branch August 21, 2024 00:46
fjcasti1 added a commit that referenced this pull request Aug 21, 2024
* main:
  ci(python): increase timeout for integration test (#4306)
  chore(main): release arize-phoenix 4.26.0 (#4299)
  fix: postgresql driver name for db migrations (#4304)
  feat(auth): add login/ logout routes and createUser mutation (#4293)
  chore(main): release arize-phoenix 4.25.0 (#4251)
  fix(python): application launch on Windows (#4276)
  ci: exclude buggy scipy version (#4298)
  chore(main): release arize-phoenix-otel 0.3.0 (#4297)
  feat: Improve OTel wrapper ergonomics (#4295)
  feat(auth): add expiry support for system keys (#4296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants