Conversation
|
@alexmojaki I've opened a new pull request, #1549, to work on those changes. Once the pull request is ready, I'll request review from you. |
Deploying logfire-docs with
|
| Latest commit: |
52db62c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7f1f4565.logfire-docs.pages.dev |
| Branch Preview URL: | https://managed-variables.logfire-docs.pages.dev |
735b295 to
ce01469
Compare
tests/test_push_variables.py
Outdated
| assert 'orphan-feature' in diff.orphaned_server_variables | ||
| assert 'my-feature' not in diff.orphaned_server_variables |
There was a problem hiding this comment.
Please replace as many assertions as possible with inline-snapshot
9c2962a to
1b755d0
Compare
1da19bc to
bcaefd8
Compare
- Add override_variables() context manager for batch overrides - Add explicit variant selection via use_variant() and get(variant=) - Add prompt_var() convenience for string prompt variables - Add var_bundle() for grouping related variables - Add VariableBundle class with override, get_all, and dict-like access - Add 19 new tests for all new functionality Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
logfire-api/logfire_api/__init__.py
Outdated
| def push_variables(*args, **kwargs) -> bool: | ||
| return False | ||
|
|
||
| def validate_variables(*args, **kwargs) -> bool: | ||
| return True |
There was a problem hiding this comment.
🟡 Missing push_variable_types method in logfire-api stub
The logfire-api stub is missing the push_variable_types method that is exposed in the main logfire module.
Click to expand
The main logfire module exports push_variable_types at logfire/__init__.py:100:
push_variable_types = DEFAULT_LOGFIRE_INSTANCE.push_variable_typesAnd includes it in __all__ at logfire/__init__.py:196.
However, the logfire-api stub package does not define this method in the Logfire class at logfire-api/logfire_api/__init__.py. This means libraries depending on logfire-api will not have access to this method through the stub.
The stub defines push_variables (line 277) and validate_variables (line 280) but is missing push_variable_types.
Impact: Type checkers and IDEs will not recognize push_variable_types when using the logfire-api package, and the method will not be available in the no-op implementation.
Recommendation: Add: def push_variable_types(*args, **kwargs) -> bool: return False
Was this helpful? React with 👍 or 👎 to provide feedback.
logfire/__init__.py
Outdated
| sync_config = DEFAULT_LOGFIRE_INSTANCE.sync_config | ||
| pull_config = DEFAULT_LOGFIRE_INSTANCE.pull_config | ||
| generate_config = DEFAULT_LOGFIRE_INSTANCE.generate_config |
There was a problem hiding this comment.
these names sound too generic and somehow related to logfire.configure. they should all have variables in their name. maybe even rename all these methods (except var) to start with variables_, e.g. logfire.variables_get, logfire.variables_push, etc, a bit like the instrument_ and metric_ 'namespaces'. it's a bit more autocomplete friendly. it would also be possible to implement logfire.variables.get etc in a way that works with both the modules and Logfire instances.
There was a problem hiding this comment.
that point aside, there's inconsistency in the verbs used: get vs pull and sync vs push. sync is especially unclear.
|
|
||
| return VariablesConfig(variables=variable_configs) | ||
|
|
||
| def merge(self, other: VariablesConfig) -> VariablesConfig: |
| config = logfire.generate_config() | ||
|
|
||
| # Save to a YAML file (JSON also supported) | ||
| config.write('variables.yaml') |
There was a problem hiding this comment.
this method doesn't seem to exist
logfire/_internal/config.py
Outdated
| self._variable_provider.shutdown() | ||
| if self.variables.config is None: | ||
| self._variable_provider = NoOpVariableProvider() | ||
| else: | ||
| # Need to move the imports here to prevent errors if pydantic is not installed | ||
| from logfire.variables import LocalVariableProvider, LogfireRemoteVariableProvider, VariablesConfig | ||
|
|
||
| if isinstance(self.variables.config, VariableProvider): | ||
| self._variable_provider = self.variables.config | ||
| elif isinstance(self.variables.config, VariablesConfig): | ||
| self._variable_provider = LocalVariableProvider(self.variables.config) | ||
| else: | ||
| assert_type(self.variables.config, RemoteVariablesConfig) | ||
| remote_config = self.variables.config | ||
| # Load api_key from config or environment variable | ||
| # Only API keys can be used for the variables API (not write tokens) | ||
| api_key = remote_config.api_key or self.param_manager.load_param('api_key') | ||
| if not api_key: | ||
| raise LogfireConfigError( # pragma: no cover | ||
| 'Remote variables require an API key. ' | ||
| 'Set the LOGFIRE_API_KEY environment variable or pass api_key to RemoteVariablesConfig.' | ||
| ) | ||
| # Determine base URL: prefer config, then advanced settings, then infer from token | ||
| base_url = self.advanced.base_url or get_base_url_from_token(api_key) | ||
| self._variable_provider = LogfireRemoteVariableProvider( | ||
| base_url=base_url, | ||
| token=api_key, | ||
| config=remote_config, | ||
| ) | ||
| self._variable_provider.start(None) |
There was a problem hiding this comment.
🟡 Remote variable provider can never receive a Logfire instance for error logging due to premature start(None)
The variable provider is started during LogfireConfig._initialize() with start(None), and then logfire.configure() attempts to start it again with the real Logfire instance. However, LogfireRemoteVariableProvider.start() is a one-shot: it returns early if _started is already True.
Root Cause
In logfire/_internal/config.py:1202-1231, after constructing the provider, the config initialization unconditionally calls:
self._variable_provider.start(None)
Then, in logfire/_internal/config.py:573-576 (shown in PR diff), configure() calls:
config.get_variable_provider().start(logfire_instance if config.variables.instrument else None)
But logfire/variables/remote.py:105-127 sets self._logfire only if logfire_instance is not None, and bails out early if self._started is already set.
So if the remote provider is started first with None, it can never later be given a Logfire instance for structured error logging.
Impact:
- Remote provider errors will be emitted via
warnings.warninstead of Logfire spans/logs even when variable instrumentation is enabled. - Also causes the remote provider threads to start even when the intention is “don’t start variable instrumentation” (because
_initializealways starts it).
Prompt for agents
Fix provider start lifecycle so the provider is not started with None during LogfireConfig._initialize(). In logfire/_internal/config.py around lines 1202-1231, remove (or gate) the unconditional `self._variable_provider.start(None)` call. Ensure the provider is started exactly once after `configure()` determines whether variable instrumentation is enabled and can pass a Logfire instance for error logging. Options:
1) Move provider start entirely out of _initialize and only start it from configure() (after Logfire instance is chosen).
2) Or modify VariableProvider.start contract to allow being called multiple times to update the logger (and adjust LogfireRemoteVariableProvider.start to set _logfire even when already started).
Also ensure that if VariablesOptions.instrument is False, the provider does not start background threads at all (or at least does not start SSE/polling threads).
Was this helpful? React with 👍 or 👎 to provide feedback.
| def var( | ||
| self, | ||
| name: str, | ||
| *, | ||
| type: type[T] | None = None, | ||
| default: T | ResolveFunction[T], | ||
| description: str | None = None, | ||
| ) -> Variable[T]: | ||
| from logfire.variables.variable import Variable | ||
|
|
||
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | ||
|
|
||
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | ||
| self._variables[name] = variable |
There was a problem hiding this comment.
🔴 logfire.var() infers an invalid type when default is a ResolveFunction and type is omitted
When Logfire.var() is called without an explicit type=..., it infers the type from default.__class__. If default is a resolve function (callable), this infers the type as the function’s class instead of the variable’s intended value type.
Actual behavior: provider values are validated/deserialized as a function object type adapter, causing variable resolution to fail validation or behave incorrectly.
Expected behavior: either require type when default is a resolve function, or infer the return type (not feasible at runtime) and raise a clear error.
Root Cause
In Logfire.var(), the type inference is:
tp = cast(Type[T], default.__class__ if type is None else type)
So passing a callable default without type yields tp == function.
Impact: Any variable declared like logfire.var(name='x', default=lambda targeting_key, attributes: 1) (no explicit type=int) will register a variable that can’t successfully validate remote JSON values and may return defaults unexpectedly.
| def var( | |
| self, | |
| name: str, | |
| *, | |
| type: type[T] | None = None, | |
| default: T | ResolveFunction[T], | |
| description: str | None = None, | |
| ) -> Variable[T]: | |
| from logfire.variables.variable import Variable | |
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | |
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | |
| self._variables[name] = variable | |
| def var( | |
| self, | |
| name: str, | |
| *, | |
| type: type[T] | None = None, | |
| default: T | ResolveFunction[T], | |
| description: str | None = None, | |
| ) -> Variable[T]: | |
| from logfire.variables.variable import Variable, is_resolve_function | |
| if type is None: | |
| if is_resolve_function(default): | |
| raise TypeError('When `default` is a resolve function, `type` must be provided.') | |
| tp = cast(Type[T], default.__class__) # noqa UP006 | |
| else: | |
| tp = cast(Type[T], type) # noqa UP006 | |
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | |
| self._variables[name] = variable | |
| return variable |
Was this helpful? React with 👍 or 👎 to provide feedback.
| def is_resolve_function(f: Any) -> TypeIs[ResolveFunction[Any]]: | ||
| """Check if a callable matches the ResolveFunction signature. | ||
|
|
||
| Args: | ||
| f: The object to check. | ||
|
|
||
| Returns: | ||
| True if the callable has a signature matching ResolveFunction. | ||
| """ | ||
| if not callable(f): | ||
| return False | ||
| signature = inspect.signature(f) | ||
| params = list(signature.parameters.values()) | ||
| if len(params) == 2 and params[0].name == 'targeting_key' and params[1].name == 'attributes': | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🔴 ResolveFunction detection depends on parameter names, breaking callable defaults and serialization
is_resolve_function() only returns True if the callable has exactly two parameters named exactly targeting_key and attributes. Many valid callables (lambdas, partials, functions using *args, or different parameter names) won’t match.
Actual behavior:
- A callable default may be treated as a literal default value, so
Variable._get_default()returns the function object instead of calling it. - Push/generate operations may attempt to JSON-serialize a callable default because
_get_default_serialized()thinks it is not a resolve function.
Expected behavior: any callable intended to be a resolve function should be detected based on callability and arity/compatibility (or a dedicated wrapper type), not strict parameter names.
Detailed Explanation
Current logic:
is_resolve_function()checks parameter names, not just count/compatibility:logfire/variables/variable.py:68-83._get_default_serialized()usesis_resolve_function()to decide whether to serialize a default:logfire/variables/abstract.py:290-301._get_default()usesis_resolve_function()to decide whether to call the default:logfire/variables/variable.py:317-324.
So a resolve function like lambda k, a: 1 is callable and takes 2 args, but it will not be detected, and downstream behavior becomes wrong.
Impact: callable defaults are unreliable and can cause incorrect values to be returned and/or crashes during push_variables() / generate_config().
Prompt for agents
In logfire/variables/variable.py, replace is_resolve_function’s parameter-name-based detection (lines 68-83) with a more robust check. For example: treat a callable as a resolve function if it can be called with two positional arguments (targeting_key, attributes), e.g. by inspecting signature for at least 2 positional parameters (or accepting *args/**kwargs). Ensure Variable._get_default() calls such callables, and logfire/variables/abstract.py:_get_default_serialized() avoids serializing any callable defaults. Add tests for lambda k,a:..., def f(k,a):..., and callables with *args/**kwargs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (context_overrides := _VARIABLE_OVERRIDES.get()) is not None and ( | ||
| context_value := context_overrides.get(self.name) | ||
| ) is not None: | ||
| if is_resolve_function(context_value): | ||
| context_value = context_value(targeting_key, attributes) | ||
| return ResolvedVariable(name=self.name, value=context_value, _reason='context_override') |
There was a problem hiding this comment.
🟡 Variable override contexts cannot override a variable to None
The context-override logic treats a missing override and an override value of None the same, because it checks context_overrides.get(self.name) is not None.
Actual behavior: with my_var.override(None): ... has no effect; resolution falls through to provider/default behavior.
Expected behavior: overriding to None should be honored (or explicitly rejected with a clear error), but it shouldn’t silently do nothing.
Root Cause
In Variable._resolve, overrides are applied only when the looked-up value is not None:
logfire/variables/variable.py:263-268
This makes it impossible to override a variable to None (even if the variable’s type allows it, e.g. Variable[int | None]).
Impact: tests and request-scoped overrides can’t represent “intentionally unset” values, which is especially relevant for feature flags or optional config.
| if (context_overrides := _VARIABLE_OVERRIDES.get()) is not None and ( | |
| context_value := context_overrides.get(self.name) | |
| ) is not None: | |
| if is_resolve_function(context_value): | |
| context_value = context_value(targeting_key, attributes) | |
| return ResolvedVariable(name=self.name, value=context_value, _reason='context_override') | |
| if (context_overrides := _VARIABLE_OVERRIDES.get()) is not None and self.name in context_overrides: | |
| context_value = context_overrides[self.name] | |
| if is_resolve_function(context_value): | |
| context_value = context_value(targeting_key, attributes) | |
| return ResolvedVariable(name=self.name, value=context_value, _reason='context_override') |
Was this helpful? React with 👍 or 👎 to provide feedback.
| from logfire.variables.variable import Variable | ||
|
|
||
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | ||
|
|
||
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | ||
| self._variables[name] = variable |
There was a problem hiding this comment.
🔴 Logfire.var() infers type from callable default, producing invalid TypeAdapter and breaking resolution
If a user passes a resolve function as default without explicitly passing type=..., Logfire.var() infers the type as default.__class__, which is function.
Actual: tp becomes function, so Variable builds TypeAdapter(function) and later validation/deserialization of provider JSON values will fail (or behave nonsensically).
Expected: When default is a resolve function, type should be required (or inferred another way) and a clear error raised.
Root Cause
Logfire.var() computes the type as:
tp = cast(Type[T], default.__class__ if type is None else type)
So when default is callable, the inferred type is wrong, but no guard exists.
Code: logfire/_internal/main.py:2446-2451.
Impact: User code like logfire.var(name='x', default=lambda targeting_key, attributes: 1) (missing type=int) will register a variable that fails at runtime during schema generation, serialization, or variant deserialization.
| from logfire.variables.variable import Variable | |
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | |
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | |
| self._variables[name] = variable | |
| from logfire.variables.variable import Variable, is_resolve_function | |
| if type is None: | |
| if is_resolve_function(default): | |
| raise TypeError('When `default` is a resolve function, `type` must be provided.') | |
| tp = cast(Type[T], default.__class__) # noqa UP006 | |
| else: | |
| tp = cast(Type[T], type) # noqa UP006 | |
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | |
| self._variables[name] = variable | |
| return variable |
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Register at_fork handler | ||
| if hasattr(os, 'register_at_fork'): # pragma: no branch | ||
| weak_reinit = weakref.WeakMethod(self._at_fork_reinit) | ||
| os.register_at_fork(after_in_child=lambda: weak_reinit()()) # pyright: ignore[reportOptionalCall] | ||
|
|
There was a problem hiding this comment.
🔴 Fork handler can crash by calling a dead WeakMethod in remote variable provider
The remote provider registers an os.register_at_fork callback that calls weak_reinit()().
Actual: If the WeakMethod is dead (returns None), the callback will raise TypeError: 'NoneType' object is not callable in the child process after fork.
Expected: The at-fork callback should safely no-op if the provider instance is gone.
Root Cause
In start(), it does:
weak_reinit = weakref.WeakMethod(self._at_fork_reinit)os.register_at_fork(after_in_child=lambda: weak_reinit()())
No guard exists for the None return.
Code: logfire/variables/remote.py:129-133.
Impact: In applications that fork after configuring Logfire, the child process can crash at fork time in edge cases where the provider instance is not strongly referenced (or during interpreter shutdown / unusual lifecycle), since at-fork callbacks run unconditionally.
| # Register at_fork handler | |
| if hasattr(os, 'register_at_fork'): # pragma: no branch | |
| weak_reinit = weakref.WeakMethod(self._at_fork_reinit) | |
| os.register_at_fork(after_in_child=lambda: weak_reinit()()) # pyright: ignore[reportOptionalCall] | |
| # Register at_fork handler | |
| if hasattr(os, 'register_at_fork'): # pragma: no branch | |
| weak_reinit = weakref.WeakMethod(self._at_fork_reinit) | |
| def _after_in_child() -> None: | |
| method = weak_reinit() | |
| if method is not None: | |
| method() | |
| os.register_at_fork(after_in_child=_after_in_child) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| def get_variable_config(self, name: str) -> VariableConfig | None: | ||
| """Retrieve the full configuration for a variable. | ||
|
|
||
| Args: | ||
| name: The name of the variable. | ||
|
|
||
| Returns: | ||
| The VariableConfig if found, or None if the variable doesn't exist. | ||
| """ | ||
| return self._config.variables.get(name) |
There was a problem hiding this comment.
🟡 Local provider ignores alias-based resolution implemented by VariablesConfig
VariablesConfig.resolve_serialized_value() supports resolving variables by aliases via _get_variable_config().
Actual: LocalVariableProvider.get_variable_config() bypasses that alias logic by directly doing self._config.variables.get(name).
Expected: get_variable_config() should use the same alias-aware lookup as the resolver so that all provider APIs (e.g., explicit variant fetch or config inspection) behave consistently.
Root Cause
LocalVariableProvider.get_variable_config() returns self._config.variables.get(name) rather than using the alias-aware _get_variable_config().
Code: logfire/variables/local.py:51-60.
Impact: If a variable is renamed and an alias is configured, get_serialized_value() will work via alias resolution, but calls that rely on get_variable_config() (e.g., VariableProvider.get_serialized_value_for_variant() default implementation in logfire/variables/abstract.py:580-593) will fail to find the config when using the alias name. This leads to inconsistent behavior depending on which API path is used.
| def get_variable_config(self, name: str) -> VariableConfig | None: | |
| """Retrieve the full configuration for a variable. | |
| Args: | |
| name: The name of the variable. | |
| Returns: | |
| The VariableConfig if found, or None if the variable doesn't exist. | |
| """ | |
| return self._config.variables.get(name) | |
| def get_variable_config(self, name: str) -> VariableConfig | None: | |
| """Retrieve the full configuration for a variable. | |
| Args: | |
| name: The name of the variable. | |
| Returns: | |
| The VariableConfig if found, or None if the variable doesn't exist. | |
| """ | |
| # Use the same alias-aware lookup as resolution. | |
| return self._config._get_variable_config(name) # pyright: ignore[reportPrivateUsage] |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| mode: Literal['local', 'remote', 'disabled'] = 'disabled' |
There was a problem hiding this comment.
🚩 VariablesOptions.mode field is defined but never used
The VariablesOptions class defines a mode field with type Literal['local', 'remote', 'disabled'] but this field is never checked or used anywhere in the codebase. The actual provider selection logic in logfire/_internal/config.py:1203-1230 is determined solely by the type of VariablesOptions.config, not by the mode field.
This suggests either:
- Incomplete implementation - the mode field was intended to provide explicit control but the logic was never added
- Dead code - the field was added but is redundant with type-based dispatch
If this is intentional, the field could be removed to avoid confusion. If incomplete, the field should be used to control provider selection (e.g., mode='disabled' could skip provider initialization even if config is set).
Was this helpful? React with 👍 or 👎 to provide feedback.
| from logfire.variables.variable import Variable | ||
|
|
||
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | ||
|
|
||
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | ||
| self._variables[name] = variable | ||
| return variable |
There was a problem hiding this comment.
🔴 logfire.var() infers type from ResolveFunction default, creating invalid TypeAdapter and crashing at runtime
Logfire.var() infers the variable type from default.__class__ when type isn’t provided. If the user provides a callable default (a ResolveFunction) and forgets to pass type=..., the inferred type becomes the function class, and Variable will build a pydantic.TypeAdapter for a function object type, which will fail during variable construction/resolution.
Actual: logfire.var(name='x', default=lambda targeting_key, attributes: 123) can raise at definition time (or later) due to invalid inferred type.
Expected: either require type when default is a resolve function, or infer the return type some other way (not feasible at runtime).
Root Cause
In logfire/_internal/main.py:2446-2452, type inference is:
tp = default.__class__ if type is None else type
For a resolve function, default.__class__ is a function type, not the intended T. That propagates into Variable(...), which constructs TypeAdapter(type) (logfire/variables/variable.py:122-134), causing runtime failures.
Impact: Misconfiguration leads to hard runtime crashes when defining variables with callable defaults.
| from logfire.variables.variable import Variable | |
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | |
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | |
| self._variables[name] = variable | |
| return variable | |
| from logfire.variables.variable import Variable, is_resolve_function | |
| if type is None and is_resolve_function(default): | |
| raise ValueError('When `default` is a resolve function, you must pass `type=...` to `logfire.var()`') | |
| tp = cast(Type[T], default.__class__ if type is None else type) # noqa UP006 | |
| variable = Variable[T](name, default=default, type=tp, logfire_instance=self, description=description) | |
| self._variables[name] = variable | |
| return variable |
Was this helpful? React with 👍 or 👎 to provide feedback.
| def matches(self, attributes: Mapping[str, Any]) -> bool: | ||
| """Check if the attribute value does not match the regex pattern.""" | ||
| value = attributes.get(self.attribute) | ||
| if not isinstance(value, str): | ||
| return False | ||
| return not re.search(self.pattern, value) |
There was a problem hiding this comment.
🟡 ValueDoesNotMatchRegex never matches when attribute is missing or non-string
ValueDoesNotMatchRegex.matches() returns False when the attribute is missing or not a string. For a “does not match regex” condition, missing/non-string values should typically be treated as “does not match” (i.e. True), consistent with other negated conditions in the same module (e.g. ValueDoesNotEqual and ValueIsNotIn treat missing values as a match).
Actual: Override rules using ValueDoesNotMatchRegex will not apply when the attribute is absent (or not a string), even though absence implies it cannot match the regex.
Expected: Missing/non-string should evaluate as not matching the regex and therefore satisfy the condition.
Detailed Explanation
In logfire/variables/config.py:135-140:
value = attributes.get(self.attribute)if not isinstance(value, str): return False
This makes the condition fail in common cases where the attribute isn’t provided in context.
Impact: Targeting/override behavior is incorrect for negated-regex conditions, causing unexpected rollout selection.
| def matches(self, attributes: Mapping[str, Any]) -> bool: | |
| """Check if the attribute value does not match the regex pattern.""" | |
| value = attributes.get(self.attribute) | |
| if not isinstance(value, str): | |
| return False | |
| return not re.search(self.pattern, value) | |
| def matches(self, attributes: Mapping[str, Any]) -> bool: | |
| """Check if the attribute value does not match the regex pattern.""" | |
| value = attributes.get(self.attribute) | |
| if not isinstance(value, str): | |
| # Missing or non-string values cannot match the pattern. | |
| return True | |
| return not re.search(self.pattern, value) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| span.set_attributes( | ||
| { | ||
| 'name': result.name, | ||
| 'value': result.value, | ||
| 'variant': result.variant, | ||
| 'reason': result._reason, # pyright: ignore[reportPrivateUsage] | ||
| } | ||
| ) |
There was a problem hiding this comment.
🔴 Resolving a variable records value span attribute with non-OTel-safe types (can raise at runtime)
Variable.get() unconditionally calls span.set_attributes({... 'value': result.value ...}), but result.value may be an arbitrary Python object (e.g. a Pydantic model, dict with non-primitive values, dataclass, etc.). OpenTelemetry span attributes only support primitives and sequences of primitives; passing unsupported types typically raises and can break the caller.
Root Cause / Impact
In logfire/variables/variable.py:237-244, the code sets:
value:result.value(user-defined type)reason:result._reason
Unlike the main span creation path which runs attributes through Logfire's prepare_otlp_attributes, this call directly forwards the raw value to the OTel SDK.
Actual: variable resolution can raise an exception while trying to set span attributes.
Expected: variable resolution should never crash due to observability, and values should either be serialized safely (e.g. JSON string) or omitted.
Impact: Any managed variable with a structured type (the primary use case in the new docs) can crash requests when instrumentation is enabled.
| span.set_attributes( | |
| { | |
| 'name': result.name, | |
| 'value': result.value, | |
| 'variant': result.variant, | |
| 'reason': result._reason, # pyright: ignore[reportPrivateUsage] | |
| } | |
| ) | |
| span.set_attributes( | |
| { | |
| 'name': result.name, | |
| # Avoid setting `value` directly: it may be a complex Python object and | |
| # OpenTelemetry span attributes only support primitives (and sequences of primitives). | |
| # The resolved value can still be inspected via application logs or by | |
| # serializing it explicitly in user code if needed. | |
| 'variant': result.variant, | |
| 'reason': result._reason, # pyright: ignore[reportPrivateUsage] | |
| } | |
| ) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| variables_response = self._session.get(urljoin(self._base_url, '/v1/variables/')) | ||
| UnexpectedResponse.raise_for_status(variables_response) | ||
| except Exception as e: | ||
| # Catch all request exceptions (ConnectionError, Timeout, UnexpectedResponse, etc.) | ||
| # to prevent crashing the user's application on network/HTTP failures. | ||
| self._log_error('Error retrieving variables', e) | ||
| return | ||
|
|
||
| variables_config_data = variables_response.json() | ||
| try: | ||
| self._config = VariablesConfig.model_validate(variables_config_data) | ||
| except ValidationError as e: | ||
| self._log_error('Failed to parse variables configuration from Logfire API', e) | ||
| finally: | ||
| self._has_attempted_fetch = True | ||
|
|
||
| self._last_fetched_at = datetime.now(tz=timezone.utc) |
There was a problem hiding this comment.
🔴 Remote variables refresh can crash background thread on invalid JSON because .json() isn’t in try/except
LogfireRemoteVariableProvider.refresh() performs variables_response.json() outside the request error handling. If the server returns invalid JSON (or an empty body, HTML error page from a proxy, etc.), Response.json() raises and the exception bubbles out of refresh(), potentially terminating the polling thread (and breaking block_before_first_resolve behavior).
Root Cause / Impact
In logfire/variables/remote.py:271-288, request errors are caught, but JSON decoding is not:
- request wrapped in
try/except Exception as e - then
variables_config_data = variables_response.json()is executed unguarded
Actual: invalid JSON can raise and escape refresh(). When called from _worker() (logfire/variables/remote.py:233-247), that can kill the worker thread and stop updates.
Expected: invalid JSON should be handled like other remote errors: log a warning/error and keep the provider running.
Impact: A transient upstream/proxy error that returns non-JSON can permanently stop managed variable refresh in a running process.
| try: | |
| variables_response = self._session.get(urljoin(self._base_url, '/v1/variables/')) | |
| UnexpectedResponse.raise_for_status(variables_response) | |
| except Exception as e: | |
| # Catch all request exceptions (ConnectionError, Timeout, UnexpectedResponse, etc.) | |
| # to prevent crashing the user's application on network/HTTP failures. | |
| self._log_error('Error retrieving variables', e) | |
| return | |
| variables_config_data = variables_response.json() | |
| try: | |
| self._config = VariablesConfig.model_validate(variables_config_data) | |
| except ValidationError as e: | |
| self._log_error('Failed to parse variables configuration from Logfire API', e) | |
| finally: | |
| self._has_attempted_fetch = True | |
| self._last_fetched_at = datetime.now(tz=timezone.utc) | |
| try: | |
| variables_response = self._session.get(urljoin(self._base_url, '/v1/variables/')) | |
| UnexpectedResponse.raise_for_status(variables_response) | |
| variables_config_data = variables_response.json() | |
| except Exception as e: | |
| # Catch all request/HTTP/JSON exceptions (ConnectionError, Timeout, UnexpectedResponse, | |
| # JSONDecodeError, etc.) to prevent crashing the user's application on failures. | |
| self._log_error('Error retrieving variables', e) | |
| return | |
| try: | |
| self._config = VariablesConfig.model_validate(variables_config_data) | |
| except ValidationError as e: | |
| self._log_error('Failed to parse variables configuration from Logfire API', e) | |
| finally: | |
| self._has_attempted_fetch = True | |
| self._last_fetched_at = datetime.now(tz=timezone.utc) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| @dataclass | ||
| class VariablesOptions: | ||
| """Configuration of managed variables.""" | ||
|
|
There was a problem hiding this comment.
🚩 VariablesOptions.mode field is defined but never read
The VariablesOptions dataclass defines a mode field at logfire/_internal/config.py:326, but searching through the codebase shows it's never actually read or used. The variable provider is instead determined solely by checking the type of config field in logfire/_internal/config.py:1209-1230.
This appears to be leftover from an earlier design or intended for future functionality. If it's meant to be used, the initialization logic should check it. If not, it should be removed to avoid confusion.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| Example: | ||
| ```python skip="true" | ||
| report = provider.validate_variables(variables) | ||
| if report.has_errors: | ||
| print(report.format()) | ||
| sys.exit(1) | ||
| ``` |
There was a problem hiding this comment.
🚩 ValidationReport property naming inconsistency with documentation
The documentation example at logfire/variables/abstract.py:197-204 shows:
if report.has_errors:
print(report.format())But the actual property is named is_valid (line 217-219), not has_errors. This is a documentation bug in the docstring example, though not a code bug. Users following the example will get an AttributeError.
The correct usage should be if not report.is_valid: or the class should provide a has_errors property as well.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
I'm going to open a new PR, this has gotten too long |
No description provided.