fix(cli): suggest logfire extras instead of raw otel package names#1789
fix(cli): suggest logfire extras instead of raw otel package names#1789tysoncung wants to merge 1 commit intopydantic:mainfrom
Conversation
…ydantic#1297) When `logfire run` recommends installing instrumentation packages, suggest `logfire[extra]` syntax (e.g. `logfire[requests,sqlite3]`) instead of raw `opentelemetry-instrumentation-*` package names for packages that have a corresponding logfire optional dependency group. This gives users a simpler install command and ensures they get the version-pinned dependencies from logfire's extras. Packages without a logfire extra (e.g. botocore, jinja2) still show the raw opentelemetry package name. Closes pydantic#1297
| OTEL_PACKAGE_TO_LOGFIRE_EXTRA: dict[str, str] = { | ||
| 'opentelemetry-instrumentation-aiohttp-client': 'aiohttp-client', | ||
| 'opentelemetry-instrumentation-aiohttp-server': 'aiohttp-server', | ||
| 'opentelemetry-instrumentation-asyncpg': 'asyncpg', | ||
| 'opentelemetry-instrumentation-aws-lambda': 'aws-lambda', | ||
| 'opentelemetry-instrumentation-celery': 'celery', | ||
| 'opentelemetry-instrumentation-django': 'django', | ||
| 'opentelemetry-instrumentation-fastapi': 'fastapi', | ||
| 'opentelemetry-instrumentation-flask': 'flask', | ||
| 'opentelemetry-instrumentation-google-genai': 'google-genai', | ||
| 'opentelemetry-instrumentation-httpx': 'httpx', | ||
| 'opentelemetry-instrumentation-mysql': 'mysql', | ||
| 'opentelemetry-instrumentation-psycopg': 'psycopg', | ||
| 'opentelemetry-instrumentation-psycopg2': 'psycopg2', | ||
| 'opentelemetry-instrumentation-pymongo': 'pymongo', | ||
| 'opentelemetry-instrumentation-redis': 'redis', | ||
| 'opentelemetry-instrumentation-requests': 'requests', | ||
| 'opentelemetry-instrumentation-sqlalchemy': 'sqlalchemy', | ||
| 'opentelemetry-instrumentation-sqlite3': 'sqlite3', | ||
| 'opentelemetry-instrumentation-starlette': 'starlette', | ||
| 'opentelemetry-instrumentation-system-metrics': 'system-metrics', | ||
| } |
There was a problem hiding this comment.
🚩 Dead entries in OTEL_PACKAGE_TO_LOGFIRE_EXTRA that can never be reached
Three entries in OTEL_PACKAGE_TO_LOGFIRE_EXTRA map to packages that are NOT in OTEL_INSTRUMENTATION_MAP: opentelemetry-instrumentation-aws-lambda, opentelemetry-instrumentation-google-genai, and opentelemetry-instrumentation-system-metrics. Since recommendations are only generated from packages in OTEL_INSTRUMENTATION_MAP (via find_recommended_instrumentations_to_install at logfire/_internal/cli/run.py:221), these entries will never be looked up. They are harmless but could be confusing—either they should be added to OTEL_INSTRUMENTATION_MAP (with corresponding logfire.instrument_* methods) or removed from OTEL_PACKAGE_TO_LOGFIRE_EXTRA to keep the maps consistent.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
2 issues found across 1 file
Confidence score: 3/5
- There is a concrete user-facing risk in
logfire/_internal/cli/run.py: generated install commands include unquotedlogfire[...]extras, which can fail when pasted into shells like zsh that glob brackets. - The extra-mapping change in
logfire/_internal/cli/run.pyintroduces duplicated instrumentation source entries and already-unreachable paths, which raises maintenance and drift risk over time. - Given the severity/confidence on the command-generation issue, this looks like a moderate merge risk rather than a blocker, but it is worth fixing before broader CLI use.
- Pay close attention to
logfire/_internal/cli/run.py- install command quoting and extra-mapping drift could cause shell breakage and dead paths.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="logfire/_internal/cli/run.py">
<violation number="1" location="logfire/_internal/cli/run.py:33">
P2: New extra mapping duplicates instrumentation source and already contains unreachable entries, creating dead paths and drift risk.</violation>
<violation number="2" location="logfire/_internal/cli/run.py:363">
P2: Install command generation emits unquoted `logfire[...]` extras, which can break when pasted into shells that treat brackets as glob patterns (e.g., zsh).</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| 'opentelemetry-instrumentation-aiohttp-client': 'aiohttp-client', | ||
| 'opentelemetry-instrumentation-aiohttp-server': 'aiohttp-server', | ||
| 'opentelemetry-instrumentation-asyncpg': 'asyncpg', | ||
| 'opentelemetry-instrumentation-aws-lambda': 'aws-lambda', |
There was a problem hiding this comment.
P2: New extra mapping duplicates instrumentation source and already contains unreachable entries, creating dead paths and drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/_internal/cli/run.py, line 33:
<comment>New extra mapping duplicates instrumentation source and already contains unreachable entries, creating dead paths and drift risk.</comment>
<file context>
@@ -23,6 +23,32 @@
+ 'opentelemetry-instrumentation-aiohttp-client': 'aiohttp-client',
+ 'opentelemetry-instrumentation-aiohttp-server': 'aiohttp-server',
+ 'opentelemetry-instrumentation-asyncpg': 'asyncpg',
+ 'opentelemetry-instrumentation-aws-lambda': 'aws-lambda',
+ 'opentelemetry-instrumentation-celery': 'celery',
+ 'opentelemetry-instrumentation-django': 'django',
</file context>
|
|
||
| parts: list[str] = [] | ||
| if logfire_extras: | ||
| parts.append(f'logfire[{",".join(sorted(logfire_extras))}]') |
There was a problem hiding this comment.
P2: Install command generation emits unquoted logfire[...] extras, which can break when pasted into shells that treat brackets as glob patterns (e.g., zsh).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At logfire/_internal/cli/run.py, line 363:
<comment>Install command generation emits unquoted `logfire[...]` extras, which can break when pasted into shells that treat brackets as glob patterns (e.g., zsh).</comment>
<file context>
@@ -308,18 +338,37 @@ def installed_packages() -> set[str]:
+
+ parts: list[str] = []
+ if logfire_extras:
+ parts.append(f'logfire[{",".join(sorted(logfire_extras))}]')
+ parts.extend(sorted(standalone_packages))
</file context>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Problem
When
logfire runrecommends installing instrumentation packages, it currently suggests raw OpenTelemetry package names like:This is confusing because logfire already provides optional dependency groups (extras) that pin compatible versions.
Solution
For packages that have a corresponding logfire optional dependency group, suggest the logfire extra syntax instead:
Packages without a logfire extra (e.g.
botocore,jinja2,urllib) still show the rawopentelemetry-instrumentation-*package name.Changes
OTEL_PACKAGE_TO_LOGFIRE_EXTRAmapping (OTel package → logfire extra name)get_recommendation_texts()to showlogfire[extra]in the checklist_full_install_command()to generatelogfire[extra1,extra2,...]install commandslogfire[...]specifier, standalone packages listed separatelyExample output (before → after)
Before:
After:
Closes #1297
Summary by cubic
logfire runnow suggestslogfire[...]extras instead of rawopentelemetry-instrumentation-*packages, making installs simpler and version-safe. Closes #1297.OTEL_PACKAGE_TO_LOGFIRE_EXTRAto map OTel packages tologfireextras.get_recommendation_texts()to showlogfire[extra]in the checklist when available._full_install_command()to produce a singlelogfire[extra1,extra2]plus any unmatched OTel packages; works for bothuvandpip.Written for commit 6132ba7. Summary will update on new commits.