refactor(observability): export buildObservabilityConfigFromEnv helpe… - #222
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the buildObservabilityConfigFromEnv helper function by extracting it from internal CLI session management code into a dedicated utilities module, making it available for SDK users to configure Langfuse observability from environment variables.
Key changes:
- Created new
observabilityHelpers.tsutility module with comprehensive JSDoc documentation - Moved
buildObservabilityConfigFromEnvfromglobalSessionState.tsto the new utilities module - Exported the helper function from the main SDK index for public use
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/lib/utils/observabilityHelpers.ts | New utility module containing the extracted buildObservabilityConfigFromEnv function with enhanced documentation |
| src/lib/session/globalSessionState.ts | Removed the internal helper function and replaced with import from new utilities module |
| src/lib/index.ts | Added export for buildObservabilityConfigFromEnv to make it available to SDK users |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * - LANGFUSE_ENVIRONMENT: Environment name (default: dev) | ||
| * - PUBLIC_APP_VERSION: Release/version identifier (default: v1.0.0) |
There was a problem hiding this comment.
The documentation lists PUBLIC_APP_VERSION but the code also checks PUBLIC_APP_ENVIRONMENT (line 50) and npm_package_version (line 54). These environment variables should be documented for completeness.
| * - LANGFUSE_ENVIRONMENT: Environment name (default: dev) | |
| * - PUBLIC_APP_VERSION: Release/version identifier (default: v1.0.0) | |
| * - LANGFUSE_ENVIRONMENT: Environment name (default: dev) | |
| * - PUBLIC_APP_ENVIRONMENT: Fallback environment name if LANGFUSE_ENVIRONMENT is not set | |
| * - PUBLIC_APP_VERSION: Release/version identifier (default: v1.0.0) | |
| * - npm_package_version: Fallback release/version identifier if PUBLIC_APP_VERSION is not set |
There was a problem hiding this comment.
Can create confusion, hence documented only langfuse environment
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR extracts the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
51b823a to
81a7969
Compare
81a7969 to
77d73eb
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| baseUrl: | ||
| process.env.LANGFUSE_BASE_URL?.trim() || "https://cloud.langfuse.com", |
There was a problem hiding this comment.
The JSDoc comment documents LANGFUSE_BASE_URL as having a default of 'https://cloud.langfuse.com', but this default is actually only applied when the environment variable is not set or is empty. Consider clarifying that this is a fallback value rather than a strict default to avoid confusion about when the default applies.
| environment: | ||
| process.env.LANGFUSE_ENVIRONMENT?.trim() || | ||
| process.env.PUBLIC_APP_ENVIRONMENT?.trim() || | ||
| "dev", |
There was a problem hiding this comment.
The JSDoc mentions only LANGFUSE_ENVIRONMENT for the environment name, but the code also checks PUBLIC_APP_ENVIRONMENT as a fallback. The documentation should list both environment variables to accurately describe the function's behavior.
| release: | ||
| process.env.PUBLIC_APP_VERSION?.trim() || | ||
| process.env.npm_package_version?.trim() || | ||
| "v1.0.0", |
There was a problem hiding this comment.
The JSDoc lists PUBLIC_APP_VERSION as the environment variable for the release identifier, but doesn't mention npm_package_version which is checked as a fallback. The documentation should include both environment variables to provide complete information.
Pull Request
Description
SDK users needed a convenient way to configure Langfuse observability from environment variables without manually constructing ObservabilityConfig objects. The helper function existed but was buried in internal CLI session management code.
Type of Change
Related Issues
Changes Made
New File:
src/lib/utils/observabilityHelpers.tsbuildObservabilityConfigFromEnvto dedicatedutilities module
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
Refactor
API Changes