Merge branch 'open-telemetry:main' into context-key-contrib

Author: euniceek

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.3.0-0.22b0...HEAD)
 
+- `opentelemetry-instrumentation` Fixed cases where trying to use an instrumentation package without the
+  target library was crashing auto instrumentation agent.
+  ([#530](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/530))
+
 ## [0.22b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.3.0-0.22b0) - 2021-06-01
 
 ### Changed

opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py

@@ -1,12 +1,16 @@
+from logging import getLogger
 from typing import Collection, Optional
 
 from pkg_resources import (
     Distribution,
     DistributionNotFound,
+    RequirementParseError,
     VersionConflict,
     get_distribution,
 )
 
+logger = getLogger(__file__)
+
 
 class DependencyConflict:
     required: str = None
@@ -25,22 +29,36 @@ def __str__(self):
 def get_dist_dependency_conflicts(
     dist: Distribution,
 ) -> Optional[DependencyConflict]:
-    deps = [
-        dep
-        for dep in dist.requires(("instruments",))
-        if dep not in dist.requires()
-    ]
-    return get_dependency_conflicts(deps)
+    main_deps = dist.requires()
+    instrumentation_deps = []
+    for dep in dist.requires(("instruments",)):
+        if dep not in main_deps:
+            # we set marker to none so string representation of the dependency looks like
+            #    requests ~= 1.0
+            # instead of
+            #    requests ~= 1.0; extra = "instruments"
+            # which does not work with `get_distribution()`
+            dep.marker = None
+            instrumentation_deps.append(str(dep))
+
+    return get_dependency_conflicts(instrumentation_deps)
 
 
 def get_dependency_conflicts(
     deps: Collection[str],
 ) -> Optional[DependencyConflict]:
     for dep in deps:
         try:
-            get_distribution(str(dep))
+            get_distribution(dep)
         except VersionConflict as exc:
             return DependencyConflict(dep, exc.dist)
         except DistributionNotFound:
             return DependencyConflict(dep)
+        except RequirementParseError as exc:
+            logger.warning(
+                'error parsing dependency, reporting as a conflict: "%s" - %s',
+                dep,
+                exc,
+            )
+            return DependencyConflict(dep)
     return None

opentelemetry-instrumentation/tests/test_dependencies.py

@@ -14,11 +14,13 @@
 
 # pylint: disable=protected-access
 
+import pkg_resources
 import pytest
 
 from opentelemetry.instrumentation.dependencies import (
     DependencyConflict,
     get_dependency_conflicts,
+    get_dist_dependency_conflicts,
 )
 from opentelemetry.test.test_base import TestBase
 
@@ -37,7 +39,6 @@ def test_get_dependency_conflicts_not_installed(self):
         conflict = get_dependency_conflicts(["this-package-does-not-exist"])
         self.assertTrue(conflict is not None)
         self.assertTrue(isinstance(conflict, DependencyConflict))
-        print(conflict)
         self.assertEqual(
             str(conflict),
             'DependencyConflict: requested: "this-package-does-not-exist" but found: "None"',
@@ -47,10 +48,32 @@ def test_get_dependency_conflicts_mismatched_version(self):
         conflict = get_dependency_conflicts(["pytest == 5000"])
         self.assertTrue(conflict is not None)
         self.assertTrue(isinstance(conflict, DependencyConflict))
-        print(conflict)
         self.assertEqual(
             str(conflict),
             'DependencyConflict: requested: "pytest == 5000" but found: "pytest {0}"'.format(
                 pytest.__version__
             ),
         )
+
+    def test_get_dist_dependency_conflicts(self):
+        def mock_requires(extras=()):
+            if "instruments" in extras:
+                return [
+                    pkg_resources.Requirement(
+                        'test-pkg ~= 1.0; extra == "instruments"'
+                    )
+                ]
+            return []
+
+        dist = pkg_resources.Distribution(
+            project_name="test-instrumentation", version="1.0"
+        )
+        dist.requires = mock_requires
+
+        conflict = get_dist_dependency_conflicts(dist)
+        self.assertTrue(conflict is not None)
+        self.assertTrue(isinstance(conflict, DependencyConflict))
+        self.assertEqual(
+            str(conflict),
+            'DependencyConflict: requested: "test-pkg~=1.0" but found: "None"',
+        )

scripts/generate_instrumentation_bootstrap.py

@@ -18,6 +18,7 @@
 import logging
 import os
 import subprocess
+import sys
 
 import astor
 import pkg_resources
@@ -91,7 +92,18 @@ def main():
     with open(gen_path, "w") as gen_file:
         gen_file.write(source)
 
-    subprocess.run(["black", "-q", gen_path], check=True)
+    subprocess.run(
+        [
+            sys.executable,
+            "scripts/eachdist.py",
+            "format",
+            "--path",
+            "opentelemetry-instrumentation",
+        ],
+        check=True,
+    )
+
+    logger.info("generated %s", gen_path)
 
 
 if __name__ == "__main__":

scripts/otel_packaging.py

@@ -27,12 +27,13 @@ def get_instrumentation_packages():
         if not os.path.isdir(pkg_path):
             continue
 
-        out = str(
-            subprocess.check_output(
-                "python setup.py meta", shell=True, cwd=pkg_path
-            )
+        out = subprocess.check_output(
+            "python setup.py meta",
+            shell=True,
+            cwd=pkg_path,
+            universal_newlines=True,
         )
-        instrumentation = json.loads(out.split("\\n")[1])
+        instrumentation = json.loads(out.splitlines()[1])
         instrumentation["requirement"] = "==".join(
             (instrumentation["name"], instrumentation["version"],)
         )

tests/opentelemetry-docker-tests/tests/sqlalchemy_tests/mixins.py

@@ -13,10 +13,12 @@
 # limitations under the License.
 
 import contextlib
+import logging
+import threading
 
-from sqlalchemy import Column, Integer, String, create_engine
+from sqlalchemy import Column, Integer, String, create_engine, insert
 from sqlalchemy.ext.declarative import declarative_base
-from sqlalchemy.orm import sessionmaker
+from sqlalchemy.orm import close_all_sessions, scoped_session, sessionmaker
 
 from opentelemetry import trace
 from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor
@@ -199,3 +201,45 @@ def test_parent(self):
         self.assertEqual(parent_span.instrumentation_info.name, "sqlalch_svc")
 
         self.assertEqual(child_span.name, "SELECT " + self.SQL_DB)
+
+    def test_multithreading(self):
+        """Ensure spans are captured correctly in a multithreading scenario
+
+        We also expect no logged warnings about calling end() on an ended span.
+        """
+
+        if self.VENDOR == "sqlite":
+            return
+
+        def insert_player(session):
+            _session = session()
+            player = Player(name="Player")
+            _session.add(player)
+            _session.commit()
+            _session.query(Player).all()
+
+        def insert_players(session):
+            _session = session()
+            players = []
+            for player_number in range(3):
+                players.append(Player(name=f"Player {player_number}"))
+            _session.add_all(players)
+            _session.commit()
+
+        session_factory = sessionmaker(bind=self.engine)
+        # pylint: disable=invalid-name
+        Session = scoped_session(session_factory)
+        thread_one = threading.Thread(target=insert_player, args=(Session,))
+        thread_two = threading.Thread(target=insert_players, args=(Session,))
+
+        logger = logging.getLogger("opentelemetry.sdk.trace")
+        with self.assertRaises(AssertionError):
+            with self.assertLogs(logger, level="WARNING"):
+                thread_one.start()
+                thread_two.start()
+                thread_one.join()
+                thread_two.join()
+                close_all_sessions()
+
+        spans = self.memory_exporter.get_finished_spans()
+        self.assertEqual(len(spans), 5)