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: use projectid when generating trace urls #1024

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

maxdeichmann
Copy link
Member

@maxdeichmann maxdeichmann commented Dec 5, 2024

Important

The PR updates the Langfuse class to include project_id in trace URLs and modifies tests to verify this behavior.

  • Behavior:
    • Modify get_trace_url() in Langfuse class to include project_id in the URL if available.
    • If project_id is not set, fetch project_id from the first project in self.client.projects.get().
  • Attributes:
    • Add project_id attribute to Langfuse class to store the project ID associated with the API keys.
  • Tests:
    • Update test_generate_trace_id() in test_core_sdk.py to verify the trace URL includes project_id.

This description was created by Ellipsis for e255492. It will automatically update as commits are pushed.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

This PR adds project ID support to trace URL generation in the Python SDK, enabling proper linking to traces in the Langfuse UI by fetching and incorporating the project ID into the URL path.

  • Added project_id attribute to Langfuse class to store project context
  • Modified get_trace_url() to fetch project ID from API if not already set
  • Added test_generate_trace_id() to verify URL generation with project ID
  • Updated URL format to include project ID: {base_url}/{project_id}/traces/{trace_id}

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

tests/test_core_sdk.py Outdated Show resolved Hide resolved
langfuse/client.py Outdated Show resolved Hide resolved
langfuse/client.py Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

(updates since last review)

Modified test_generate_trace_id() to use HTTP protocol instead of HTTPS, maintaining consistency with the new URL format that includes project ID in the path.

  • Changed protocol from 'https' to 'http' in tests/test_core_sdk.py for trace URL assertions
  • Test still uses hardcoded project ID (7a88fb47-b4e2-43b8-a06c-a5ce950dc53a) which should be made dynamic

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

langfuse/client.py Show resolved Hide resolved
tests/test_core_sdk.py Outdated Show resolved Hide resolved
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

(updates since last review)

This PR continues to evolve the trace URL functionality by updating the test implementation in the Python SDK.

  • Updated test_core_sdk.py to dynamically fetch project ID instead of using hardcoded value 7a88fb47-b4e2-43b8-a06c-a5ce950dc53a
  • Test now uses langfuse.client.projects.get() to retrieve actual project context
  • Ensures test is environment-independent and more maintainable

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Disclaimer: Experimental PR review

PR Summary

(updates since last review)

Updated the URL path structure in langfuse/client.py to include '/project/' prefix before project ID in trace URLs, aligning with the latest API endpoint format.

  • Modified URL format in get_trace_url() from /{project_id}/traces/{trace_id} to /project/{project_id}/traces/{trace_id}
  • Updated test assertions in tests/test_core_sdk.py to verify new URL path structure

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@maxdeichmann maxdeichmann enabled auto-merge (squash) December 5, 2024 17:40
@maxdeichmann maxdeichmann disabled auto-merge December 5, 2024 17:40
@maxdeichmann maxdeichmann merged commit 17a2f4e into main Dec 5, 2024
7 of 10 checks passed
@maxdeichmann maxdeichmann deleted the max/lfe-357-upgrade-traces-api-using-project branch December 5, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants