Add _level param to @logfire.instrument()#1871
Add _level param to @logfire.instrument()#1871imp-joshi wants to merge 6 commits intopydantic:mainfrom
Conversation
- adds _level: LevelName | int | None = None to Logfire.instrument() - level attrs merged into span attributes at decoration time (static) - _level=None leaves the existing fast path untouched - test: warn-level span carries logfire.level_num=13
- extracts level_num at decoration time for O(1) call-time check - guard (level_num < config.min_level) added to all three open_span variants - NoopSpan._span property returns trace_api.INVALID_SPAN - set_user_attributes_on_raw_span exits cleanly via is_recording(); fixes latent crash on record_return=True
- async path works via same open_span closure, verified with snapshot (logfire.level_num: 13) - extract_args + _level test verifies both attrs appear together - shim gets explicit _level=None kwarg
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if _level is not None: | ||
| _level_attrs = log_level_attributes(_level) | ||
| level_num = int(_level_attrs[ATTRIBUTES_LOG_LEVEL_NUM_KEY]) | ||
| attributes = {**attributes, **_level_attrs} |
There was a problem hiding this comment.
🚩 Shared mutable attributes dict in _instrument_span_with_args is a pre-existing concern
The attributes dict created in get_open_span is captured by the open_span closures and passed to _instrument_span_with_args (logfire/_internal/main.py:282-299), which mutates it in-place (adding logfire.msg, logfire.json_schema, and function arg keys). This means the dict accumulates state across calls. It currently works because each call overwrites the same keys, but it's fragile — e.g., if a function had **kwargs and different keyword arguments were passed on different calls, stale keys from previous calls would persist. This is pre-existing and not introduced by this PR; the PR's {**attributes, **_level_attrs} copy at logfire/_internal/instrument.py:140 doesn't change this dynamic since the new dict is still the one that gets mutated on subsequent calls.
Was this helpful? React with 👍 or 👎 to provide feedback.
…or py3.9/3.10 compat
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| def instrument(self, *args, _level=None, **kwargs): | ||
| def decorator(func): | ||
| return func | ||
|
|
There was a problem hiding this comment.
🚩 *logfire-api shim uses args to accept positional func arg but silently replaces it with decorator
In the logfire-api no-op shim, instrument(self, *args, _level=None, **kwargs) always returns decorator (which takes a func and returns it). When used as @logfire.instrument (without parens), instrument(func) returns decorator rather than decorator(func), so my_func becomes decorator — a function that takes one arg and returns it, rather than the original function. This is a pre-existing issue in the logfire-api shim (not introduced by this PR), and is unlikely to matter in practice since the shim is only used when logfire is not installed.
(Refers to lines 134-138)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Closes #1452.
Every span API in logfire (
logfire.span(),logfire.debug(),logfire.warn(), etc.) accepts a_levelparameter that tags the span with a log level and respectsmin_levelfiltering.@logfire.instrument()was the only one that didn't. This PR closes that gap.What changed
You can now do:
and it behaves exactly like a manual
with logfire.span("my span", _level="warn"):block. The span gets the warn-level attributes, and if yourmin_levelis above warn, the span is suppressed entirely.The
_level=Nonedefault is completely untouched, no overhead on existing code.Implementation note
log_level_attributes(_level)is called once at decoration time and merged into the attribute dict. At call time the only extra work islevel_num < config.min_levelbefore deciding whether to proceed or return aNoopSpan.NoopSpan._span (pre-existing bug, fixed here)
When
open_spanreturns aNoopSpan(now an intentional code path, not just an error case),record_return=Truewould crash trying to call.is_recording()on a lambda returned by__getattr__. Added_spanas an explicit property returningtrace_api.INVALID_SPANsoset_user_attributes_on_raw_spanexits cleanly.Tests
test_instrument_with_level- warn level, verifieslogfire.level_num: 13on the spantest_instrument_level_filtered- debug withmin_level="info", nothing exportedtest_instrument_level_filtered_record_return- same but withrecord_return=True(exercises the NoopSpan fix)test_instrument_with_level_async- async pathtest_instrument_with_level_and_extract_args- level attrs and function args coexisttest_instrument_level_filtered_extract_args- filtering withextract_args=Truetest_instrument_level_filtered_extract_args_iterable- filtering withextract_args=('x',)