Feat: local mode, and allowing empty/unset API key to be passed for local_mode#7
Feat: local mode, and allowing empty/unset API key to be passed for local_mode#7apelsynca wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Looked at the diff. The apply_payload_limits refactor and the test changes are fine. Three things to fix:
1. (blocking) local_mode is missing in the async client - async_client.py log() only checks self._closed, so with AsyncLogTideClient and local_mode=True logs still get sent. And since local_mode=True allows api_key=None, the async client will attempt to send with a null key. The same guard from the sync client needs to be replicated:
if self._closed or self.options.local_mode is True:
return
if self.options.local_mode == "if_unset_api_key" and not self.options.api_key:
return2. dsn now always overwrites explicit api_url/api_key (models.py __post_init__). Before, explicit values won (if not self.api_url: ...); now dsn always wins and hand-passed values are silently dropped. Intentional?
3. default api_url = production + relaxed validation (models.py). With api_url now defaulting to https://api.logtide.dev, building ClientOptions(api_key="...") without a url or dsn no longer raises and silently points at prod. It used to raise ValueError. A misconfig ends up in production unnoticed.
Minor: dead comment # if self.options and the two TODOs in log(), plus a typo in the error message ("other then" -> "other than").
…d, removed unused metadata check
Polliog
left a comment
There was a problem hiding this comment.
Re-checked after the refactor. The three original points are resolved: async now honors local_mode via BaseClient._is_logging_disabled(), the dsn-overwrites-explicit behavior is gone, and api_url no longer defaults to production. Nice.
Remaining changes requested:
1. Keep dsn as an option instead of the from_dsn factory. The other SDKs expose dsn as a first-class option, so the Python SDK should match for parity. Concretely:
- restore
dsn: str | None = Noneas a field api_urlback tostr | None = None(not a required positional)- in
__post_init__, ifdsnis set, parse it and fillapi_url/api_key. Pick a precedence and document it (dsn wins, orself.api_url = self.api_url or parts.api_urlif explicit should win) - validation: with
local_modeoff, requiredsnor (api_url+api_key) - drop
from_dsnoncedsnis a field. This also removes the mutable defaultglobal_metadata={}and the ~18-field duplication in its signature.
2. Initialize self._trace_id in BaseClient.__init__. Right now the base class reads self._trace_id (in get_trace_id / _pin_trace_id_to_entry) but only the subclasses set it. A subclass that forgets it crashes with AttributeError. Set it to None in the base.
3. Restore the metadata None coercion. The old if entry.metadata is None: entry.metadata = {} was dropped, so client.log(LogEntry(..., metadata=None)) now crashes on {**entry.metadata}. Minor, but it used to be handled.
Nits: redundant validation (BaseClient.__init__ raises RuntimeError for the missing-key case that ClientOptions.__post_init__ already rejects), and typo in the error message: "other then" -> "other than".
local_mode itself is good, leave it as is.
A bit of refactor (for the tests mostly), also moved function _process_value from client.py to payload_limits.py
And renamed it to apply_payload_limits (underscore was bad cuz async_client.py was also importing it)
In local mode (or if local_mode is 'if_unset_api_key' and the api_key unset) the client on
logmethod call does nothing.This allows for easy functionale in test environments, dev environments where you dont want to set
api_keyor just want to stop logtide from working