Fix urllib3 patching not activating tracing (#2349) (#2352)

* Fix urllib3 patching not activating tracing

* remove unused import

* update circleci job

* Always use docker-compose httpbin for urllib3 tests

* forgot to checkout the code

(cherry picked from commit 8b33989)

Co-authored-by: Brett Langdon <[email protected]>

Author: mergify[bot]

.circleci/config.yml

@@ -929,14 +929,14 @@ jobs:
           pattern: '^rediscluster_contrib-'
 
   urllib3:
-    <<: *contrib_job
-    docker:
-      - image: *ddtrace_dev_image
-      - *httpbin_local
-    resource_class: *resource_class
+    <<: *machine_executor
     steps:
+      - checkout
+      # `snapshot: true` will run `docker-compose logs -f` for us
+      - run: docker-compose up -d httpbin_local
       - run_test:
           pattern: 'urllib3'
+          snapshot: true
 
   vertica:
     <<: *contrib_job

ddtrace/contrib/urllib3/patch.py

@@ -37,6 +37,7 @@ def patch():
     setattr(urllib3, "__datadog_patch", True)
 
     _w("urllib3", "connectionpool.HTTPConnectionPool.urlopen", _wrap_urlopen)
+    Pin().onto(urllib3.connectionpool.HTTPConnectionPool)
 
 
 def unpatch():

docker-compose.yml

@@ -139,5 +139,11 @@ services:
           - "${TMPDIR:-/tmp/localstack}:/tmp/localstack"
           - "/var/run/docker.sock:/var/run/docker.sock"
 
+    httpbin_local:
+      image: kennethreitz/httpbin@sha256:2c7abc4803080c22928265744410173b6fea3b898872c01c5fd0f0f9df4a59fb
+      ports:
+        - "127.0.0.1:8001:80"
+
+
 volumes:
     ddagent:

releasenotes/notes/fix.urllib3.integration-610312498fcdce86.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Fix ``urllib3`` patching not properly activating the integration.

tests/contrib/urllib3/test_urllib3.py

@@ -11,10 +11,13 @@
 from ddtrace.pin import Pin
 from tests.opentracer.utils import init_tracer
 from tests.utils import TracerTestCase
+from tests.utils import snapshot
 
 
-# socket name comes from https://english.stackexchange.com/a/44048
-SOCKET = "httpbin.org"
+# host:port of httpbin_local container
+HOST = "localhost"
+PORT = 8001
+SOCKET = "{}:{}".format(HOST, PORT)
 URL_200 = "http://{}/status/200".format(SOCKET)
 URL_500 = "http://{}/status/500".format(SOCKET)
 
@@ -37,7 +40,7 @@ def tearDown(self):
 class TestUrllib3(BaseUrllib3TestCase):
     def test_HTTPConnectionPool_traced(self):
         """Tests that requests made from the HTTPConnectionPool are traced"""
-        pool = urllib3.connectionpool.HTTPConnectionPool(SOCKET)
+        pool = urllib3.connectionpool.HTTPConnectionPool(HOST, PORT)
         # Test a relative URL
         r = pool.request("GET", "/status/200")
         assert r.status == 200
@@ -98,9 +101,9 @@ def test_args_kwargs(self):
 
         for args, kwargs in inputs:
 
-            with self.override_config("urllib3", {}):
+            with self.override_http_config("urllib3", {"_whitelist_headers": set()}):
                 config.urllib3.http.trace_headers(["accept"])
-                pool = urllib3.connectionpool.HTTPConnectionPool(SOCKET)
+                pool = urllib3.connectionpool.HTTPConnectionPool(HOST, PORT)
                 out = pool.urlopen(*args, **kwargs)
             assert out.status == 200
             spans = self.pop_spans()
@@ -124,7 +127,7 @@ def test_untraced_request(self):
     def test_double_patch(self):
         """Ensure that double patch doesn't duplicate instrumentation"""
         patch()
-        connpool = urllib3.connectionpool.HTTPConnectionPool(SOCKET)
+        connpool = urllib3.connectionpool.HTTPConnectionPool(HOST, PORT)
         setattr(connpool, "datadog_tracer", self.tracer)
 
         out = connpool.urlopen("GET", URL_200)
@@ -209,8 +212,7 @@ def test_default_service_name(self):
 
     def test_user_set_service_name(self):
         """Test the user-set service name is set on the span"""
-        with self.override_config("urllib3", dict(split_by_domain=False)):
-            config.urllib3["service_name"] = "clients"
+        with self.override_config("urllib3", dict(split_by_domain=False, service_name="clients")):
             out = self.http.request("GET", URL_200)
         assert out.status == 200
         spans = self.pop_spans()
@@ -312,7 +314,7 @@ def test_request_and_response_headers(self):
         assert s.get_tag("http.response.headers.access-control-allow-origin") is None
 
         # Enabled when explicitly configured
-        with self.override_config("urllib3", {}):
+        with self.override_http_config("urllib3", {"_whitelist_headers": set()}):
             config.urllib3.http.trace_headers(["my-header", "access-control-allow-origin"])
             self.http.request("GET", URL_200, headers={"my-header": "my_value"})
             spans = self.pop_spans()
@@ -384,3 +386,24 @@ def test_distributed_tracing_disabled(self):
             m_make_request.assert_called_with(
                 mock.ANY, "GET", "/status/200", body=None, chunked=mock.ANY, headers={}, timeout=mock.ANY
             )
+
+
+@pytest.fixture()
+def patch_urllib3():
+    patch()
+    try:
+        yield
+    finally:
+        unpatch()
+
+
+@snapshot()
+def test_urllib3_poolmanager_snapshot(patch_urllib3):
+    pool = urllib3.PoolManager()
+    pool.request("GET", URL_200)
+
+
+@snapshot()
+def test_urllib3_connectionpool_snapshot(patch_urllib3):
+    pool = urllib3.connectionpool.HTTPConnectionPool(HOST, PORT)
+    pool.request("GET", "/status/200")

tests/snapshots/tests.contrib.urllib3.test_urllib3.test_urllib3_connectionpool_snapshot.snap

@@ -0,0 +1,18 @@
+[[{"name" "urllib3.request"
+   "service" "urllib3"
+   "resource" "urllib3.request"
+   "type" "http"
+   "error" 0
+   "span_id" 0
+   "trace_id" 0
+   "parent_id" nil
+   "start" 1619121392243862000
+   "duration" 9724000
+   "meta" {"runtime-id" "17c4dec94e02426e936e6a7c445f2a38"
+           "http.method" "GET"
+           "http.url" "http://localhost:8001/status/200"
+           "http.status_code" "200"}
+   "metrics" {"_dd.agent_psr" 1.0
+              "_sampling_priority_v1" 1
+              "system.pid" 39986
+              "_dd.tracer_kr" 1.0}}]]

tests/snapshots/tests.contrib.urllib3.test_urllib3.test_urllib3_poolmanager_snapshot.snap

@@ -0,0 +1,18 @@
+[[{"name" "urllib3.request"
+   "service" "urllib3"
+   "resource" "urllib3.request"
+   "type" "http"
+   "error" 0
+   "span_id" 0
+   "trace_id" 0
+   "parent_id" nil
+   "start" 1619121391909007000
+   "duration" 10709000
+   "meta" {"runtime-id" "17c4dec94e02426e936e6a7c445f2a38"
+           "http.method" "GET"
+           "http.url" "http://localhost:8001/status/200"
+           "http.status_code" "200"}
+   "metrics" {"_dd.agent_psr" 1.0
+              "_sampling_priority_v1" 1
+              "system.pid" 39986
+              "_dd.tracer_kr" 1.0}}]]