dd-trace-py icon indicating copy to clipboard operation
dd-trace-py copied to clipboard

fix: Remove override of enter method that was preventing the context …

Open ca-simone-chiorazzo opened this issue 2 years ago • 6 comments

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

ca-simone-chiorazzo avatar Jun 03 '22 15:06 ca-simone-chiorazzo

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 avatar Jun 03 '22 15:06 ca-simone-chiorazzo

@ca-simone-chiorazzo awesome find, thanks! We'll look into this!

brettlangdon avatar Jun 03 '22 15:06 brettlangdon

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 avatar Jun 03 '22 16:06 brettlangdon

@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.

ca-simone-chiorazzo avatar Jun 07 '22 08:06 ca-simone-chiorazzo

@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 avatar Jul 13 '22 09:07 Giaco9

@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)

brettlangdon avatar Jul 13 '22 11:07 brettlangdon

@ca-simone-chiorazzo this pull request is now in conflict 😩

mergify[bot] avatar Aug 23 '22 15:08 mergify[bot]

I close this PR since there are unexpected side effects and the issue that we're facing was not related to this change.

ca-simone-chiorazzo avatar Sep 05 '22 08:09 ca-simone-chiorazzo