Move managed variable registry onto config#2034
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughVariable storage for 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
logfire/_internal/main.py (1)
2786-2788:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the remaining default-variable docs to config scope.
These methods call the now config-scoped
variables_get(), so their default includes variables fromwith_settings()siblings too, not just variables registered on this wrapper instance.Suggested wording update
- variables: Variable instances to push. If None, all variables - registered with this Logfire instance will be pushed. + variables: Variable instances to push. If None, all variables + registered with this Logfire instance's config will be pushed. ... - variables: Variable instances to validate. If None, all variables - registered with this Logfire instance will be validated. + variables: Variable instances to validate. If None, all variables + registered with this Logfire instance's config will be validated. ... - variables: Variable instances to include. If None, uses all registered variables. + variables: Variable instances to include. If None, uses all variables registered with this config.Also applies to: 2895-2897, 2998-3000
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@logfire/_internal/main.py` around lines 2786 - 2788, Update the docstring documentation for the default behavior of the variables parameter across three methods (locations at lines 2786-2788, 2895-2897, and 2998-3000) in the file. The current documentation states that when variables is None, all variables registered with the Logfire instance will be pushed, but since these methods now call the config-scoped variables_get() method, the documentation should reflect that the default includes variables from with_settings() siblings as well, not just variables registered on the specific wrapper instance. Update each docstring to clarify the expanded scope of variable inclusion when None is passed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_push_variables.py`:
- Around line 1290-1325: The tests
test_with_settings_duplicate_variable_names_conflict and
test_variables_clear_clears_with_settings_siblings use the global Logfire()
which accesses GLOBAL_CONFIG, causing variables registered in one test to
persist into subsequent tests and create order-dependent failures. Fix this by
either using a fresh isolated config fixture for each test instead of the shared
global Logfire instance, or by adding a teardown/cleanup step that calls
variables_clear() after each test to ensure the shared registry is reset between
test runs.
---
Outside diff comments:
In `@logfire/_internal/main.py`:
- Around line 2786-2788: Update the docstring documentation for the default
behavior of the variables parameter across three methods (locations at lines
2786-2788, 2895-2897, and 2998-3000) in the file. The current documentation
states that when variables is None, all variables registered with the Logfire
instance will be pushed, but since these methods now call the config-scoped
variables_get() method, the documentation should reflect that the default
includes variables from with_settings() siblings as well, not just variables
registered on the specific wrapper instance. Update each docstring to clarify
the expanded scope of variable inclusion when None is passed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43c493f4-4c91-4909-b370-c7ea75f79169
📒 Files selected for processing (4)
logfire/_internal/config.pylogfire/_internal/main.pytests/test_push_variables.pytests/test_variable_composition.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/pydantic(auto-detected)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_push_variables.py (1)
1256-1283: ⚡ Quick winPrefer user-facing API over internal
Logfire()constructor.These tests import and directly instantiate
Logfire()from the internal module. Per maintainer guidance, tests should use the user-facinglogfiremodule (preconfigured by the test framework) or calllogfire.configure()rather than importing internal constructors.While these isolated instances don't cause test pollution, using internal APIs makes tests fragile to refactoring and doesn't match actual usage patterns.
♻️ Refactor to use user-facing API
def test_var_registers_variable() -> None: """Test that var() registers variables with the logfire instance.""" - from logfire._internal.main import Logfire - - lf = Logfire() - assert lf.variables_get() == [] + logfire.variables_clear() + assert logfire.variables_get() == [] - var1 = lf.var(name='test_var_1', default=True, type=bool) - assert len(lf.variables_get()) == 1 - assert lf.variables_get()[0] is var1 + var1 = logfire.var(name='test_var_1', default=True, type=bool) + assert len(logfire.variables_get()) == 1 + assert logfire.variables_get()[0] is var1 - var2 = lf.var(name='test_var_2', default=42, type=int) - assert len(lf.variables_get()) == 2 - assert var2 in lf.variables_get() + var2 = logfire.var(name='test_var_2', default=42, type=int) + assert len(logfire.variables_get()) == 2 + assert var2 in logfire.variables_get() + + logfire.variables_clear()Apply the same pattern to
test_get_variables_returns_all_registered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_push_variables.py` around lines 1256 - 1283, Replace the direct instantiation of the internal Logfire class in test_get_variables_returns_all_registered with the public user-facing logfire API. Remove the import statement from logfire._internal.main and the line lf = Logfire(), then replace all references to the lf object with the preconfigured logfire module that is available from the test framework. Update all calls like lf.var() and lf.variables_get() to use logfire.var() and logfire.variables_get() instead. Apply the same refactoring to the preceding test function that also uses the internal Logfire constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_push_variables.py`:
- Around line 1256-1283: Replace the direct instantiation of the internal
Logfire class in test_get_variables_returns_all_registered with the public
user-facing logfire API. Remove the import statement from logfire._internal.main
and the line lf = Logfire(), then replace all references to the lf object with
the preconfigured logfire module that is available from the test framework.
Update all calls like lf.var() and lf.variables_get() to use logfire.var() and
logfire.variables_get() instead. Apply the same refactoring to the preceding
test function that also uses the internal Logfire constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e899ccbe-7b87-4176-b0b8-c54ea98c1047
📒 Files selected for processing (2)
logfire/_internal/main.pytests/test_push_variables.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/pydantic-ai(auto-detected)pydantic/pydantic(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (1)
- logfire/_internal/main.py
1 participant
Summary
LogfireConfiginstead of eachLogfirewrapper.Logfire.var()/template_var()as the public API while makingwith_settings()siblings share registered variables.Tests
uv run pytest tests/test_push_variables.py tests/test_variable_composition.py tests/test_variable_templates.pyuv run pyright logfire/_internal/config.py logfire/_internal/main.py logfire/variables/variable.py tests/test_push_variables.py tests/test_variable_composition.py