-
Notifications
You must be signed in to change notification settings - Fork 2
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
Full refactoring. #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design is much better now 🔥
I don’t have critical objections, but may add more suggestions once the whole thing gets done.
Perhaps, it's a good idea to change abstractions a bit to something like this: from contextlib import ExitStack, contextmanager
from functools import partial
from typing import (
TYPE_CHECKING,
Any,
Callable,
ContextManager,
Generator,
Protocol,
TypedDict,
TypeVar,
)
if TYPE_CHECKING:
def merge_dict_configs(a: Any, b: Any) -> Any: ...
def merge_pydantic_configs(a: Any, b: Any) -> Any: ...
class Litestar: ...
class OpenTelemetryInstrumentationMiddleware: ...
class LitestarOpentelemetryConfig: ...
InstrumentConfig = TypeVar("InstrumentConfig")
InstrumentResult = TypeVar("InstrumentResult")
class Instrument(ContextManager, Protocol[InstrumentConfig, InstrumentResult]):
def __enter__(self, config: InstrumentConfig) -> InstrumentResult: ...
class OpenTelemetryConfig: ...
class OpenTelemetryInstrumentResult(TypedDict):
tracer_provider: Any
exclude_urls: list[str]
@contextmanager
def instrument_opentelemetry(
config: OpenTelemetryConfig,
) -> Generator[OpenTelemetryInstrumentResult, None, None]:
yield {"tracer_provider": ..., "exclude_urls": []}
ApplicationT = TypeVar("ApplicationT")
ApplicationConfigT = TypeVar("ApplicationConfigT")
class Configurator(Protocol[ApplicationT, ApplicationConfigT]):
def make_application_from_config(
self, config: ApplicationConfigT
) -> ApplicationT: ...
def configure_opentelemetry(
self, result: OpenTelemetryInstrumentResult
) -> None: ...
def configure_teardown(self, teardown: Callable[[], None]) -> None: ...
class Settings: ...
def bootstrap(
configurator: Configurator[ApplicationT, ApplicationConfigT],
settings: Settings,
*,
application_config: ApplicationConfigT | None = None,
opentelemetry_config: OpenTelemetryConfig | None = None,
) -> ApplicationT:
exit_stack = ExitStack()
full_application_config = application_config
for instrument_config, instrument, framework_adapter in [
(
opentelemetry_config,
instrument_opentelemetry,
configurator.configure_opentelemetry,
)
]:
config_piece = framework_adapter(
instrument(merge_pydantic_configs(settings, instrument_config))
)
full_application_config = merge_dict_configs(
full_application_config, exit_stack.enter_context(config=config_piece)
)
full_application_config = merge_dict_configs(
full_application_config, configurator.configure_teardown(exit_stack.close)
)
return configurator.make_application_from_config(full_application_config)
class LitestarConfigurator(Configurator[Litestar, dict[str, Any]]):
def make_application_from_config(self, config: dict[str, Any]) -> Any:
return Litestar(**config)
def configure_opentelemetry(self, result: OpenTelemetryInstrumentResult) -> None:
return {
"middleware": OpenTelemetryInstrumentationMiddleware(
LitestarOpentelemetryConfig(
tracer_provider=result["tracer_provider"],
exclude=result["exclude_urls"],
),
),
}
def configure_teardown(self, teardown: Callable[[], None]) -> None:
return {"on_shutdown": [teardown]}
bootstrap_litestar = partial(bootstrap, configurator=LitestarConfigurator())
def main() -> None:
application = bootstrap_litestar(Settings())
|
Tests are working! But there is much to be done still |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
First at all, thank you for your work. Package are really getting better. |
However, I have some suggestions and questions, and I am hopeful that they will only make better the package.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for now I think, thanks!
LGTM! |
Tests are broken yet.