-
Notifications
You must be signed in to change notification settings - Fork 229
CLI: Use logfire[extras] for instrumentation suggestions and improve environment-aware tips #1833
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
base: main
Are you sure you want to change the base?
Changes from all commits
fa0457b
c0070be
39777f2
0d38714
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,35 @@ | |
| 'openai-agents': 'openai_agents', | ||
| } | ||
|
|
||
| # Map of instrumentation packages to the Logfire extras that install them | ||
| OTEL_PACKAGE_TO_LOGFIRE_EXTRA = { | ||
| 'opentelemetry-instrumentation-aiohttp-client': 'aiohttp', | ||
| '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', | ||
| 'opentelemetry-instrumentation-urllib': 'urllib', | ||
| 'opentelemetry-instrumentation-asgi': 'asgi', | ||
| 'opentelemetry-instrumentation-wsgi': 'wsgi', | ||
| 'openinference-instrumentation-litellm': 'litellm', | ||
| 'openinference-instrumentation-dspy': 'dspy', | ||
| } | ||
|
|
||
|
|
||
| @dataclass | ||
| class InstrumentationContext: | ||
|
|
@@ -232,12 +261,30 @@ def instrumented_packages_text( | |
| return text | ||
|
|
||
|
|
||
| def _get_logfire_extras(recommendations: list[tuple[str, str]]) -> tuple[list[str], list[str]]: | ||
| """Convert OTel packages to Logfire extras where possible.""" | ||
| extras: set[str] = set() | ||
| standalone: list[str] = [] | ||
| for pkg_name, _ in recommendations: | ||
| extra = OTEL_PACKAGE_TO_LOGFIRE_EXTRA.get(pkg_name) | ||
| if extra: | ||
| extras.add(extra) | ||
| else: | ||
| standalone.append(pkg_name) | ||
| return sorted(extras), sorted(standalone) | ||
|
|
||
|
|
||
| def get_recommendation_texts(recommendations: set[tuple[str, str]]) -> tuple[Text, Text]: | ||
| """Return (recommended_packages_text, install_all_text) as Text objects.""" | ||
| sorted_recommendations = sorted(recommendations) | ||
| recommended_text = Text() | ||
| for pkg_name, instrumented_pkg in sorted_recommendations: | ||
| recommended_text.append(f'☐ {instrumented_pkg} (need to install {pkg_name})\n', style='grey50') | ||
| extra = OTEL_PACKAGE_TO_LOGFIRE_EXTRA.get(pkg_name) | ||
| if extra: | ||
| suggestion = f'logfire[{extra}]' | ||
| else: | ||
| suggestion = pkg_name | ||
| recommended_text.append(f'☐ {instrumented_pkg} (need to install {suggestion})\n', style='grey50') | ||
| recommended_text.append('\n') | ||
|
|
||
| install_text = Text() | ||
|
|
@@ -312,14 +359,38 @@ def _full_install_command(recommendations: list[tuple[str, str]]) -> str: | |
| if not recommendations: | ||
| return '' # pragma: no cover | ||
|
|
||
| package_names = [pkg_name for pkg_name, _ in recommendations] | ||
| logfire_extras, standalone_packages = _get_logfire_extras(recommendations) | ||
| extras_str = f'[{",".join(logfire_extras)}]' if logfire_extras else '' | ||
|
|
||
| import shlex | ||
|
|
||
| # 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() | ||
|
Comment on lines
+367
to
+382
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 UV path generates re-run commands, not install commands When Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| parts: list[str] = [] | ||
| if logfire_extras: | ||
| parts.append(f'logfire{extras_str}') | ||
| parts.extend(standalone_packages) | ||
| all_packages = ' '.join(shlex.quote(p) for p in parts) | ||
|
|
||
| # TODO(Marcelo): We should customize this. If the user uses poetry, they'd use `poetry add`. | ||
| # Something like `--install-format` with options like `requirements`, `poetry`, `uv`, `pip`. | ||
| if is_uv_installed(): | ||
| return f'uv add {" ".join(package_names)}' | ||
| return f'uv add {all_packages}\n # or\n pip install {all_packages}' | ||
| else: | ||
| return f'pip install {" ".join(package_names)}' # pragma: no cover | ||
| return f'pip install {all_packages}' # pragma: no cover | ||
|
|
||
|
|
||
| def collect_instrumentation_context(exclude: set[str]) -> InstrumentationContext: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -267,27 +267,47 @@ def test_clean_default_dir_is_not_a_directory( | |
|
|
||
|
|
||
| def test_inspect( | ||
| tmp_dir_cwd: Path, logfire_credentials: LogfireCredentials, capsys: pytest.CaptureFixture[str] | ||
| tmp_dir_cwd: Path, | ||
| logfire_credentials: LogfireCredentials, | ||
| capsys: pytest.CaptureFixture[str], | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| os.environ['COLUMNS'] = '150' | ||
| monkeypatch.setitem(os.environ, 'COLUMNS', '150') | ||
| logfire_credentials.write_creds_file(tmp_dir_cwd / '.logfire') | ||
|
|
||
| # Mock distributions so the test is environment-independent | ||
| class MockDist: | ||
| def __init__(self, metadata: dict[str, str]): | ||
| self.metadata = metadata | ||
|
|
||
| mock_pkgs = ['django', 'fastapi', 'flask', 'httpx', 'pymongo', 'redis', 'requests', 'sqlalchemy'] | ||
| monkeypatch.setattr('importlib.metadata.distributions', lambda: [MockDist({'Name': p}) for p in mock_pkgs]) | ||
| monkeypatch.setattr('logfire._internal.cli.run.is_uv_installed', lambda: True) | ||
|
|
||
| with pytest.raises(SystemExit): | ||
| main(['inspect']) | ||
| assert capsys.readouterr().err == snapshot("""\ | ||
|
|
||
|
|
||
| ╭───────────────────────────────────────────────────────────────── Logfire Summary ──────────────────────────────────────────────────────────────────╮ | ||
| │ │ | ||
| │ ☐ botocore (need to install opentelemetry-instrumentation-botocore) │ | ||
| │ ☐ jinja2 (need to install opentelemetry-instrumentation-jinja2) │ | ||
| │ ☐ pymysql (need to install opentelemetry-instrumentation-pymysql) │ | ||
| │ ☐ urllib (need to install opentelemetry-instrumentation-urllib) │ | ||
| │ ☐ django (need to install logfire[django]) │ | ||
| │ ☐ fastapi (need to install logfire[fastapi]) │ | ||
| │ ☐ flask (need to install logfire[flask]) │ | ||
| │ ☐ httpx (need to install logfire[httpx]) │ | ||
| │ ☐ pymongo (need to install logfire[pymongo]) │ | ||
| │ ☐ redis (need to install logfire[redis]) │ | ||
| │ ☐ requests (need to install logfire[requests]) │ | ||
| │ ☐ sqlalchemy (need to install logfire[sqlalchemy]) │ | ||
| │ ☐ sqlite3 (need to install logfire[sqlite3]) │ | ||
| │ ☐ urllib (need to install logfire[urllib]) │ | ||
| │ │ | ||
| │ │ | ||
| │ To install all recommended packages at once, run: │ | ||
| │ │ | ||
| │ uv add opentelemetry-instrumentation-botocore opentelemetry-instrumentation-jinja2 opentelemetry-instrumentation-pymysql │ | ||
| │ opentelemetry-instrumentation-urllib │ | ||
| │ uv add 'logfire[django,fastapi,flask,httpx,pymongo,redis,requests,sqlalchemy,sqlite3,urllib]' │ | ||
| │ # or │ | ||
| │ pip install 'logfire[django,fastapi,flask,httpx,pymongo,redis,requests,sqlalchemy,sqlite3,urllib]' │ | ||
| │ │ | ||
| │ ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── │ | ||
| │ │ | ||
|
devin-ai-integration[bot] marked this conversation as resolved.
|
||
|
|
@@ -1625,9 +1645,50 @@ def test_instrumented_packages_text_basic(): | |
| 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) | ||
|
Comment on lines
1645
to
+1650
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Tests fail in CI because CI runs tests via
With Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
1645
to
+1650
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 UV=1 environment variable in tests not isolated Several new tests ( Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
|
|
||
| def test_get_recommendation_texts_with_extras(): | ||
| recs = { | ||
| ('opentelemetry-instrumentation-requests', 'requests'), | ||
| ('opentelemetry-instrumentation-sqlite3', 'sqlite3'), | ||
| } | ||
| recommended, install = get_recommendation_texts(recs) | ||
| assert "pip install 'logfire[requests,sqlite3]'" in str(install) | ||
| assert 'need to install logfire[requests]' in str(recommended) | ||
| assert 'need to install logfire[sqlite3]' in str(recommended) | ||
|
|
||
|
|
||
| def test_get_recommendation_texts_uv(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setattr('logfire._internal.cli.run.is_uv_installed', lambda: True) | ||
| recs = {('opentelemetry-instrumentation-requests', 'requests')} | ||
| _, install = get_recommendation_texts(recs) | ||
| assert "uv add 'logfire[requests]'\n # or\n pip install 'logfire[requests]'" in str(install) | ||
|
|
||
|
|
||
| def test_get_recommendation_texts_uv_run(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setenv('UV', '1') | ||
| monkeypatch.setattr('sys.executable', '/path/to/venv/bin/python') | ||
| recs = { | ||
| ('opentelemetry-instrumentation-requests', 'requests'), | ||
| ('opentelemetry-instrumentation-jinja2', 'jinja2'), | ||
| } | ||
| monkeypatch.setattr(sys, 'argv', ['logfire', 'run', 'myapp.py']) | ||
| _, install = get_recommendation_texts(recs) | ||
| assert "uv run --with 'logfire[requests]' --with opentelemetry-instrumentation-jinja2 logfire run myapp.py" in str( | ||
| install | ||
| ) | ||
|
|
||
|
|
||
| def test_get_recommendation_texts_uvx(monkeypatch: pytest.MonkeyPatch): | ||
| monkeypatch.setenv('UV', '1') | ||
| monkeypatch.setattr('sys.executable', '/Users/user/.cache/uv/tools/logfire/bin/python') | ||
| recs = {('opentelemetry-instrumentation-requests', 'requests')} | ||
| monkeypatch.setattr(sys, 'argv', ['logfire', 'run', 'myapp.py']) | ||
| _, install = get_recommendation_texts(recs) | ||
| assert "uvx --from 'logfire[requests]' logfire run myapp.py" in str(install) | ||
|
|
||
|
|
||
| def test_instrument_packages_openai() -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
str.replace(' ', ' ')corrupts shell-quoted arguments containing double spacesThe
_full_install_commandfunction uses.replace(' ', ' ')to clean up double spaces caused by empty list joins in the f-string. However,str.replacedoesn't understand shell quoting, so it will also corrupt legitimate double spaces inside shell-quotedsys.argvvalues. For example, ifsys.argvcontains'arg with spaces',shlex.joincorrectly 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
Was this helpful? React with 👍 or 👎 to provide feedback.