Add on_change callbacks for managed variables#1990
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Reworked on top of main now that composition and templating (#1954) have landed: - Variable.on_change() registers callbacks fired when the variable's effective value may have changed. - Change notifications are composition-aware: when a variable's config changes, dependents that (transitively) reference it via @{ref}@ — in server-stored values or code defaults — are notified too, with reference and changed names normalized through the alias map. - Providers report config-level diffs (local create/update/delete, remote refresh) including changed variables' aliases; the remote provider notifies outside its refresh lock to avoid deadlocks. - Change listeners live on LogfireConfig (not the provider), so they survive reconfiguration, and registration is idempotent so variables_clear()/re-registration cycles can't duplicate notifications.
dmontagu
force-pushed
the
managed-variables-on-change-merge
branch
from
629dbb6 to
6c310d0
Compare
…mpotency docs Follow-up addressing review of the on_change semantics: - Only fire when resolution-relevant config (name, labels, rollout, overrides, latest_version, aliases) changes; metadata-only edits like descriptions no longer notify. Local update_variable now diffs the same way as remote refresh, so the two providers share one contract (an identical-config update is silent on both). - Snapshot the variables registry and callback/listener lists before iterating, so registration on another thread (or from inside a callback) can't disrupt an in-flight dispatch. - Document the contract: callbacks must be idempotent (changes to unserved labels or other targeting keys' values still fire), run on the polling thread, must not mutate variables, and don't fire on the provider's initial load — call your handler once after registering to reconcile at startup. - Characterization tests pinning the conservative over-notification (unserved label changes fire, metadata-only changes don't), plus re-entrant registration and local/remote consistency tests.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Docs Preview
Re-add the |
…r lifecycle - remote.py: fire on_change on the first successful fetch (diff against an empty config), so a variable transitioning from its code default to the server value notifies. Eager + idempotent per the on_change contract. - variable.py: in expand_config_changes, treat a callable code default as referencing every variable (any change effectively changes it) rather than silently skipping it, since we can't introspect the callable's refs. - main.py/config.py/variable.py: registering an on_change callback now starts the background poller (Logfire._ensure_variable_change_polling), so callbacks fire even without an eager get(); the request is recorded on LogfireConfig so it survives reconfiguration. variables_clear() resets it. - docs: apply review suggestion (import logfire, drop undefined invalidate_cache, make the snippet runnable); update initial-load note to best-effort delivery. - tests: first-fetch notification, callable-default unrelated-change, poller-starts-on-registration, poller-survives-reconfigure; updated existing remote change-detection tests for the new first-fetch behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| fetch it before your callback is registered, so don't rely on it firing. To reliably | ||
| reconcile once at startup, call your handler yourself right after registering it: | ||
|
|
||
| ```python |
There was a problem hiding this comment.
this is basically the same snippet as the first
| # are started when first accessed via get_variable_provider(). | ||
| if config.variables is not None: | ||
| config.get_variable_provider().start(logfire_instance if config.variables.instrument else None) | ||
| elif config._variable_change_polling_requested: # pyright: ignore[reportPrivateUsage] |
There was a problem hiding this comment.
Maybe polling should start if any variable is defined? Then it has a chance of being ready by the first .get().
| # the changed variable). `changed_names` is always non-empty here, so include them all. | ||
| if changed_names: |
There was a problem hiding this comment.
"changed_names is always non-empty here"
so why the if condition?
…bles-on-change-merge
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a change notification system for managed variables. A new 🚥 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🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
logfire/variables/variable.py (1)
341-341: Variable.on_change lacks idempotency guard unlike provider's add_on_config_change.The provider's
add_on_config_changedocuments idempotent registration (line 1166:if callback not in self._on_config_change_callbacks), butVariable.on_changeappends unconditionally, so registering the same callback twice will invoke it twice per change. This likely mirrors intended usage (decorator-style callbacks are typically unique lambdas that don't compare equal), but the asymmetry warrants consistency consideration if idempotency becomes a requirement.🤖 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/variables/variable.py` at line 341, The Variable.on_change method at line 341 appends callbacks unconditionally, allowing duplicate registrations, while the provider's add_on_config_change method uses an idempotency check (if callback not in self._on_config_change_callbacks). To maintain consistency, add a conditional check in Variable.on_change to verify the callback is not already in self._on_change_callbacks before appending it, mirroring the pattern used in the provider's add_on_config_change method.
🤖 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 `@logfire/variables/local.py`:
- Line 101: The issue is that old_config is a mutable object stored by
reference, so if a caller modifies a returned config and then calls
update_variable, the comparison in changed_config_keys will use an
already-modified old_config instead of the true original state, causing real
changes to be missed. Create a snapshot copy of the configuration before calling
_notify_config_change and changed_config_keys at line 101, and apply the same
snapshot approach to the other affected locations at lines 120-124 and 139-141,
ensuring that the diff comparison always operates on the original unmodified
state of old_config.
---
Nitpick comments:
In `@logfire/variables/variable.py`:
- Line 341: The Variable.on_change method at line 341 appends callbacks
unconditionally, allowing duplicate registrations, while the provider's
add_on_config_change method uses an idempotency check (if callback not in
self._on_config_change_callbacks). To maintain consistency, add a conditional
check in Variable.on_change to verify the callback is not already in
self._on_change_callbacks before appending it, mirroring the pattern used in the
provider's add_on_config_change method.
🪄 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: 241734c9-7cf7-42f3-a6f0-adb22acbf2f3
📒 Files selected for processing (8)
docs/reference/advanced/managed-variables/remote.mdlogfire/_internal/config.pylogfire/_internal/main.pylogfire/variables/abstract.pylogfire/variables/local.pylogfire/variables/remote.pylogfire/variables/variable.pytests/test_variables.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
pydantic/pydantic(auto-detected)
| raise VariableAlreadyExistsError(f"Variable '{config.name}' already exists") | ||
| self._config.variables[config.name] = config | ||
| self._config._invalidate_alias_map() # pyright: ignore[reportPrivateUsage] | ||
| self._notify_config_change(changed_config_keys(config)) |
There was a problem hiding this comment.
Snapshot configs before diffing notifications.
old_config is a live mutable object, and the provider also returns/stores VariableConfig objects by reference. If a caller edits a returned config and then calls update_variable, the “old” config can already contain the new labels/aliases, causing on_change to be skipped for a real resolution-relevant change.
Suggested boundary-copy shape
def get_variable_config(self, name: str) -> VariableConfig | None:
...
with self._lock:
- return self._config._get_variable_config(name) # pyright: ignore[reportPrivateUsage]
+ config = self._config._get_variable_config(name) # pyright: ignore[reportPrivateUsage]
+ return config.model_copy(deep=True) if config is not None else None
def create_variable(self, config: VariableConfig) -> VariableConfig:
...
with self._lock:
+ config = config.model_copy(deep=True)
if config.name in self._config.variables:
raise VariableAlreadyExistsError(f"Variable '{config.name}' already exists")
self._config.variables[config.name] = config
self._config._invalidate_alias_map() # pyright: ignore[reportPrivateUsage]
def update_variable(self, name: str, config: VariableConfig) -> VariableConfig:
...
with self._lock:
if name not in self._config.variables:
raise VariableNotFoundError(f"Variable '{name}' not found")
- old_config = self._config.variables[name]
- self._config.variables[name] = config
+ old_config = self._config.variables[name].model_copy(deep=True)
+ config = config.model_copy(deep=True)
+ self._config.variables[name] = config
self._config._invalidate_alias_map() # pyright: ignore[reportPrivateUsage]
def delete_variable(self, name: str) -> None:
...
- old_config = self._config.variables.pop(name)
+ old_config = self._config.variables.pop(name).model_copy(deep=True)Also applies to: 120-124, 139-141
🤖 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/variables/local.py` at line 101, The issue is that old_config is a
mutable object stored by reference, so if a caller modifies a returned config
and then calls update_variable, the comparison in changed_config_keys will use
an already-modified old_config instead of the true original state, causing real
changes to be missed. Create a snapshot copy of the configuration before calling
_notify_config_change and changed_config_keys at line 101, and apply the same
snapshot approach to the other affected locations at lines 120-124 and 139-141,
ensuring that the diff comparison always operates on the original unmodified
state of old_config.
| description=description, | ||
| ) | ||
| self._variables[name] = variable | ||
| self._config.add_variables_change_listener(self._on_variables_config_change) |
There was a problem hiding this comment.
I think #2034 should be merged first, and then this should change so that Logfire doesn't need to own callbacks. then there'll be 3 layers of callbacks instead of 4.
2 participants
Summary
Adds support for registering callbacks that fire when a managed variable's configuration changes, enabling reactive patterns like cache invalidation and feature-flag updates.
Reworked on top of current
mainnow that composition and templating (#1954) have landed, addressing the review feedback about composition and duplicate registration.What's added
Variable.on_change()decorator +_notify_change()(variable.py)expand_config_changes()(variable.py) expands provider-level config changes to every registered variable that (transitively) composes a changed variable via@{ref}@references — whether the reference appears in a server-stored value (any label orlatest_version) or in a code default. Reference and changed names are normalized through the alias map.VariableProvider.add_on_config_change()/_notify_config_change()(abstract.py), with idempotent registration. Providers report changed names including aliases (from both old and new configs), so dependents referencing a deleted variable via an alias are still notified.create/update/delete(local.py) and remote refresh diffing (remote.py). The remote provider notifies outside its refresh lock to avoid deadlocks if a callback triggers resolution or another refresh.LogfireConfig, not the provider (config.py): every provider the config creates (including the lazy-init path) is wired to the config's dispatcher, so notifications surviveconfigure()re-runs — including forwith_settings()instances — and there is no setup flag to get out of sync.Logfire._on_variables_config_changedispatches to registered variables (main.py).managed-variables/remote.mdNotification contract
update_variableis silent on both.on_changereturns the callback to make that easy.Usage
If another variable composes
feature_enabled(e.g. a prompt containing@{feature_enabled}@), itson_changecallbacks fire too whenfeature_enabledchanges.Tests
latest_version), aliases (registration, reference, deleted target), duplicate prevention aftervariables_clear(), reconfigure, remote diff detection, and characterization tests pinning the contract (metadata-only edits don't fire; unserved-label changes do; re-entrant registration is safe)