Support Instrumenting Async SqlAlchemy Engines #1425
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for instrumenting async SqlAlchemy engines by handling the engines parameter and automatically converting async engines to their sync equivalents for proper instrumentation.
- Adds validation and conversion logic for the
enginesparameter in the instrumentation function - Updates test coverage to verify both single
engineand multipleenginesparameter usage - Introduces a test case to validate proper error handling for invalid
enginesparameter types
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| logfire/_internal/integrations/sqlalchemy.py | Implements async engine conversion logic and engines parameter validation |
| tests/otel_integrations/test_sqlalchemy.py | Adds comprehensive test coverage for engines parameter and parametrized existing tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- move sync conversion to separate function - remove extra parameter passed to async test - remove unneeded test
| engine = _convert_to_sync_engine(engine) | ||
|
|
||
| if engines is not None: | ||
| engines = [_convert_to_sync_engine(engine_entry) for engine_entry in engines] # type: ignore |
There was a problem hiding this comment.
i'm surprised there's a type error here
There was a problem hiding this comment.
Yes, I also thought it would coerce the types internally, as the list[Engine] type is just a subset of Iterable[Engine | AsyncEngine | None]
|
Thanks! |
2 participants
This PR addresses the need to properly instrument async
enginesvalues, as mentioned in #1402's review comment.