fix(logfire-api): support bare @logfire.instrument decorator#2047
fix(logfire-api): support bare @logfire.instrument decorator#2047syf2211 wants to merge 1 commit into
Conversation
When logfire is not installed, logfire-api's instrument() shim always returned the inner decorator function. Using @logfire.instrument without parentheses passed the target function as the first argument but never applied the decorator, replacing the function with decorator itself. Detect callable first arguments the same way the real Logfire.instrument does and apply the no-op decorator immediately. Fixes pydantic#1877
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR fixes the logfire-api no-op shim so @logfire.instrument (without parentheses) behaves correctly when the real logfire package is not installed, and adds a regression test to prevent the decorator from replacing the target function.
Changes:
- Update the
logfire-apishimLogfire.instrumentto immediately apply the no-op decorator when invoked as a bare decorator. - Add a regression test validating bare
@logfire_api.instrumentpreserves call behavior and function identity metadata.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_logfire_api.py | Adds a regression assertion for bare @logfire_api.instrument in the no-logfire (shim) import path. |
| logfire-api/logfire_api/init.py | Adjusts the shim instrument implementation to handle the bare-decorator invocation pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def instrument(self, msg_template=None, **kwargs): | ||
| def decorator(func): | ||
| return func | ||
|
|
||
| if callable(msg_template): | ||
| return decorator(msg_template) | ||
| return decorator |
There was a problem hiding this comment.
The real instrument only accepts a single positional arg (msg_template) — everything after it is keyword-only — so instrument('tpl', extra_positional) already raises TypeError when logfire is installed. Dropping *args makes the shim match that contract instead of silently accepting calls that would fail for real. And the callable() check mirrors the real SDK's own dispatch (if callable(msg_template): return self.instrument()(msg_template)), which is the whole point of the shim.
Summary
Fix the
logfire-apino-op shim so@logfire.instrument(without parentheses) returns the original function instead of the inner decorator.Motivation
When
logfireis not installed,logfire-apiprovides a lightweight stub. Itsinstrument()always returned the innerdecoratorfunction. With bare@logfire.instrument, Python passes the target function as the first positional argument, but the shim never applied the decorator — the decorated name becamedecoratoritself.This matches the behavior described in #1877 and mirrors the real SDK dispatch (
if callable(msg_template): return self.instrument()(msg_template)).Changes
Logfire.instrumentshim and apply the no-op decorator immediatelytest_logfire_api.pyfor the no-logfire code pathTests
uv run pytest tests/test_logfire_api.py -v— 3 passed, 1 skippeduv run ruff check logfire-api/logfire_api/__init__.py tests/test_logfire_api.py— passNotes
logfire-apistub used when the fulllogfirepackage is not installed@logfire.instrument()and@logfire.instrument('template')behavior unchangedFixes #1877