dd-trace-py
dd-trace-py copied to clipboard
fix: Remove override of enter method that was preventing the context …
Related to https://github.com/DataDog/dd-trace-py/issues/3303
While investigating why ddtrace opens too much connections. I've found this piece of code related to the TracedCursor
that changes the behaviour of the wrapped object since it does not call the parent enter method.
Checklist
- [ ] Library documentation is updated.
- [ ] Corp site documentation is updated (link to the PR).
Reviewer Checklist
- [ X] Title is accurate.
- [ X] Description motivates each change.
- [ X] No unnecessary changes were introduced in this PR.
- [ X] PR cannot be broken up into smaller PRs.
- [X ] Avoid breaking API changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is included in the code or PR.
- [ ] Release note has been added for fixes and features, or else
changelog/no-changelog
label added. - [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
- [ ] Add to milestone.
cc @brettlangdon @Giaco9
That override was introduced to mitigate the fact that psycopg < 2.5 were not supporting context managers. From https://ddtrace.readthedocs.io/en/stable/ looks like that has been already deprecated, so this change should be safe
@ca-simone-chiorazzo awesome find, thanks! We'll look into this!
hmm, this seemed to break all of our dbapi related test suites, but not getting the error I was expecting. It is saying we no longer are producing traces. we'll have to dig into it a bit further
@brettlangdon Yes looks like something is changed with the change that I'm proposing. Is not really clear to me if the tests are based on snapshots that have been registered on a real installation of ddtrace.
Please let me know if I can help with this in some way.
@brettlangdon do you have any news on this? Should we close this considering that #3303 has been closed? In my option the contribution is still valid, it should help remove same old magic about psycopg.
@Giaco9 I am still not sure, given it's impact on the other dbapi compatible libraries (they are basically all failing), I am wondering if this would be a breaking change with our library or if there is a specific library we would break compatibility for. (I have not dug any deeper to investigate)
@ca-simone-chiorazzo this pull request is now in conflict 😩
I close this PR since there are unexpected side effects and the issue that we're facing was not related to this change.