Merge branch 'main' into django-asgi

Author: ocelotl

CHANGELOG.md

@@ -6,6 +6,15 @@ 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)
 
+### Changed
+- `opentelemetry-instrumentation-tornado` properly instrument work done in tornado on_finish method.
+  ([#499](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/499))
+- `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))
+- Fix weak reference error for pyodbc cursor in SQLAlchemy instrumentation.
+  ([#469](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/469))
+
 ## [0.22b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.3.0-0.22b0) - 2021-06-01
 
 ### Changed

instrumentation/opentelemetry-instrumentation-sqlalchemy/src/opentelemetry/instrumentation/sqlalchemy/engine.py

@@ -13,7 +13,6 @@
 # limitations under the License.
 
 from threading import local
-from weakref import WeakKeyDictionary
 
 from sqlalchemy.event import listen  # pylint: disable=no-name-in-module
 
@@ -60,7 +59,7 @@ def __init__(self, tracer, engine):
         self.tracer = tracer
         self.engine = engine
         self.vendor = _normalize_vendor(engine.name)
-        self.cursor_mapping = WeakKeyDictionary()
+        self.cursor_mapping = {}
         self.local = local()
 
         listen(engine, "before_cursor_execute", self._before_cur_exec)
@@ -116,6 +115,7 @@ def _after_cur_exec(self, conn, cursor, statement, *args):
             return
 
         span.end()
+        self._cleanup(cursor)
 
     def _handle_error(self, context):
         span = self.current_thread_span
@@ -129,6 +129,13 @@ def _handle_error(self, context):
                 )
         finally:
             span.end()
+            self._cleanup(context.cursor)
+
+    def _cleanup(self, cursor):
+        try:
+            del self.cursor_mapping[cursor]
+        except KeyError:
+            pass
 
 
 def _get_attributes_from_url(url):

instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py

@@ -213,8 +213,9 @@ def _prepare(tracer, request_hook, func, handler, args, kwargs):
 
 
 def _on_finish(tracer, func, handler, args, kwargs):
+    response = func(*args, **kwargs)
     _finish_span(tracer, handler)
-    return func(*args, **kwargs)
+    return response
 
 
 def _log_exception(tracer, func, handler, args, kwargs):

instrumentation/opentelemetry-instrumentation-tornado/tests/test_instrumentation.py

@@ -347,6 +347,54 @@ def test_dynamic_handler(self):
             },
         )
 
+    def test_handler_on_finish(self):
+
+        response = self.fetch("/on_finish")
+        self.assertEqual(response.code, 200)
+
+        spans = self.sorted_spans(self.memory_exporter.get_finished_spans())
+        self.assertEqual(len(spans), 3)
+        auditor, server, client = spans
+
+        self.assertEqual(server.name, "FinishedHandler.get")
+        self.assertTrue(server.parent.is_remote)
+        self.assertNotEqual(server.parent, client.context)
+        self.assertEqual(server.parent.span_id, client.context.span_id)
+        self.assertEqual(server.context.trace_id, client.context.trace_id)
+        self.assertEqual(server.kind, SpanKind.SERVER)
+        self.assert_span_has_attributes(
+            server,
+            {
+                SpanAttributes.HTTP_METHOD: "GET",
+                SpanAttributes.HTTP_SCHEME: "http",
+                SpanAttributes.HTTP_HOST: "127.0.0.1:"
+                + str(self.get_http_port()),
+                SpanAttributes.HTTP_TARGET: "/on_finish",
+                SpanAttributes.NET_PEER_IP: "127.0.0.1",
+                SpanAttributes.HTTP_STATUS_CODE: 200,
+            },
+        )
+
+        self.assertEqual(client.name, "GET")
+        self.assertFalse(client.context.is_remote)
+        self.assertIsNone(client.parent)
+        self.assertEqual(client.kind, SpanKind.CLIENT)
+        self.assert_span_has_attributes(
+            client,
+            {
+                SpanAttributes.HTTP_URL: self.get_url("/on_finish"),
+                SpanAttributes.HTTP_METHOD: "GET",
+                SpanAttributes.HTTP_STATUS_CODE: 200,
+            },
+        )
+
+        self.assertEqual(auditor.name, "audit_task")
+        self.assertFalse(auditor.context.is_remote)
+        self.assertEqual(auditor.parent.span_id, server.context.span_id)
+        self.assertEqual(auditor.context.trace_id, client.context.trace_id)
+
+        self.assertEqual(auditor.kind, SpanKind.INTERNAL)
+
     def test_exclude_lists(self):
         def test_excluded(path):
             self.fetch(path)

instrumentation/opentelemetry-instrumentation-tornado/tests/tornado_test_app.py

@@ -1,4 +1,5 @@
 # pylint: disable=W0223,R0201
+import time
 
 import tornado.web
 from tornado import gen
@@ -79,6 +80,16 @@ def get(self):
         self.set_status(202)
 
 
+class FinishedHandler(tornado.web.RequestHandler):
+    def on_finish(self):
+        with self.application.tracer.start_as_current_span("audit_task"):
+            time.sleep(0.05)
+
+    def get(self):
+        self.write("Test can finish")
+        self.set_status(200)
+
+
 class HealthCheckHandler(tornado.web.RequestHandler):
     def get(self):
         self.set_status(200)
@@ -91,6 +102,7 @@ def make_app(tracer):
             (r"/error", BadHandler),
             (r"/cor", CoroutineHandler),
             (r"/async", AsyncHandler),
+            (r"/on_finish", FinishedHandler),
             (r"/healthz", HealthCheckHandler),
             (r"/ping", HealthCheckHandler),
         ]

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"',
+        )

tests/opentelemetry-docker-tests/tests/check_availability.py

@@ -18,6 +18,7 @@
 import mysql.connector
 import psycopg2
 import pymongo
+import pyodbc
 import redis
 
 MONGODB_COLLECTION_NAME = "test"
@@ -36,6 +37,11 @@
 POSTGRES_USER = os.getenv("POSTGRESQL_USER", "testuser")
 REDIS_HOST = os.getenv("REDIS_HOST", "localhost")
 REDIS_PORT = int(os.getenv("REDIS_PORT ", "6379"))
+MSSQL_DB_NAME = os.getenv("MSSQL_DB_NAME", "opentelemetry-tests")
+MSSQL_HOST = os.getenv("MSSQL_HOST", "localhost")
+MSSQL_PORT = int(os.getenv("MSSQL_PORT", "1433"))
+MSSQL_USER = os.getenv("MSSQL_USER", "sa")
+MSSQL_PASSWORD = os.getenv("MSSQL_PASSWORD", "yourStrong(!)Password")
 RETRY_COUNT = 8
 RETRY_INTERVAL = 5  # Seconds
 
@@ -104,12 +110,23 @@ def check_redis_connection():
     connection.hgetall("*")
 
 
+@retryable
+def check_mssql_connection():
+    connection = pyodbc.connect(
+        f"DRIVER={{ODBC Driver 17 for SQL Server}};SERVER={MSSQL_HOST},"
+        f"{MSSQL_PORT};DATABASE={MSSQL_DB_NAME};UID={MSSQL_USER};"
+        f"PWD={MSSQL_PASSWORD}"
+    )
+    connection.close()
+
+
 def check_docker_services_availability():
     # Check if Docker services accept connections
     check_pymongo_connection()
     check_mysql_connection()
     check_postgres_connection()
     check_redis_connection()
+    check_mssql_connection()
 
 
 check_docker_services_availability()

tests/opentelemetry-docker-tests/tests/docker-compose.yml

@@ -39,3 +39,11 @@ services:
         - "16686:16686"
         - "14268:14268"
         - "9411:9411"
+  otmssql:
+    image: mcr.microsoft.com/mssql/server:2017-latest
+    ports:
+      - "1433:1433"
+    environment:
+      ACCEPT_EULA: "Y"
+      SA_PASSWORD: "yourStrong(!)Password"
+    command: /bin/sh -c "sleep 10s && /opt/mssql-tools/bin/sqlcmd -S localhost -U sa -P yourStrong\(!\)Password -d master -Q 'CREATE DATABASE [opentelemetry-tests]' & /opt/mssql/bin/sqlservr"