feat: use dependency groups in logfire run suggestions (fixes #1297)#2082
feat: use dependency groups in logfire run suggestions (fixes #1297)#2082krishna3554 wants to merge 4 commits into
Conversation
…c#1297) - Add urllib optional dependency group in pyproject.toml - Map target packages to logfire dependency groups (TARGET_TO_DEP_GROUP) - Update _full_install_command to suggest 'uv add logfire[requests,sqlite3,urllib]' instead of raw opentelemetry packages - Add _detect_uv_invocation() to detect uvx, uv run --with, uv run patterns - Update test snapshots for new output format
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI now maps recommended instrumentation targets to Logfire dependency groups and generates 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@logfire/_internal/cli/run.py`:
- Around line 205-218: Update the Linux process-detection logic around the
`/proc/self/cmdline` handling to traverse the parent/ancestor process chain and
read each ancestor’s command line instead. Match `uvx`, `uv run`, and `uv run
--with` against the ancestor arguments, returning the existing mode values while
preserving the current error handling and fallback behavior.
- Around line 250-259: Update the dependency-install command assembly around
_format_dep_group_install_command so grouped installs also respect
is_uv_installed(). Emit the uv-based grouped command only when uv is available;
otherwise generate a usable pip install command for the grouped packages, while
preserving existing grouping and sorting behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b8543d6-97e3-4ba5-8ba6-cca72482ce4e
📒 Files selected for processing (3)
logfire/_internal/cli/run.pypyproject.tomltests/test_cli.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/logfire(manual)pydantic/pydantic-ai(manual)pydantic/platform(auto-detected)pydantic/pydantic(auto-detected)
| # On Linux, we can check /proc/self/cmdline for the parent process | ||
| try: | ||
| with open('/proc/self/cmdline', 'rb') as f: | ||
| cmdline = f.read().decode('utf-8', errors='ignore') | ||
| # cmdline is null-separated arguments | ||
| parts = cmdline.split('\x00') | ||
| if parts and 'uvx' in parts[0]: | ||
| return 'uvx' | ||
| if 'uv' in parts[0] and 'run' in parts: | ||
| # Check for --with | ||
| if '--with' in parts: | ||
| return 'uv_run_with' | ||
| return 'uv_run' | ||
| except (OSError, IndexError): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Read the uv parent process, not /proc/self/cmdline.
/proc/self/cmdline contains the running Logfire process, so normal uvx and uv run invocations will not match these branches and will fall through to None. Walk the parent/ancestor process chain before matching the uv invocation mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@logfire/_internal/cli/run.py` around lines 205 - 218, Update the Linux
process-detection logic around the `/proc/self/cmdline` handling to traverse the
parent/ancestor process chain and read each ancestor’s command line instead.
Match `uvx`, `uv run`, and `uv run --with` against the ancestor arguments,
returning the existing mode values while preserving the current error handling
and fallback behavior.
| parts = [] | ||
| if dep_groups: | ||
| parts.append(_format_dep_group_install_command(sorted(dep_groups))) | ||
| if fallback_packages: | ||
| if is_uv_installed(): | ||
| parts.append(f'uv add {" ".join(sorted(fallback_packages))}') | ||
| else: | ||
| parts.append(f'pip install {" ".join(sorted(fallback_packages))}') # pragma: no cover | ||
|
|
||
| return '\n'.join(parts) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle grouped installs when uv is unavailable.
The grouped branch always emits uv add, while is_uv_installed() is checked only for fallback packages. Users without uv therefore receive an unusable command for mapped targets such as urllib.
Proposed fix
parts = []
if dep_groups:
- parts.append(_format_dep_group_install_command(sorted(dep_groups)))
+ groups = sorted(dep_groups)
+ if is_uv_installed():
+ parts.append(_format_dep_group_install_command(groups))
+ else:
+ parts.append(f"pip install 'logfire[{','.join(groups)}]'")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts = [] | |
| if dep_groups: | |
| parts.append(_format_dep_group_install_command(sorted(dep_groups))) | |
| if fallback_packages: | |
| if is_uv_installed(): | |
| parts.append(f'uv add {" ".join(sorted(fallback_packages))}') | |
| else: | |
| parts.append(f'pip install {" ".join(sorted(fallback_packages))}') # pragma: no cover | |
| return '\n'.join(parts) | |
| parts = [] | |
| if dep_groups: | |
| groups = sorted(dep_groups) | |
| if is_uv_installed(): | |
| parts.append(_format_dep_group_install_command(groups)) | |
| else: | |
| parts.append(f"pip install 'logfire[{','.join(groups)}]'") | |
| if fallback_packages: | |
| if is_uv_installed(): | |
| parts.append(f'uv add {" ".join(sorted(fallback_packages))}') | |
| else: | |
| parts.append(f'pip install {" ".join(sorted(fallback_packages))}') # pragma: no cover | |
| return '\n'.join(parts) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@logfire/_internal/cli/run.py` around lines 250 - 259, Update the
dependency-install command assembly around _format_dep_group_install_command so
grouped installs also respect is_uv_installed(). Emit the uv-based grouped
command only when uv is available; otherwise generate a usable pip install
command for the grouped packages, while preserving existing grouping and sorting
behavior.
There was a problem hiding this comment.
Pull request overview
This PR updates the Logfire CLI’s inspect/run instrumentation summary to recommend installing Logfire’s own optional dependency groups (e.g. logfire[requests,sqlite3,urllib]) instead of listing individual opentelemetry-instrumentation-* packages, and adds a missing urllib optional dependency group to pyproject.toml.
Changes:
- Added a
urllibextra inpyproject.tomlto match the existing instrumentation recommendations. - Introduced a target→dependency-group mapping and updated the “install all” recommendation to prefer
logfire[...]groups where available. - Added (but did not yet wire in)
uvinvocation detection logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
pyproject.toml |
Adds urllib optional dependency group to support logfire[urllib] installation suggestions. |
logfire/_internal/cli/run.py |
Implements group-based install command generation and adds uv invocation detection helper(s). |
tests/test_cli.py |
Updates snapshot output for the new install suggestion format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TARGET_TO_DEP_GROUP = { | ||
| 'requests': 'requests', | ||
| 'sqlite3': 'sqlite3', | ||
| 'urllib': 'urllib', | ||
| 'urllib3': 'urllib3', | ||
| 'httpx': 'httpx', | ||
| 'aiohttp_client': 'aiohttp', | ||
| 'aiohttp_server': 'aiohttp-server', | ||
| 'fastapi': 'fastapi', | ||
| 'flask': 'flask', | ||
| 'django': 'django', | ||
| 'starlette': 'starlette', | ||
| 'sqlalchemy': 'sqlalchemy', | ||
| 'redis': 'redis', | ||
| 'psycopg': 'psycopg', | ||
| 'psycopg2': 'psycopg2', | ||
| 'pymongo': 'pymongo', | ||
| 'mysql': 'mysql', | ||
| 'celery': 'celery', | ||
| 'asyncpg': 'asyncpg', |
| 'aiopg': 'aiopg', | ||
| 'pymysql': 'pymysql', | ||
| 'tornado': 'tornado', | ||
| 'falcon': 'falcon', | ||
| 'pika': 'pika', | ||
| 'confluent_kafka': 'confluent-kafka', | ||
| 'kafka_python': 'kafka-python', | ||
| 'elasticsearch': 'elasticsearch', | ||
| 'boto': 'boto', | ||
| 'botocore': 'botocore', | ||
| 'grpc': 'grpc', | ||
| 'jinja2': 'jinja2', | ||
| 'pyramid': 'pyramid', | ||
| 'tortoise_orm': 'tortoise-orm', | ||
| 'remoulade': 'remoulade', |
| # On Linux, we can check /proc/self/cmdline for the parent process | ||
| try: | ||
| with open('/proc/self/cmdline', 'rb') as f: | ||
| cmdline = f.read().decode('utf-8', errors='ignore') | ||
| # cmdline is null-separated arguments | ||
| parts = cmdline.split('\x00') | ||
| if parts and 'uvx' in parts[0]: | ||
| return 'uvx' | ||
| if 'uv' in parts[0] and 'run' in parts: | ||
| # Check for --with | ||
| if '--with' in parts: | ||
| return 'uv_run_with' | ||
| return 'uv_run' | ||
| except (OSError, IndexError): | ||
| pass |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Improve
logfire runCLI output to suggest installing instrumentation checklist items using dependency groups (logfire[requests,sqlite3,urllib]) instead of individualopentelemetry-instrumentation-*packages.Also adds detection for common
uvinvocation patterns and suggests appropriate re-run commands.Changes
urlliboptional dependency group (was missing)TARGET_TO_DEP_GROUPmapping from target packages to logfire dependency group names_full_install_command()to use dependency groups where available, falling back to individual packages for others_detect_uv_invocation()to detectuvx,uv run --with, anduv runinvocation patterns_format_dep_group_install_command()helperExample Output
Before:
After:
Verification
Fixes #1297