Skip to content

Conversation

@brettlangdon
Copy link
Member

In a release 0.17.0 we added instrumentation for dbapi2 fetchone, fetchmany, and fetchall methods.

While we want to provide instrumentation for these methods tracing them by default has caused issues. The reason being that most database libraries will use fetchone() when iterating over a cursor. This will cause your application to create an unnecessarily large number of spans when returning back a large number of rows from the database.

With this change we are disabling this tracing by default but provide a configuration option for enabling:

from ddtrace import config
config.dbapi2.trace_fetch_methods = True

It can also be enabled with the environment variable DD_DBAPI2_TRACE_FETCH_METHODS=true

@brettlangdon brettlangdon added this to the 0.19.0 milestone Dec 20, 2018
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this PR, great work!

One possible future optimisation could be to split configuration per-method, or at least fetchone vs fetch*. With only one config parameter a user having even only one problematic method would have to give up completely on tracing fetch methods.

Again, a possible future optimisation, and only if we find out that users actually require it.

@brettlangdon
Copy link
Member Author

Yeah, I thought about that approach as well, wasn't sure if we wanted the ability to over-customize, and I wanted to avoid instrumenting those functions but bailing based on configuration.

e.g.

def trace_fetchone():
    if not config.dbapi2.trace_fetchone:
        return wrapped(*args, **kwargs)

    # do tracing

Since this approach could still cause performance issues.

@brettlangdon brettlangdon merged commit e7b07dc into 0.19-dev Dec 21, 2018
@brettlangdon brettlangdon deleted the brettlangdon/dbapi.config branch December 21, 2018 19:47
@brettlangdon brettlangdon mentioned this pull request Dec 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants