CLI: Use logfire[extras] for instrumentation suggestions and improve environment-aware tips#1833
CLI: Use logfire[extras] for instrumentation suggestions and improve environment-aware tips#1833jalpatel11 wants to merge 4 commits intopydantic:mainfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Closes #1297 |
| return f'uvx --from {shlex.quote(logfire_target)} {" ".join(with_args)} logfire {shlex.join(sys.argv[1:])}'.replace( | ||
| ' ', ' ' | ||
| ).strip() |
There was a problem hiding this comment.
🟡 str.replace(' ', ' ') corrupts shell-quoted arguments containing double spaces
The _full_install_command function uses .replace(' ', ' ') to clean up double spaces caused by empty list joins in the f-string. However, str.replace doesn't understand shell quoting, so it will also corrupt legitimate double spaces inside shell-quoted sys.argv values. For example, if sys.argv contains 'arg with spaces', shlex.join correctly preserves these as 'arg with spaces' (within shell quotes), but the subsequent .replace(' ', ' ') blindly reduces all double spaces, producing the corrupted 'arg with spaces'.
Demonstration of the issue
Input: sys.argv = ['logfire', 'run', 'arg with spaces']
Before replace: uvx --from 'logfire[req]' logfire run 'arg with spaces'
After replace: uvx --from 'logfire[req]' logfire run 'arg with spaces'
A safer approach would be to build the command as a list of parts and join once, avoiding the need for .replace().
Prompt for agents
The _full_install_command function at logfire/_internal/cli/run.py:374-376 and :382 uses .replace(' ', ' ') on the fully assembled shell command string. This is a blunt approach that does not respect shell quoting -- if any element of sys.argv contains a literal double space, shlex.join will correctly quote it, but the .replace will then corrupt the quoted content.
A safer approach is to build the command as a list of string parts (each being a logical token in the command) and then join them with a single space. For example:
cmd_parts = ['uvx', '--from', shlex.quote(logfire_target)]
cmd_parts.extend(with_args) # each with_args element is already '--with quoted_pkg'
cmd_parts.append('logfire')
cmd_parts.extend(shlex.quote(a) for a in sys.argv[1:])
return ' '.join(cmd_parts)
This eliminates the structural double-space problem without needing the fragile .replace() hack. Apply the same approach to the uv run path at line 382.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def test_get_recommendation_texts(): | ||
| recs = {('opentelemetry-instrumentation-foo', 'foo'), ('opentelemetry-instrumentation-bar', 'bar')} | ||
| recommended, install = get_recommendation_texts(recs) | ||
| assert 'uv add opentelemetry-instrumentation-bar opentelemetry-instrumentation-foo' in install | ||
| assert 'need to install opentelemetry-instrumentation-bar' in recommended | ||
| assert 'need to install opentelemetry-instrumentation-foo' in recommended | ||
| assert 'pip install opentelemetry-instrumentation-bar opentelemetry-instrumentation-foo' in str(install) | ||
| assert 'need to install opentelemetry-instrumentation-bar' in str(recommended) | ||
| assert 'need to install opentelemetry-instrumentation-foo' in str(recommended) |
There was a problem hiding this comment.
🔴 Tests fail in CI because UV=1 environment variable is not cleared
CI runs tests via uv run --no-sync coverage run -m pytest (.github/workflows/main.yml:129), which sets UV=1 in the environment. The _full_install_command function at logfire/_internal/cli/run.py:368 checks os.environ.get('UV') == '1' and takes a completely different code path (producing uv run --with ... commands) before ever reaching the is_uv_installed() check. Four tests don't isolate this variable:
test_inspect(line 285 mocksis_uv_installedbut snapshot on line 308 expectsuv add ...)test_get_recommendation_texts(line 1648 assertspip installin output)test_get_recommendation_texts_with_extras(line 1659 assertspip installin output)test_get_recommendation_texts_uv(line 1665 mocksis_uv_installedbut line 1668 assertsuv addin output)
With UV=1, all four assertions fail because the code returns uv run --with ... instead of the expected uv add ... or pip install ... output.
Prompt for agents
Four tests in tests/test_cli.py are missing UV environment variable isolation and will fail in CI (where UV=1 is set by uv run).
The root cause is that _full_install_command in logfire/_internal/cli/run.py:368 checks os.environ.get('UV') == '1' before checking is_uv_installed(). When UV=1 is set (as it is in CI via uv run), a completely different code path is taken, producing uv run --with output instead of uv add or pip install.
Affected tests:
1. test_get_recommendation_texts (line 1645) - needs monkeypatch parameter added, then monkeypatch.delenv('UV', raising=False)
2. test_get_recommendation_texts_with_extras (line 1653) - same fix needed
3. test_get_recommendation_texts_uv (line 1664) - already has monkeypatch, just needs monkeypatch.delenv('UV', raising=False)
4. test_inspect (line 269) - already has monkeypatch, needs monkeypatch.delenv('UV', raising=False)
For tests 1 and 2, the function signature needs to be updated to accept monkeypatch: pytest.MonkeyPatch as a parameter. All four tests need monkeypatch.delenv('UV', raising=False) added before calling get_recommendation_texts or main.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def test_get_recommendation_texts(): | ||
| recs = {('opentelemetry-instrumentation-foo', 'foo'), ('opentelemetry-instrumentation-bar', 'bar')} | ||
| recommended, install = get_recommendation_texts(recs) | ||
| assert 'uv add opentelemetry-instrumentation-bar opentelemetry-instrumentation-foo' in install | ||
| assert 'need to install opentelemetry-instrumentation-bar' in recommended | ||
| assert 'need to install opentelemetry-instrumentation-foo' in recommended | ||
| assert 'pip install opentelemetry-instrumentation-bar opentelemetry-instrumentation-foo' in str(install) | ||
| assert 'need to install opentelemetry-instrumentation-bar' in str(recommended) | ||
| assert 'need to install opentelemetry-instrumentation-foo' in str(recommended) |
There was a problem hiding this comment.
🚩 UV=1 environment variable in tests not isolated
Several new tests (test_get_recommendation_texts, test_get_recommendation_texts_with_extras) do not monkeypatch the UV environment variable. If these tests happen to run in a CI environment where UV=1 is set (e.g., if the test runner itself is invoked via uv run), the _full_install_command at logfire/_internal/cli/run.py:368 would take the UV-specific path instead of the is_uv_installed() path, causing unexpected command output and test failures. The tests test_get_recommendation_texts_uv, test_get_recommendation_texts_uv_run, and test_get_recommendation_texts_uvx correctly set/control the UV env var, but the base tests don't unset it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # If run via `uvx logfire run` or `uv run --with logfire logfire run` | ||
| if os.environ.get('UV') == '1': | ||
| logfire_target = f'logfire{extras_str}' | ||
|
|
||
| # Heuristic to detect `uvx` (uv tool run) | ||
| if '/uv/tools/' in sys.executable: | ||
| with_args = [f'--with {shlex.quote(p)}' for p in standalone_packages] | ||
| return f'uvx --from {shlex.quote(logfire_target)} {" ".join(with_args)} logfire {shlex.join(sys.argv[1:])}'.replace( | ||
| ' ', ' ' | ||
| ).strip() | ||
|
|
||
| # Heuristic for `uv run --with logfire` | ||
| # We assume if it's run via uv and not as a tool, it's either `uv run` or `uv run --with`. | ||
| # Providing the `--with` version is safer for ephemeral environments. | ||
| all_with = [f'--with {shlex.quote(p)}' for p in [logfire_target] + standalone_packages] | ||
| return f'uv run {" ".join(all_with)} logfire {shlex.join(sys.argv[1:])}'.replace(' ', ' ').strip() |
There was a problem hiding this comment.
🚩 UV path generates re-run commands, not install commands
When UV=1 is set, _full_install_command at logfire/_internal/cli/run.py:368-382 generates commands like uvx --from 'logfire[extras]' logfire ... or uv run --with 'logfire[extras]' logfire ... that re-invoke logfire with the extra dependencies, rather than uv add ... / pip install ... commands that permanently install packages. This is intentionally different behavior for ephemeral UV environments, but it means the logfire inspect command (which also calls this via print_otel_summary at logfire/_internal/cli/__init__.py:123) would suggest re-running logfire inspect rather than installing packages. This is semantically different from the non-UV path and could be confusing for users running logfire inspect in a uv-managed project.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR improves the recommendations provided by
logfire runwhen missing instrumentations are detected. Instead of suggesting individual OpenTelemetry instrumentation packages, the CLI now recommends installing the corresponding Logfire extras (e.g.,logfire[requests]instead ofopentelemetry-instrumentation-requests). This ensures users are using the dependencies Pydantic Logfire is tested against and simplifies dependency management.I have also added logic to detect the execution context (such as
uvxoruv run --with) and suggest the appropriate re-run commands. This makes it easier for users in ephemeral environments to get their scripts running with the necessary dependencies.Technical Implementation
OTEL_PACKAGE_TO_LOGFIRE_EXTRAinlogfire/_internal/cli/run.pyto translate detected OTel packages into Logfire extras.os.environ.get('UV') == '1'andsys.executablepath analysis to detect iflogfireis running as auvtool (uvx) or via an ephemeraluv run._full_install_commandto generate context-specific suggestions using--fromand multiple--withflags for these environments.shlex.quotefor all generated commands to ensure that brackets inlogfire[...]syntax are correctly escaped for shells likezsh.urllibextra topyproject.tomland updated the instrumentation maps inrun.pyto ensure it is recommended correctly aslogfire[urllib].uv addandpip install.Verification
All 91 CLI tests in
tests/test_cli.pypass, including 5 new tests covering:uvxanduv runheuristic detection.--withflags.logfire[urllib]andlogfire[sqlite3]mapping verification.