diff --git a/AGENTS.md b/AGENTS.md index 8423534..0baac8f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,10 +56,18 @@ Four rules that are not visible in the code that follows them: re-export both from `__init__.py` if the old name was exported. It is a class assignment, not a subclass, so `isinstance` still holds. `FreeBootstrapperConfig`, `OpentelemetryConfig` and `IGNORED_STRUCTLOG_ATTRIBUTES` exist for this reason. -- **A warning raised while a config is being built goes through `warn_at_caller`.** No literal - `stacklevel=` reaches the user from a `__post_init__`: the depth is that config's MRO chain plus - the `__init__` `dataclasses` generates. Warnings raised outside config construction keep their - literal `stacklevel`: their target is the bootstrapper's caller, not the user. +- **A warning raised while a config is built or an instrument is bootstrapped goes through + `warn_at_caller`.** No literal `stacklevel=` reaches the user on either path: from a + `__post_init__` the depth is that config's MRO chain plus the `__init__` `dataclasses` generates, + and from an instrument's `bootstrap()` it is one frame deeper whenever that instrument calls + `super().bootstrap()` first. `warn_at_caller` walks out to the first frame outside + `lite_bootstrap` instead, which on the bootstrap path is the user's `bootstrap()` call. Three + literal `stacklevel=` sites survive, all outside those two paths and all deliberate: the + dep-missing warning in `BaseBootstrapper._select_instruments` (`stacklevel=4`) and the + double-attach warning in `_attach_teardown_once` (`stacklevel=3`, one frame shallower because + the subclass `__init__` calls it directly rather than through `BaseBootstrapper.__init__`), both + a fixed depth below the user and scoped out by #202; and `helpers/fastapi_helpers.py`, which + warns while serving a request, with no user frame anywhere on the stack. - **Sentinels on a user's app get a `_lite_bootstrap_` prefix.** Set a direct attribute on the app object; never squat in a framework namespace like Starlette's `application.state`. Read it with `getattr(target, name, default)` (no SLF violation); write it with `# noqa: SLF001`. diff --git a/lite_bootstrap/bootstrappers/fastapi_bootstrapper.py b/lite_bootstrap/bootstrappers/fastapi_bootstrapper.py index bcd7ac9..78c6ca3 100644 --- a/lite_bootstrap/bootstrappers/fastapi_bootstrapper.py +++ b/lite_bootstrap/bootstrappers/fastapi_bootstrapper.py @@ -1,7 +1,6 @@ import contextlib import dataclasses import typing -import warnings from lite_bootstrap import import_checker from lite_bootstrap.bootstrappers.base import BaseBootstrapper @@ -158,10 +157,7 @@ def bootstrap(self) -> None: config = self.bootstrap_config application = config.app if config.swagger_path != application.docs_url: - warnings.warn( - f"swagger_path differs from docs_url, {application.docs_url} will be used for docs path", - stacklevel=2, - ) + warn_at_caller(f"swagger_path differs from docs_url, {application.docs_url} will be used for docs path") if config.swagger_offline_docs: enable_offline_docs(application, static_path=config.swagger_static_path) diff --git a/lite_bootstrap/helpers/warn.py b/lite_bootstrap/helpers/warn.py index 211b76c..4b15394 100644 --- a/lite_bootstrap/helpers/warn.py +++ b/lite_bootstrap/helpers/warn.py @@ -5,11 +5,11 @@ _DATACLASS_GENERATED_INIT: typing.Final = "" -_CONSTRUCTION_MODULES: typing.Final = frozenset({__name__.split(".")[0], "dataclasses"}) +_INTERNAL_MODULES: typing.Final = frozenset({__name__.split(".")[0], "dataclasses"}) def _is_internal_frame(frame: types.FrameType) -> bool: - """Return True while the frame is still part of building a config rather than asking for one. + """Return True while the frame still belongs to lite-bootstrap rather than to its user. `dataclasses` is in the set because `replace()` calls the constructor itself. The `__init__` it generates reports its file as ``, in the defining class's module rather than this one. @@ -17,18 +17,20 @@ def _is_internal_frame(frame: types.FrameType) -> bool: if frame.f_code.co_name == "__init__" and frame.f_code.co_filename == _DATACLASS_GENERATED_INIT: return True module_name: str = frame.f_globals.get("__name__", "") - return module_name.split(".", maxsplit=1)[0] in _CONSTRUCTION_MODULES + return module_name.split(".", maxsplit=1)[0] in _INTERNAL_MODULES -def warn_at_caller(message: str) -> None: - """Warn at the nearest frame outside the machinery that builds a config. +def warn_at_caller(message: str, category: type[Warning] = UserWarning) -> None: + """Warn at the nearest frame outside lite-bootstrap's own machinery. - Config validation runs inside a `__post_init__` cascade whose depth is the length of that - config's MRO chain, so no literal `stacklevel` can name the frame that built the config. + Both call paths that reach here have a depth no literal `stacklevel` can name: a config's + `__post_init__` cascade is as deep as that config's MRO chain plus the `__init__` + `dataclasses` generates, and an instrument's `bootstrap()` sits one frame deeper when it + calls `super().bootstrap()` first. """ stacklevel = 1 frame: types.FrameType | None = inspect.currentframe() while frame is not None and _is_internal_frame(frame): stacklevel += 1 frame = frame.f_back - warnings.warn(message, stacklevel=stacklevel) + warnings.warn(message, category=category, stacklevel=stacklevel) diff --git a/lite_bootstrap/instruments/opentelemetry_instrument.py b/lite_bootstrap/instruments/opentelemetry_instrument.py index 1e83f8a..d30d03f 100644 --- a/lite_bootstrap/instruments/opentelemetry_instrument.py +++ b/lite_bootstrap/instruments/opentelemetry_instrument.py @@ -3,7 +3,6 @@ import os import typing import urllib.parse -import warnings from lite_bootstrap import import_checker from lite_bootstrap.exceptions import InstrumentDependencyMissingWarning, collect_teardown_errors @@ -195,14 +194,12 @@ def _build_span_exporter(self) -> "SpanExporter | None": Only call this once opentelemetry_endpoint is set: both warnings claim that it is. """ config = self.bootstrap_config - # stacklevel counts this frame as well as bootstrap()'s, to land on bootstrap()'s caller. if config.opentelemetry_exporter_protocol == "grpc": if not import_checker.is_otlp_grpc_exporter_installed: - warnings.warn( + warn_at_caller( "opentelemetry_endpoint is set but the gRPC OTLP exporter is not installed; " "spans will not be exported. Install lite-bootstrap[otl].", category=InstrumentDependencyMissingWarning, - stacklevel=3, ) return None return OTLPGrpcSpanExporter( @@ -210,11 +207,10 @@ def _build_span_exporter(self) -> "SpanExporter | None": insecure=config.opentelemetry_insecure, ) if not import_checker.is_otlp_http_exporter_installed: - warnings.warn( + warn_at_caller( "opentelemetry_endpoint is set but the HTTP OTLP exporter is not installed; " "spans will not be exported. Install lite-bootstrap[otl-http].", category=InstrumentDependencyMissingWarning, - stacklevel=3, ) return None return OTLPHttpSpanExporter(endpoint=config.opentelemetry_endpoint) diff --git a/tests/conftest.py b/tests/conftest.py index 26c039a..d798f14 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -60,6 +60,15 @@ def logging_mock() -> LoggingMock: return LoggingMock() +def warning_source_files(caught: typing.Iterable[warnings.WarningMessage], category: type[Warning]) -> list[str]: + """Return the file each recorded warning of ``category`` was attributed to, in the order raised. + + Attribution is what the stacklevel rules decide, so a test that pins it compares whole lists: + a warning raised from the wrong frame and one raised twice are different bugs. + """ + return [one_warning.filename for one_warning in caught if issubclass(one_warning.category, category)] + + @contextlib.contextmanager def emulate_package_missing(package_name: str) -> typing.Iterator[None]: old_module = sys.modules[package_name] diff --git a/tests/instruments/test_opentelemetry_instrument.py b/tests/instruments/test_opentelemetry_instrument.py index d164620..0ab0c78 100644 --- a/tests/instruments/test_opentelemetry_instrument.py +++ b/tests/instruments/test_opentelemetry_instrument.py @@ -15,7 +15,7 @@ OpenTelemetryConfig, OpenTelemetryInstrument, ) -from tests.conftest import CustomInstrumentor, emulate_package_missing_with_module_reload +from tests.conftest import CustomInstrumentor, emulate_package_missing_with_module_reload, warning_source_files def test_opentelemetry_instrument() -> None: @@ -306,10 +306,12 @@ def test_missing_exporter_warning_points_at_the_caller_of_bootstrap( ) -> None: """INVARIANT: the missing-exporter warning is attributed to the frame that called bootstrap(). - The stacklevel is a literal, so it counts frames that only exist by convention: any helper - extracted out of bootstrap() moves the warning one frame deeper, onto lite_bootstrap's own - source, and nothing but this test notices. Both transports are pinned because each warns - from its own branch and a refactor can reshape one without the other. + Replacing warn_at_caller with a literal stacklevel breaks it: the literal counts frames that + only exist by convention, so any helper extracted out of bootstrap() moves the warning one + frame deeper, onto lite_bootstrap's own source, and nothing but this test notices. Both + transports are pinned because each warns from its own branch and a refactor can reshape one + without the other. This covers the instrument called on its own; the bootstrapper path, where + the frame to skip past is lite_bootstrap's own, is pinned in tests/test_fastapi_bootstrap.py. """ instrument = OpenTelemetryInstrument( bootstrap_config=OpenTelemetryConfig(opentelemetry_endpoint=endpoint, opentelemetry_exporter_protocol=protocol) @@ -324,5 +326,4 @@ def test_missing_exporter_warning_points_at_the_caller_of_bootstrap( finally: instrument.teardown() - matching = [w for w in caught if issubclass(w.category, InstrumentDependencyMissingWarning)] - assert [w.filename for w in matching] == [__file__] + assert warning_source_files(caught, InstrumentDependencyMissingWarning) == [__file__] diff --git a/tests/test_fastapi_bootstrap.py b/tests/test_fastapi_bootstrap.py index c3a0d8a..7dd78ed 100644 --- a/tests/test_fastapi_bootstrap.py +++ b/tests/test_fastapi_bootstrap.py @@ -1,6 +1,7 @@ import dataclasses import logging import warnings +from unittest.mock import patch import fastapi import pytest @@ -8,10 +9,10 @@ from starlette import status from starlette.testclient import TestClient -from lite_bootstrap import FastAPIBootstrapper, FastAPIConfig -from lite_bootstrap.exceptions import ConfigurationError +from lite_bootstrap import FastAPIBootstrapper, FastAPIConfig, import_checker +from lite_bootstrap.exceptions import ConfigurationError, InstrumentDependencyMissingWarning from lite_bootstrap.types import UNSET -from tests.conftest import CustomInstrumentor, SentryTestTransport, emulate_package_missing +from tests.conftest import CustomInstrumentor, SentryTestTransport, emulate_package_missing, warning_source_files logger = structlog.getLogger(__name__) @@ -189,3 +190,45 @@ def test_second_fastapi_bootstrapper_bootstrap_raises(fastapi_config: FastAPICon ) finally: first.teardown() + + +def test_swagger_warning_points_at_the_bootstrap_call_site(fastapi_config: FastAPIConfig) -> None: + """INVARIANT: a warning raised while an instrument bootstraps names the user's bootstrap() line. + + FastAPISwaggerInstrument.bootstrap() is called straight from the loop in + BaseBootstrapper.bootstrap(), one frame shallower than an instrument that calls + super().bootstrap() first. A literal stacklevel pins one of those two depths and misses the + other, naming lite_bootstrap's own source instead. This test and the OpenTelemetry one below + are a pair: each covers one depth, and either passing alone proves nothing. + """ + new_config = dataclasses.replace(fastapi_config, application=fastapi.FastAPI(docs_url="/custom-docs/")) + bootstrapper = FastAPIBootstrapper(bootstrap_config=new_config) + try: + with pytest.warns(UserWarning, match="swagger_path differs from docs_url") as caught: + bootstrapper.bootstrap() + finally: + bootstrapper.teardown() + + assert warning_source_files(caught, UserWarning) == [__file__] + + +def test_missing_exporter_warning_points_at_the_bootstrap_call_site(fastapi_config: FastAPIConfig) -> None: + """INVARIANT: the deeper super().bootstrap() shape names the user's bootstrap() line as well. + + FastAPIOpenTelemetryInstrument.bootstrap() calls super().bootstrap() before the warning is + raised, so the user's frame sits one deeper than for the swagger instrument above. Any + instrument that grows or loses a super() call shifts that depth again; only a rule that finds + the first frame outside lite_bootstrap survives it. + """ + new_config = dataclasses.replace(fastapi_config, opentelemetry_endpoint="localhost:4317") + bootstrapper = FastAPIBootstrapper(bootstrap_config=new_config) + try: + with ( + patch.object(import_checker, "is_otlp_grpc_exporter_installed", False), + pytest.warns(InstrumentDependencyMissingWarning) as caught, + ): + bootstrapper.bootstrap() + finally: + bootstrapper.teardown() + + assert warning_source_files(caught, InstrumentDependencyMissingWarning) == [__file__]