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 #477

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

maxdeichmann
Copy link
Member

@maxdeichmann maxdeichmann commented Dec 5, 2024

Important

Enhance getTraceUrl() to include projectId and add project fetching functionality with tests.

  • Behavior:
    • getTraceUrl() in LangfuseObjectClient now includes projectId in the URL. If projectId is not set, it fetches from /api/public/projects.
    • Removes DeferRuntime type from types.ts.
  • Functions:
    • Adds fetchProjects() in LangfuseCoreStateless to retrieve project data.
  • Tests:
    • Adds test in langfuse.traces.spec.ts to verify getTraceUrl() returns correct URL with projectId.

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

Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
langfuse-js ✅ Ready (Inspect) Visit Preview Dec 5, 2024 3:23pm

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 modifies the trace URL generation system to incorporate project IDs, enabling project-specific trace URLs while maintaining backward compatibility.

  • Added _projectId optional parameter in LangfuseCoreOptions type in /langfuse-core/src/types.ts
  • Added project ID validation in trace creation tests in /langfuse-core/test/langfuse.traces.spec.ts
  • Implemented async getTraceUrl method with project ID support in /langfuse-core/src/index.ts
  • Added new fetchProjects method for retrieving available projects
  • Removed legacy DeferRuntime type and related integration code

💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!

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

langfuse-core/test/langfuse.traces.spec.ts Outdated Show resolved Hide resolved
langfuse-core/src/index.ts 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 adds tests to verify the enhanced getTraceUrl() functionality that now includes project IDs in trace URLs.

  • Added test case in langfuse-core/test/langfuse.traces.spec.ts to verify correct URL generation with project ID
  • Mocked /api/public/projects endpoint response to test project ID fetching
  • Verified URL format matches http://localhost:3000/test-project-id/traces/${traceId}
  • Removed unnecessary timer manipulation code from tests

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)

This PR updates the URL structure for trace endpoints and adds project data fetching functionality, with a focus on consistent URL patterns.

  • Modified URL format in langfuse-core/src/index.ts from /{projectId}/traces/{traceId} to /project/{projectId}/traces/{traceId}
  • Added critical issue: projectId is initialized as null in LangfuseObjectClient but never updated in LangfuseTraceClient
  • Test in langfuse-core/test/langfuse.traces.spec.ts lacks error handling scenarios for project data fetching
  • Hardcoded localhost:3000 URL in tests may cause issues with different baseUrl configurations

2 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)

This PR updates the test suite for trace URL generation, focusing on proper mocking and validation of project-related functionality.

  • Added mock response validation in /langfuse-core/test/langfuse.traces.spec.ts to ensure correct project API endpoint calls
  • Improved test structure by removing redundant timer manipulations
  • Added async/await handling for getTraceUrl() test cases
  • Fixed test URL assertions to match new /project/{projectId}/traces/{traceId} format

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)

This PR update adds test coverage for trace creation functionality, focusing on request validation and timing behavior.

  • Added timing control using Jest's fake timers in /langfuse-core/test/langfuse.traces.spec.ts for more reliable testing
  • Verified correct request URL format to https://cloud.langfuse.com/api/public/ingestion
  • Added validation for trace creation payload structure including ID and timestamp fields

Note: No significant changes requiring additional review points since the last comprehensive review.

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)

This PR update enhances the test suite for trace creation functionality, focusing on request validation and endpoint verification.

  • Updated ingestion endpoint URL in /langfuse-core/test/langfuse.traces.spec.ts from localhost to cloud.langfuse.com
  • Added validation for trace creation request body structure including required fields
  • Implemented proper timing controls using Jest's fake timers for consistent test behavior

Note: Changes are minor and focused on test improvements since the last review.

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

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.

1 participant