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

fix(vercel): allow nested executions in parent trace #466

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

hassiebp
Copy link
Contributor

@hassiebp hassiebp commented Nov 25, 2024

Important

Enhance LangfuseExporter to support nested generateText calls and improve trace management with new test coverage.

  • Behavior:
    • Add test in langfuse-integration-vercel.spec.ts to verify nesting of multiple generateText calls under a single trace.
    • Modify LangfuseExporter in LangfuseExporter.ts to handle trace updates and creations based on user-provided trace ID.
  • Functions:
    • Update processTraceSpans() in LangfuseExporter.ts to separate trace update and creation logic.
    • Update processSpanAsLangfuseSpan() to optionally use rootSpanName for root spans.
  • Misc:
    • Add traceUpdate and traceCreate objects in LangfuseExporter.ts for better trace management.
    • Update CI configuration in .github/workflows/ci.yml to disable CLICKHOUSE_CLUSTER_ENABLED.

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

Copy link

vercel bot commented Nov 25, 2024

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

Name Status Preview Updated (UTC)
langfuse-js ✅ Ready (Inspect) Visit Preview Nov 26, 2024 1:52pm

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

Enhanced support for nested executions in Vercel AI SDK integration by improving trace hierarchy and observation linking.

  • Added handling in langfuse-vercel/src/LangfuseExporter.ts to properly nest multiple generateText calls under a parent trace
  • Modified span naming logic in processSpanAsLangfuseSpan to maintain correct hierarchy with root span names
  • Added integration test in integration-test/langfuse-integration-vercel.spec.ts to verify nested execution behavior with 3 generateText calls
  • Split trace creation into separate update/create objects to prevent duplicate data when nesting

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

integration-test/langfuse-integration-vercel.spec.ts 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 adds integration tests for the Vercel AI SDK integration, focusing on testing nested execution behavior and trace management.

  • Added comprehensive test in /integration-test/langfuse-integration-vercel.spec.ts verifying multiple generateText calls under a single trace
  • Added test cases for various Vercel AI SDK features including streamText, embeddings, and tool calls
  • Added test validation for cost calculations, token counts, and metadata propagation
  • Added test for prompt linking functionality with version tracking

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)

Based on the latest changes, I'll provide a focused summary of the key updates:

Fixed string comparison logic in root observation sorting for nested generateText calls in integration tests.

  • Changed arithmetic comparison (-) to localeCompare() in /integration-test/langfuse-integration-vercel.spec.ts for proper string-based sorting of root observations
  • Added validation for root observation names to ensure correct ordering with format ${baseRootSpanName}-${i}
  • Improved test structure with proper cleanup using beforeEach/afterEach hooks for OTEL state management

The changes are minimal but important for ensuring reliable test behavior when dealing with nested trace hierarchies.

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)

Based on the latest changes, here's a focused summary of the key updates:

Updated CI workflow configuration to improve integration test reliability and performance.

  • Disabled CLICKHOUSE_CLUSTER_ENABLED in .github/workflows/ci.yml for simpler test environment
  • Added health check step to verify langfuse server availability before running tests
  • Added retry mechanism with max 10 attempts for server health checks
  • Added detailed logging for server startup failures

The changes focus on making the CI pipeline more robust and reliable when testing the nested trace functionality.

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

@hassiebp hassiebp merged commit a904428 into main Nov 26, 2024
7 of 9 checks passed
@hassiebp hassiebp deleted the fix-vercel-parent-trace-nested-spans branch November 26, 2024 14:12
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.

bug: vercel ai sdk integration does not reliably pick up functionId as the generation/span name
1 participant