Skip to content
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

Instrument SQLAlchemy engine connection phase #1133

Closed
shahargl opened this issue Jun 15, 2022 · 6 comments · Fixed by #1134
Closed

Instrument SQLAlchemy engine connection phase #1133

shahargl opened this issue Jun 15, 2022 · 6 comments · Fixed by #1134

Comments

@shahargl
Copy link
Contributor

** Is your feature request related to a problem? **
The SQLAlchemy instrumentation does not trace the actual connection to the database

Describe the solution you'd like
I want that connect function will also be traced

Describe alternatives you've considered
Which alternative solutions or features have you considered?

Additional context
We are working with SQLAlchemy (snowflake db) and we implemented a solution where we can see the connect span also as attached in the screenshot (the span is database-connect)
image

@shahargl
Copy link
Contributor Author

I've created a PR adding this functionality - #1134
I've tested this only for our infrastructure so I'll be happy if you will help me with testing this (or tell me how to do the tests my self)

@theobouwman
Copy link

Does this suggest that it creates a connection for each query?
Because if thats the case then I probably found out why with multiple request our API responses are around 1second right?

Screenshot 2023-11-08 at 15 47 56
Screenshot 2023-11-08 at 15 48 00

Screenshot 2023-11-08 at 15 46 50

@shahargl
Copy link
Contributor Author

shahargl commented Nov 8, 2023

From your image it looks like there is a connect before SELECT but I don't have the full context to say if that's the reason.

@theobouwman
Copy link

@shahargl so it creates a connection for each select here right? Because then my FastAPI SQLAlchemy implementation is not right. Should just have a share connection pool i guess.

@shahargl
Copy link
Contributor Author

shahargl commented Nov 8, 2023

Do you use session factory?

@theobouwman
Copy link

theobouwman commented Nov 8, 2023

I use this example: https://github.com/ets-labs/python-dependency-injector/blob/master/examples/miniapps/fastapi-sqlalchemy/webapp/database.py

class Database:

    def __init__(self, db_url: str) -> None:
        self._engine = create_engine(
            db_url,
            echo=get_config().QUERY_ECHO,
            json_serializer=_custom_json_serializer,
            pool_pre_ping=True,
            pool_size=get_config().DB_POOL_SIZE,
        )
        self._session_factory = orm.scoped_session(
            orm.sessionmaker(
                autocommit=False,
                autoflush=False,
                bind=self._engine,
            ),
        )

    def create_database(self) -> None:
        Base.metadata.create_all(self._engine)

    @contextmanager
    def session(self) -> Callable[..., AbstractContextManager[Session]]:
        session: Session = self._session_factory()
        try:
            yield session
        except Exception as e:
            # logger.exception("Session rollback because of exception")
            session.rollback()
            raise e
        finally:
            session.close()

and our dependency injector:

from dependency_injector import containers, providers

class Container(containers.DeclarativeContainer):

    wiring_config = containers.WiringConfiguration(packages=[
        "api.routes",
        "tasks.routes",
        "common.observability"
    ])

    config = providers.Configuration()

    db = providers.Singleton(Database, db_url=get_config().DB_URL())

And then our Repository uses:

event_repository = providers.Factory(EventRepository, session_factory=db.provided.session)

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

Successfully merging a pull request may close this issue.

2 participants