Skip to content
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

Improve logfire inspect #727

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Dec 24, 2024

This PR adds the following arguments to logfire inspect:

  • --explain: Shouldn't be used frequently, but it helps, because it mentions the reason why the opentelemetry package was suggested.
  • --ignore-package: Used to ignore packages that user don't want to instrument.
  • --ignore-standard-library: Redundant if you use --ignore-package urllib --ignore-package sqlite3, but helps as a shortcut.

I've added a logic to handle packages that are only required by opentelemetry, like what is mentioned here: #330.


Suggestions welcome on how to test this.

Copy link

cloudflare-workers-and-pages bot commented Dec 24, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9495827
Status: ✅  Deploy successful!
Preview URL: https://b8e4b353.logfire-docs.pages.dev
Branch Preview URL: https://kludex-improve-inspect-comma.logfire-docs.pages.dev

View logs

@Kludex Kludex force-pushed the kludex/improve-inspect-command branch from 6afdee2 to 9495827 Compare December 24, 2024 13:58
Comment on lines -209 to -220
def test_inspect_drop_dependant_packages(
tmp_dir_cwd: Path, logfire_credentials: LogfireCredentials, capsys: pytest.CaptureFixture[str]
) -> None:
logfire_credentials.write_creds_file(tmp_dir_cwd / '.logfire')
with ExitStack() as stack:
find_spec = stack.enter_context(patch('importlib.util.find_spec'))
find_spec.side_effect = [True, None] * len(OTEL_PACKAGES)

main(['inspect'])
output = capsys.readouterr().err
assert 'opentelemetry-instrumentation-fastapi' in output
assert 'opentelemetry-instrumentation-starlette' not in output
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now fails because we handle the logic in a more assertive way.

@Kludex Kludex requested a review from alexmojaki December 24, 2024 13:59
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 58.53659% with 17 lines in your changes missing coverage. Please review.

Project coverage is 99.83%. Comparing base (8434258) to head (9495827).

Files with missing lines Patch % Lines
logfire/_internal/cli.py 57.50% 11 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #727      +/-   ##
===========================================
- Coverage   100.00%   99.83%   -0.17%     
===========================================
  Files          139      139              
  Lines        11206    11235      +29     
  Branches      1572     1583      +11     
===========================================
+ Hits         11206    11217      +11     
- Misses           0       12      +12     
- Partials         0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexmojaki
Copy link
Contributor

This doesn't pick up packages that are merely installed with e.g. uv pip install:

$ uv venv
Using CPython 3.12.6 interpreter at: /Users/alex/.pyenv/versions/3.12.6/bin/python
Creating virtual environment at: .venv
Activate with: source .venv/bin/activate
$ source .venv/bin/activate
$ uv pip install fastapi sqlalchemy logfire
Resolved 34 packages in 6ms
Installed 34 packages in 66ms
$ python -m logfire inspect  # works with logfire from pypi

The following packages from your environment have an OpenTelemetry instrumentation that is not installed:
┏━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Package    ┃ OpenTelemetry instrumentation package    ┃
┡━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
│ fastapi    │ opentelemetry-instrumentation-fastapi    │
│ requests   │ opentelemetry-instrumentation-requests   │
│ sqlalchemy │ opentelemetry-instrumentation-sqlalchemy │
│ sqlite3    │ opentelemetry-instrumentation-sqlite3    │
│ urllib     │ opentelemetry-instrumentation-urllib     │
│ urllib3    │ opentelemetry-instrumentation-urllib3    │
└────────────┴──────────────────────────────────────────┘

$ uv pip install -e ../logfire  # install this branch
Uninstalled 1 package in 5ms
Installed 1 package in 1ms
 - logfire==2.11.0
 + logfire==2.11.0 (from file:///Users/alex/work/logfire)
$ python -m logfire inspect
# No output

@alexmojaki
Copy link
Contributor

And when I use it in the logfire venv it tells me to install opentelemetry-instrumentation-aiohttp-client even though it's already installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants