Skip to content

Feat: local mode, and allowing empty/unset API key to be passed for local_mode#7

Open
apelsynca wants to merge 11 commits into
logtide-dev:mainfrom
apelsynca:feat/no-send-mode
Open

Feat: local mode, and allowing empty/unset API key to be passed for local_mode#7
apelsynca wants to merge 11 commits into
logtide-dev:mainfrom
apelsynca:feat/no-send-mode

Conversation

@apelsynca

Copy link
Copy Markdown
Contributor

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 log method call does nothing.

This allows for easy functionale in test environments, dev environments where you dont want to set api_key or just want to stop logtide from working

@Polliog Polliog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
    return

2. 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").

@Polliog Polliog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = None as a field
  • api_url back to str | None = None (not a required positional)
  • in __post_init__, if dsn is set, parse it and fill api_url/api_key. Pick a precedence and document it (dsn wins, or self.api_url = self.api_url or parts.api_url if explicit should win)
  • validation: with local_mode off, require dsn or (api_url + api_key)
  • drop from_dsn once dsn is a field. This also removes the mutable default global_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants