Call missing parent init #1183

ocelotl · 2022-07-06T11:03:19Z

ocelotl · 2022-07-06T11:04:03Z

@phillipuniverse please give it a try with these changes. Since it is hard for me to reproduce I am kind of guessing what the problem and the solution may be.

phillipuniverse

@ocelotl I think we're on the right track. I pulled in your changes and now I get this:

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/_pytest/main.py", line 268, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/_pytest/main.py", line 322, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pluggy/_hooks.py", line 265, in __call__
INTERNALERROR>     return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pluggy/_manager.py", line 80, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pluggy/_callers.py", line 55, in _multicall
INTERNALERROR>     gen.send(outcome)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pytest_cov/plugin.py", line 294, in pytest_runtestloop
INTERNALERROR>     self.cov_controller.finish()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pytest_cov/engine.py", line 44, in ensure_topdir_wrapper
INTERNALERROR>     return meth(self, *args, **kwargs)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pytest_cov/engine.py", line 230, in finish
INTERNALERROR>     self.cov.stop()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/control.py", line 706, in save
INTERNALERROR>     data = self.get_data()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/control.py", line 775, in get_data
INTERNALERROR>     if self._collector and self._collector.flush_data():
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/collector.py", line 471, in flush_data
INTERNALERROR>     self.covdata.add_arcs(self.mapped_file_dict(data))
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 238, in _wrapped
INTERNALERROR>     return method(self, *args, **kwargs)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 499, in add_arcs
INTERNALERROR>     self._choose_lines_or_arcs(arcs=True)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 528, in _choose_lines_or_arcs
INTERNALERROR>     with self._connect() as con:
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 315, in _connect
INTERNALERROR>     self._open_db()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 266, in _open_db
INTERNALERROR>     self._read_db()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 270, in _read_db
INTERNALERROR>     with self._dbs[threading.get_ident()] as db:
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 1084, in __enter__
INTERNALERROR>     self._connect()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 1063, in _connect
INTERNALERROR>     self.con = sqlite3.connect(self.filename, check_same_thread=False)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 144, in wrap_connect_
INTERNALERROR>     return db_integration.wrapped_connection(wrapped, args, kwargs)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 267, in wrapped_connection
INTERNALERROR>     return get_traced_connection_proxy(connection, self)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 332, in get_traced_connection_proxy
INTERNALERROR>     return TracedConnectionProxy(connection)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 313, in __init__
INTERNALERROR>     super().__init__()
INTERNALERROR> TypeError: function missing required argument 'database' (pos 1)

Maybe we need to get at *args and *kwargs and pass those up? This stack trace is pretty informative with the sqlite3.connect

ocelotl · 2022-07-06T16:28:32Z

Ready, @phillipuniverse, please give it a try.

phillipuniverse · 2022-07-06T16:50:28Z

@ocelotl same exception but I'm not 100% sure if I've got the right reproduction case going on. Let me take a closer look here later.

But my suspicion is that probably need to propagate the *args and **kwargs when you construct the TracedConnectionProxy at https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1183/files#diff-554be8b86b646bc469f7e52d34360fb36743e0a584bd4322ad805d3df22d0001R332

ocelotl · 2022-07-06T16:55:13Z

@ocelotl same exception but I'm not 100% sure if I've got the right reproduction case going on. Let me take a closer look here later.

But my suspicion is that probably need to propagate the *args and **kwargs when you construct the TracedConnectionProxy at https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1183/files#diff-554be8b86b646bc469f7e52d34360fb36743e0a584bd4322ad805d3df22d0001R332

Done ✌️

phillipuniverse · 2022-07-06T19:33:33Z

Hm same exception, idk what's up here.

INTERNALERROR>     data = self.get_data()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/control.py", line 775, in get_data
INTERNALERROR>     if self._collector and self._collector.flush_data():
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/collector.py", line 471, in flush_data
INTERNALERROR>     self.covdata.add_arcs(self.mapped_file_dict(data))
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 238, in _wrapped
INTERNALERROR>     return method(self, *args, **kwargs)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 499, in add_arcs
INTERNALERROR>     self._choose_lines_or_arcs(arcs=True)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 528, in _choose_lines_or_arcs
INTERNALERROR>     with self._connect() as con:
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 315, in _connect
INTERNALERROR>     self._open_db()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 266, in _open_db
INTERNALERROR>     self._read_db()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 270, in _read_db
INTERNALERROR>     with self._dbs[threading.get_ident()] as db:
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 1084, in __enter__
INTERNALERROR>     self._connect()
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/sqldata.py", line 1063, in _connect
INTERNALERROR>     self.con = sqlite3.connect(self.filename, check_same_thread=False)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 144, in wrap_connect_
INTERNALERROR>     return db_integration.wrapped_connection(wrapped, args, kwargs)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 267, in wrapped_connection
INTERNALERROR>     return get_traced_connection_proxy(connection, self)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 332, in get_traced_connection_proxy
INTERNALERROR>     return TracedConnectionProxy(connection, *args, **kwargs)
INTERNALERROR>   File "/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/opentelemetry/instrumentation/dbapi/__init__.py", line 313, in __init__
INTERNALERROR>     super().__init__(*args, **kwargs)
INTERNALERROR> TypeError: function missing required argument 'database' (pos 1)

We should have a much smaller reproduction case though of connecting to a file-based sqlite db. Running with the coverage module isn't particularly special, it's just creating a new SQLite db based on a file.

phillipuniverse · 2022-07-06T23:36:07Z

It's really unclear why the initial issue in #1179 was not exposed by the tests. It seems like it's doing the same sort of connect() there.

But with the latest changes the tests do fail so at least there's some way to iterate. But I'm really not sure what the difference here is between what coverage is doing vs what vanilla SQLlite is doing.

ocelotl · 2022-07-07T11:07:37Z

It's really unclear why the initial issue in #1179 was not exposed by the tests. It seems like it's doing the same sort of connect() there.

But with the latest changes the tests do fail so at least there's some way to iterate. But I'm really not sure what the difference here is between what coverage is doing vs what vanilla SQLlite is doing.

I have pushed another commit that may make things work on your environment but I don't think we actually want these changes. The idea of the TracedConnectionProxy is to act as an object that proxies all attribute accesses of another, already created connection object, but we don't want to again call the connection __init__ method. With the last changes I noticed that a database file was created at instrumentation/opentelemetry-instrumentation-sqlite3/tests/database. Seems like calling __init__ again instantiates a new database which is something we certainly do not want to do.

phillipuniverse · 2022-07-07T14:39:48Z

may make things work on your environment

Not quite, presumably because this is effectively overriding the given coverage file with a hardcoded "database":

  self._warn("No data was collected.", slug="no-data-collected")
/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/coverage/data.py:129: CoverageWarning: Data file '/Users/phillip/shipwell/common_python/.coverage.Phillips-MacBook-Pro.local.51032.604604' doesn't seem to be a coverage data file: cannot unpack non-iterable NoneType object
  data._warn(str(exc))
/Users/phillip/Library/Caches/pypoetry/virtualenvs/shipwell-common-python-zjZyLK7l-py3.7/lib/python3.7/site-packages/pytest_cov/plugin.py:294: CovReportWarning: Failed to generate report: No data to report.

  self.cov_controller.finish()

The idea of the TracedConnectionProxy is to act as an object that proxies all attribute accesses of another, already created connection object, but we don't want to again call the connection init method

This makes sense. As an experiment, I tried to isolate the changes and move back to the wrapt.ObjectProxy version for TracedConnectionProxy. So I modified get_traced_connection_proxy to be the old version:

def get_traced_connection_proxy(
    connection, db_api_integration, *args, **kwargs
):
    class TracedConnectionProxy(wrapt.ObjectProxy):
        # pylint: disable=unused-argument
        def __init__(self, connection, *args, **kwargs):
            wrapt.ObjectProxy.__init__(self, connection)

        def cursor(self, *args, **kwargs):
            return get_traced_cursor_proxy(
                self.__wrapped__.cursor(*args, **kwargs), db_api_integration
            )

        def __enter__(self):
            self.__wrapped__.__enter__()
            return self

        def __exit__(self, *args, **kwargs):
            self.__wrapped__.__exit__(*args, **kwargs)

    return TracedConnectionProxy(connection, *args, **kwargs)

And then in uninstrument_connection added:

    if isinstance(connection, wrapt.ObjectProxy):
        return connection.__wrapped__

And this made the coverage run succeed, no exceptions or issues!

So I suspect that the real issue here is that wrapt.ObjectProxy was forwarding additional calls to the underlying connection that are now not being forwarded from the manual proxy.

ocelotl · 2022-11-21T14:16:07Z

@phillipuniverse Can you give it a try with the latest commit? I noticed that this latest commit seems to fix #1319, it may work here as well.

alisonatwork · 2022-11-22T05:22:05Z

I have run this test using the current commit as of 15 hours ago, and this is still broken for #1319, because args and kwargs are not set inside get_traced_connection_proxy.

What needs to happen is for the args and kwargs to be passed all of the way done from the top level. That is, wrapped_connection inside DatabaseApiIntegration should pass args and kwargs down to get_traced_connection_proxy and then into the TracedConnectionProxy constructor and its parent constructor.

I think for instrument_connection it should be okay to keep its get_traced_connection_proxy call as-is since it doesn't have any args or kwargs to relay.

Fixes open-telemetry#1179

ocelotl · 2022-11-28T20:25:59Z

@alisonatwork added your suggested changes, please give it a try ✌️

alisonatwork · 2022-11-30T06:40:33Z

@ocelotl I dropped in the __init__.py only from the latest commit and commented out the sql comment refactoring churn that happened in #1224 (I still need to use 0.33b0 for Python 3.6 support) and the connection worked as expected.

ocelotl · 2022-12-07T19:05:32Z

d in the __init__.py only from

Hm, sorry, I am a bit confused, can you paste a diff of your changes here against the latest commit on this PR, please?

alisonatwork · 2022-12-07T23:00:02Z

I can't run the current release of open telemetry in our project because Python 3.6 is no longer supported, so any fix applied upstream will not work for us until we upgrade Python. As such, I had to manually apply a partial diff of your changes to the 0.33b0 version installed in my venv to test it. The changes I had to make to get it running in my venv are unrelated to this PR and not important to fix the regression in the current version. Ideally this bug should've been fixed immediately after it was introduced in 0.32b0, before support for Python 3.6 was dropped in 0.34b0, but we'll find a way to work around it for our use case.

phillipuniverse · 2022-12-08T15:23:45Z

@ocelotl sorry I was out of pocket for a bit, I'll working on re-validating today or tomorrow.

ocelotl · 2022-12-08T17:58:59Z

Test case that reproduces this issue by @rogersd.

OrenZhang · 2022-12-14T10:45:41Z

@ocelotl hello, i wonder whether this issue would be fixed above 0.36b0, so i could upgrade to the latest version of opentelemetry sdk to test other features like _log

ps:
dbapi sdk doesn't work well since #1097

alisonatwork · 2023-01-09T02:17:38Z

Please can someone prioritize addressing this issue, #1319 and #1179? The dbapi plugin has been broken for over 6 months now across multiple releases. It is hard to get buy-in for moving from proprietary tracing solutions to OpenTelemetry when such a fundamental feature is broken. The change in #1097 removed the functionality where custom args could be passed into the constructor of the underlying connection, so the fix is clear: all that needs to happen is to explicitly pass those args.

phillipuniverse · 2023-01-16T03:53:44Z

@ocelotl FWIW I pulled down your branch and ran my tests with the coverage module and have confirmed that the latest changes fixes the issue.

phillipuniverse · 2023-02-03T17:48:01Z

@ocelotl @srikanthccv any way we can prioritize getting this in for the release next week?

philipcwhite · 2023-02-09T20:21:55Z

If this fixes #1179 then I'd like to see it added as well. Thanks.

philipcwhite · 2023-02-20T18:31:33Z

Any chance this will get fixed in the near future? I see it's not in the 0.37b0 release notes. Thanks.

phillipuniverse · 2023-02-20T18:39:29Z

@philipcwhite this particular PR is not included in 0.37b0, but the revert of the change that caused the original problem is.

I confirmed that #1179 is resolved.

philipcwhite · 2023-02-20T19:08:24Z

@phillipuniverse I tested 0.37b0 and it does indeed fix the issue. Thanks so much!

ocelotl · 2024-09-03T19:27:39Z

I won't be working on this PR for the foreseeable future, closing.

phillipuniverse suggested changes Jul 6, 2022

View reviewed changes

phillipuniverse mentioned this pull request Nov 1, 2022

Instrumentation issues with coverage module and SQLite in latest 0.32b0 release #1179

Closed

ocelotl force-pushed the issue_1179 branch from daa007a to 60f4dfe Compare November 21, 2022 11:14

ocelotl self-assigned this Nov 21, 2022

ocelotl mentioned this pull request Nov 21, 2022

dbapi instrumentation does not work for MySQLdb/mysqlclient #1319

Open

ocelotl added 6 commits November 28, 2022 21:14

Call missing parent init

ef32342

Fixes open-telemetry#1179

Add args and kwargs

4d2ea16

Pass missing args and kwargs

41f74a0

Another attempt

596f25a

Remove fixed database string

ffdb906

Pass *args and **kwargs

53529db

ocelotl force-pushed the issue_1179 branch from 4de0b41 to 53529db Compare November 28, 2022 20:19

Add changelog

81fa5df

ocelotl added 2 commits November 28, 2022 22:50

WIP

830c5f9

WIP

6496ab2

Merge branch 'main' into issue_1179

7dbbee8

ocelotl closed this Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call missing parent init #1183

Call missing parent init #1183

ocelotl commented Jul 6, 2022

ocelotl commented Jul 6, 2022

phillipuniverse left a comment

ocelotl commented Jul 6, 2022

phillipuniverse commented Jul 6, 2022 •

edited

Loading

ocelotl commented Jul 6, 2022

phillipuniverse commented Jul 6, 2022

phillipuniverse commented Jul 6, 2022

ocelotl commented Jul 7, 2022

phillipuniverse commented Jul 7, 2022 •

edited

Loading

ocelotl commented Nov 21, 2022

alisonatwork commented Nov 22, 2022

ocelotl commented Nov 28, 2022

alisonatwork commented Nov 30, 2022

ocelotl commented Dec 7, 2022

alisonatwork commented Dec 7, 2022

phillipuniverse commented Dec 8, 2022

ocelotl commented Dec 8, 2022

OrenZhang commented Dec 14, 2022

alisonatwork commented Jan 9, 2023

phillipuniverse commented Jan 16, 2023

phillipuniverse commented Feb 3, 2023

philipcwhite commented Feb 9, 2023

philipcwhite commented Feb 20, 2023

phillipuniverse commented Feb 20, 2023

philipcwhite commented Feb 20, 2023

ocelotl commented Sep 3, 2024

Call missing parent init #1183

Call missing parent init #1183

Conversation

ocelotl commented Jul 6, 2022

ocelotl commented Jul 6, 2022

phillipuniverse left a comment

Choose a reason for hiding this comment

ocelotl commented Jul 6, 2022

phillipuniverse commented Jul 6, 2022 • edited Loading

ocelotl commented Jul 6, 2022

phillipuniverse commented Jul 6, 2022

phillipuniverse commented Jul 6, 2022

ocelotl commented Jul 7, 2022

phillipuniverse commented Jul 7, 2022 • edited Loading

ocelotl commented Nov 21, 2022

alisonatwork commented Nov 22, 2022

ocelotl commented Nov 28, 2022

alisonatwork commented Nov 30, 2022

ocelotl commented Dec 7, 2022

alisonatwork commented Dec 7, 2022

phillipuniverse commented Dec 8, 2022

ocelotl commented Dec 8, 2022

OrenZhang commented Dec 14, 2022

alisonatwork commented Jan 9, 2023

phillipuniverse commented Jan 16, 2023

phillipuniverse commented Feb 3, 2023

philipcwhite commented Feb 9, 2023

philipcwhite commented Feb 20, 2023

phillipuniverse commented Feb 20, 2023

philipcwhite commented Feb 20, 2023

ocelotl commented Sep 3, 2024

phillipuniverse commented Jul 6, 2022 •

edited

Loading

phillipuniverse commented Jul 7, 2022 •

edited

Loading