-
Notifications
You must be signed in to change notification settings - Fork 231
feat: surface X-Logfire-Warning / X-Logfire-Error response headers #1906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
9fbfb44
278a76c
9aadfa8
4808b4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| """Surface out-of-band signals the Logfire backend wants every SDK request to know about. | ||
|
|
||
| The server attaches custom headers to API responses: | ||
|
|
||
| * `X-Logfire-Warning`: an out-of-band warning the server wants the user to see. | ||
| Surfaced via `warnings.warn(..., LogfireServerWarning)`. Python's standard | ||
| "default" filter dedupes identical messages, so a chatty server only warns once. | ||
| * `X-Logfire-Error`: an out-of-band error the server wants the SDK to raise. | ||
| Always raised as `LogfireServerError`. Callers that want to keep working past | ||
| it (the OTLP pipeline, the variables provider) already swallow exceptions from | ||
| their HTTP calls; CRUD/CLI propagate the error to the user. | ||
|
|
||
| `install_logfire_response_hook(session)` wires this into a `requests.Session` as | ||
| a response hook so every Logfire-bound HTTP response is inspected. Callers can | ||
| pass a custom `hook` to replace the default behaviour (see | ||
| `AdvancedOptions.transport_response_hook`). | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import warnings | ||
| from typing import Any, Callable | ||
|
|
||
| import requests | ||
|
|
||
| from logfire.exceptions import LogfireServerError, LogfireServerWarning | ||
|
|
||
| WARNING_HEADER_NAME = 'X-Logfire-Warning' | ||
| ERROR_HEADER_NAME = 'X-Logfire-Error' | ||
|
|
||
| TransportResponseHook = Callable[[requests.Response], object] | ||
| """Callable invoked for every Logfire API response received by the SDK. | ||
|
|
||
| The return value is ignored; raise to abort the call. | ||
| """ | ||
|
|
||
|
|
||
| def process_logfire_response_headers(response: requests.Response) -> None: | ||
| """Default transport response hook: surface `X-Logfire-Warning` / `X-Logfire-Error` headers.""" | ||
| warning_message = response.headers.get(WARNING_HEADER_NAME) | ||
| if warning_message: | ||
| warnings.warn(warning_message, LogfireServerWarning, stacklevel=2) | ||
| error_message = response.headers.get(ERROR_HEADER_NAME) | ||
| if error_message: | ||
| raise LogfireServerError(error_message) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worry that users might have a reason to opt out of this and won't be able to. Maybe that's paranoid. We could theoretically make this hook configurable, defaulting to this one, but allowing users to take any action for all API responses. Probably overkill. I'd like to know what concrete use cases we have in mind for both warnings and errors.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a way to opt out in 278a76c
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From the PR description:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'd deprecate endpoints by changing code here though, right? or is the point to push users to upgrade their sdk? hard failures like what? where would the server respond with the error header instead of a 4xx/5xx which would have a similar effect here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes the point is that we can propagate deprecation to old versions of the SDK.
The error part was a bit more just "seems like it could be useful". But I imagine it might be more informative to raise a I also wonder if we could emit an error / warn logfire log so it shows up in users projects 🤔 |
||
|
|
||
|
|
||
| def install_logfire_response_hook( | ||
| session: requests.Session, | ||
| hook: TransportResponseHook | None = None, | ||
| ) -> None: | ||
| """Install a `requests` response hook on `session` for every Logfire API response. | ||
|
|
||
| `hook` defaults to `process_logfire_response_headers`. Pass a custom callable | ||
| to replace the default behaviour (e.g. opt out by passing `lambda response: None`). | ||
| """ | ||
| user_hook = hook if hook is not None else process_logfire_response_headers | ||
|
|
||
| def _hook(response: requests.Response, *_args: Any, **_kwargs: Any) -> requests.Response: | ||
| user_hook(response) | ||
| return response | ||
|
|
||
| response_hooks: list[Any] = session.hooks.setdefault('response', []) | ||
| response_hooks.append(_hook) | ||
Uh oh!
There was an error while loading. Please reload this page.