Merge branch 'main' into issue-1757

Author: shalevr

Diff for: CHANGELOG.md

@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+- Instrument all httpx versions >= 0.18. ([#1748](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1748))
+
 ## Version 1.18.0/0.39b0 (2023-05-10)
 
 - `opentelemetry-instrumentation-system-metrics` Add `process.` prefix to `runtime.memory`, `runtime.cpu.time`, and `runtime.gc_count`. Change `runtime.memory` from count to UpDownCounter. ([#1735](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1735))
@@ -38,12 +40,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Fix redis db.statements to be sanitized by default
+  ([#1778](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1778))
 - Fix elasticsearch db.statement attribute to be sanitized by default
   ([#1758](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1758))
 - Fix `AttributeError` when AWS Lambda handler receives a list event
   ([#1738](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1738))
 - Fix `None does not implement middleware` error when there are no middlewares registered
   ([#1766](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1766))
+- Fix Flask instrumentation to only close the span if it was created by the same request context.
+  ([#1692](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1692))
 
 ### Changed
 - Update HTTP server/client instrumentation span names to comply with spec

Diff for: dev-requirements.txt

@@ -14,7 +14,7 @@ bleach==4.1.0 # transient dependency for readme-renderer
 grpcio-tools==1.29.0
 mypy-protobuf>=1.23
 protobuf~=3.13
-markupsafe==2.0.1
+markupsafe>=2.0.1
 codespell==2.1.0
 requests==2.28.1
 ruamel.yaml==0.17.21

Diff for: instrumentation/opentelemetry-instrumentation-aiohttp-client/pyproject.toml

@@ -38,6 +38,7 @@ instruments = [
 ]
 test = [
   "opentelemetry-instrumentation-aiohttp-client[instruments]",
+  "http-server-mock"
 ]
 
 [project.entry-points.opentelemetry_instrumentor]

Diff for: instrumentation/opentelemetry-instrumentation-aiohttp-client/tests/test_aiohttp_client_integration.py

@@ -23,6 +23,7 @@
 import aiohttp
 import aiohttp.test_utils
 import yarl
+from http_server_mock import HttpServerMock
 from pkg_resources import iter_entry_points
 
 from opentelemetry import context
@@ -313,18 +314,26 @@ async def request_handler(request):
     def test_credential_removal(self):
         trace_configs = [aiohttp_client.create_trace_config()]
 
-        url = "http://username:[email protected]/status/200"
-        with self.subTest(url=url):
+        app = HttpServerMock("test_credential_removal")
 
-            async def do_request(url):
-                async with aiohttp.ClientSession(
-                    trace_configs=trace_configs,
-                ) as session:
-                    async with session.get(url):
-                        pass
+        @app.route("/status/200")
+        def index():
+            return "hello"
 
-            loop = asyncio.get_event_loop()
-            loop.run_until_complete(do_request(url))
+        url = "http://username:password@localhost:5000/status/200"
+
+        with app.run("localhost", 5000):
+            with self.subTest(url=url):
+
+                async def do_request(url):
+                    async with aiohttp.ClientSession(
+                        trace_configs=trace_configs,
+                    ) as session:
+                        async with session.get(url):
+                            pass
+
+                loop = asyncio.get_event_loop()
+                loop.run_until_complete(do_request(url))
 
         self.assert_spans(
             [
@@ -333,7 +342,9 @@ async def do_request(url):
                     (StatusCode.UNSET, None),
                     {
                         SpanAttributes.HTTP_METHOD: "GET",
-                        SpanAttributes.HTTP_URL: "http://httpbin.org/status/200",
+                        SpanAttributes.HTTP_URL: (
+                            "http://localhost:5000/status/200"
+                        ),
                         SpanAttributes.HTTP_STATUS_CODE: int(HTTPStatus.OK),
                     },
                 )

Diff for: instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py

@@ -705,11 +705,11 @@ def test_response_attributes_invalid_status_code(self):
         self.assertEqual(self.span.set_status.call_count, 1)
 
     def test_credential_removal(self):
-        self.scope["server"] = ("username:password@httpbin.org", 80)
+        self.scope["server"] = ("username:password@mock", 80)
         self.scope["path"] = "/status/200"
         attrs = otel_asgi.collect_request_attributes(self.scope)
         self.assertEqual(
-            attrs[SpanAttributes.HTTP_URL], "http://httpbin.org/status/200"
+            attrs[SpanAttributes.HTTP_URL], "http://mock/status/200"
         )
 
     def test_collect_target_attribute_missing(self):

Diff for: instrumentation/opentelemetry-instrumentation-flask/pyproject.toml

@@ -30,6 +30,7 @@ dependencies = [
   "opentelemetry-instrumentation-wsgi == 0.40b0.dev",
   "opentelemetry-semantic-conventions == 0.40b0.dev",
   "opentelemetry-util-http == 0.40b0.dev",
+  "packaging >= 21.0",
 ]
 
 [project.optional-dependencies]
@@ -38,7 +39,7 @@ instruments = [
 ]
 test = [
   "opentelemetry-instrumentation-flask[instruments]",
-  "markupsafe==2.0.1",
+  "markupsafe==2.1.2",
   "opentelemetry-test-utils == 0.40b0.dev",
 ]
 

Diff for: instrumentation/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py

@@ -238,13 +238,14 @@ def response_hook(span: Span, status: str, response_headers: List):
 API
 ---
 """
+import weakref
 from logging import getLogger
-from threading import get_ident
 from time import time_ns
 from timeit import default_timer
 from typing import Collection
 
 import flask
+from packaging import version as package_version
 
 import opentelemetry.instrumentation.wsgi as otel_wsgi
 from opentelemetry import context, trace
@@ -265,11 +266,21 @@ def response_hook(span: Span, status: str, response_headers: List):
 _ENVIRON_STARTTIME_KEY = "opentelemetry-flask.starttime_key"
 _ENVIRON_SPAN_KEY = "opentelemetry-flask.span_key"
 _ENVIRON_ACTIVATION_KEY = "opentelemetry-flask.activation_key"
-_ENVIRON_THREAD_ID_KEY = "opentelemetry-flask.thread_id_key"
+_ENVIRON_REQCTX_REF_KEY = "opentelemetry-flask.reqctx_ref_key"
 _ENVIRON_TOKEN = "opentelemetry-flask.token"
 
 _excluded_urls_from_env = get_excluded_urls("FLASK")
 
+if package_version.parse(flask.__version__) >= package_version.parse("2.2.0"):
+
+    def _request_ctx_ref() -> weakref.ReferenceType:
+        return weakref.ref(flask.globals.request_ctx._get_current_object())
+
+else:
+
+    def _request_ctx_ref() -> weakref.ReferenceType:
+        return weakref.ref(flask._request_ctx_stack.top)
+
 
 def get_default_span_name():
     try:
@@ -399,7 +410,7 @@ def _before_request():
         activation = trace.use_span(span, end_on_exit=True)
         activation.__enter__()  # pylint: disable=E1101
         flask_request_environ[_ENVIRON_ACTIVATION_KEY] = activation
-        flask_request_environ[_ENVIRON_THREAD_ID_KEY] = get_ident()
+        flask_request_environ[_ENVIRON_REQCTX_REF_KEY] = _request_ctx_ref()
         flask_request_environ[_ENVIRON_SPAN_KEY] = span
         flask_request_environ[_ENVIRON_TOKEN] = token
 
@@ -439,17 +450,22 @@ def _teardown_request(exc):
             return
 
         activation = flask.request.environ.get(_ENVIRON_ACTIVATION_KEY)
-        thread_id = flask.request.environ.get(_ENVIRON_THREAD_ID_KEY)
-        if not activation or thread_id != get_ident():
+
+        original_reqctx_ref = flask.request.environ.get(
+            _ENVIRON_REQCTX_REF_KEY
+        )
+        current_reqctx_ref = _request_ctx_ref()
+        if not activation or original_reqctx_ref != current_reqctx_ref:
             # This request didn't start a span, maybe because it was created in
             # a way that doesn't run `before_request`, like when it is created
             # with `app.test_request_context`.
             #
-            # Similarly, check the thread_id against the current thread to ensure
-            # tear down only happens on the original thread. This situation can
-            # arise if the original thread handling the request spawn children
-            # threads and then uses something like copy_current_request_context
-            # to copy the request context.
+            # Similarly, check that the request_ctx that created the span
+            # matches the current request_ctx, and only tear down if they match.
+            # This situation can arise if the original request_ctx handling
+            # the request calls functions that push new request_ctx's,
+            # like any decorated with `flask.copy_current_request_context`.
+
             return
         if exc is None:
             activation.__exit__(None, None, None)

Diff for: instrumentation/opentelemetry-instrumentation-flask/tests/base_test.py

@@ -19,7 +19,7 @@
 from werkzeug.test import Client
 from werkzeug.wrappers import Response
 
-from opentelemetry import context
+from opentelemetry import context, trace
 
 
 class InstrumentationTest:
@@ -37,6 +37,21 @@ def _sqlcommenter_endpoint():
         )
         return sqlcommenter_flask_values
 
+    @staticmethod
+    def _copy_context_endpoint():
+        @flask.copy_current_request_context
+        def _extract_header():
+            return flask.request.headers["x-req"]
+
+        # Despite `_extract_header` copying the request context,
+        # calling it shouldn't detach the parent Flask span's contextvar
+        request_header = _extract_header()
+
+        return {
+            "span_name": trace.get_current_span().name,
+            "request_header": request_header,
+        }
+
     @staticmethod
     def _multithreaded_endpoint(count):
         def do_random_stuff():
@@ -84,6 +99,7 @@ def excluded2_endpoint():
         self.app.route("/hello/<int:helloid>")(self._hello_endpoint)
         self.app.route("/sqlcommenter")(self._sqlcommenter_endpoint)
         self.app.route("/multithreaded")(self._multithreaded_endpoint)
+        self.app.route("/copy_context")(self._copy_context_endpoint)
         self.app.route("/excluded/<int:helloid>")(self._hello_endpoint)
         self.app.route("/excluded")(excluded_endpoint)
         self.app.route("/excluded2")(excluded2_endpoint)

Diff for: instrumentation/opentelemetry-instrumentation-flask/tests/test_copy_context.py

@@ -0,0 +1,48 @@
+# Copyright The OpenTelemetry Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+import flask
+from werkzeug.test import Client
+from werkzeug.wrappers import Response
+
+from opentelemetry.instrumentation.flask import FlaskInstrumentor
+from opentelemetry.test.wsgitestutil import WsgiTestBase
+
+from .base_test import InstrumentationTest
+
+
+class TestCopyContext(InstrumentationTest, WsgiTestBase):
+    def setUp(self):
+        super().setUp()
+        FlaskInstrumentor().instrument()
+        self.app = flask.Flask(__name__)
+        self._common_initialization()
+
+    def tearDown(self):
+        super().tearDown()
+        with self.disable_logging():
+            FlaskInstrumentor().uninstrument()
+
+    def test_copycontext(self):
+        """Test that instrumentation tear down does not blow up
+        when the request calls functions where the context has been
+        copied via `flask.copy_current_request_context`
+        """
+        self.app = flask.Flask(__name__)
+        self.app.route("/copy_context")(self._copy_context_endpoint)
+        client = Client(self.app, Response)
+        resp = client.get("/copy_context", headers={"x-req": "a-header"})
+
+        self.assertEqual(200, resp.status_code)
+        self.assertEqual("/copy_context", resp.json["span_name"])
+        self.assertEqual("a-header", resp.json["request_header"])

Diff for: instrumentation/opentelemetry-instrumentation-httpx/README.rst

@@ -30,7 +30,7 @@ When using the instrumentor, all clients will automatically trace requests.
      import httpx
      from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor
 
-     url = "https://httpbin.org/get"
+     url = "https://some.url/get"
      HTTPXClientInstrumentor().instrument()
 
      with httpx.Client() as client:
@@ -51,7 +51,7 @@ use the `instrument_client` method.
     import httpx
     from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor
 
-    url = "https://httpbin.org/get"
+    url = "https://some.url/get"
 
     with httpx.Client(transport=telemetry_transport) as client:
         HTTPXClientInstrumentor.instrument_client(client)
@@ -96,7 +96,7 @@ If you don't want to use the instrumentor class, you can use the transport class
         SyncOpenTelemetryTransport,
     )
 
-    url = "https://httpbin.org/get"
+    url = "https://some.url/get"
     transport = httpx.HTTPTransport()
     telemetry_transport = SyncOpenTelemetryTransport(transport)
 

Diff for: instrumentation/opentelemetry-instrumentation-httpx/pyproject.toml

@@ -32,7 +32,7 @@ dependencies = [
 
 [project.optional-dependencies]
 instruments = [
-  "httpx >= 0.18.0, <= 0.23.0",
+  "httpx >= 0.18.0",
 ]
 test = [
   "opentelemetry-instrumentation-httpx[instruments]",

Diff for: instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py

@@ -25,7 +25,7 @@
      import httpx
      from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor
 
-     url = "https://httpbin.org/get"
+     url = "https://some.url/get"
      HTTPXClientInstrumentor().instrument()
 
      with httpx.Client() as client:
@@ -46,7 +46,7 @@
     import httpx
     from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor
 
-    url = "https://httpbin.org/get"
+    url = "https://some.url/get"
 
     with httpx.Client(transport=telemetry_transport) as client:
         HTTPXClientInstrumentor.instrument_client(client)
@@ -91,7 +91,7 @@
         SyncOpenTelemetryTransport,
     )
 
-    url = "https://httpbin.org/get"
+    url = "https://some.url/get"
     transport = httpx.HTTPTransport()
     telemetry_transport = SyncOpenTelemetryTransport(transport)
 

Diff for: instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py

@@ -59,7 +59,7 @@ def _async_call(coro: typing.Coroutine) -> asyncio.Task:
 def _response_hook(span, request: "RequestInfo", response: "ResponseInfo"):
     span.set_attribute(
         HTTP_RESPONSE_BODY,
-        response[2].read(),
+        b"".join(response[2]),
     )
 
 
@@ -68,7 +68,7 @@ async def _async_response_hook(
 ):
     span.set_attribute(
         HTTP_RESPONSE_BODY,
-        await response[2].aread(),
+        b"".join([part async for part in response[2]]),
     )
 
 
@@ -97,7 +97,7 @@ class BaseTestCases:
     class BaseTest(TestBase, metaclass=abc.ABCMeta):
         # pylint: disable=no-member
 
-        URL = "http://httpbin.org/status/200"
+        URL = "http://mock/status/200"
         response_hook = staticmethod(_response_hook)
         request_hook = staticmethod(_request_hook)
         no_update_request_hook = staticmethod(_no_update_request_hook)
@@ -165,7 +165,7 @@ def test_basic_multiple(self):
             self.assert_span(num_spans=2)
 
         def test_not_foundbasic(self):
-            url_404 = "http://httpbin.org/status/404"
+            url_404 = "http://mock/status/404"
 
             with respx.mock:
                 respx.get(url_404).mock(httpx.Response(404))