Fixes after code review

open-telemetry · Nov 4, 2024 · caf990c · caf990c
1 parent f9d9d7e
commit caf990c
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 30 deletions.
diff --git a/src/Instrumentation/Curl/src/CurlHandleMetadata.php b/src/Instrumentation/Curl/src/CurlHandleMetadata.php
@@ -120,7 +120,8 @@ public function updateFromCurlOption(int $option, mixed $value)
 
                 break;
             case CURLOPT_URL:
-                // $this->setAttribute(TraceAttributes::URL_FULL, self::redactUrlString($value));
+                $this->setAttribute(TraceAttributes::URL_FULL, self::redactUrlString($value));
+
                 break;
             case CURLOPT_USERAGENT:
                 $this->setAttribute(TraceAttributes::USER_AGENT_ORIGINAL, $value);
@@ -132,9 +133,34 @@ public function updateFromCurlOption(int $option, mixed $value)
                 break;
             case CURLOPT_HEADERFUNCTION:
                 $this->originalHeaderFunction = $value;
-                // no break
-            case CURLOPT_VERBOSE:
                 $this->verboseEnabled = false;
+
+                break;
+            case CURLOPT_VERBOSE:
+                $this->verboseEnabled = $value;
+
+                break;
         }
     }
+
+    public static function redactUrlString(string $fullUrl)
+    {
+        $urlParts = parse_url($fullUrl);
+        if ($urlParts == false) {
+            return;
+        }
+
+        $scheme   = isset($urlParts['scheme']) ? $urlParts['scheme'] . '://' : '';
+        $host     = isset($urlParts['host']) ? $urlParts['host'] : '';
+        $port     = isset($urlParts['port']) ? ':' . $urlParts['port'] : '';
+        $user     = isset($urlParts['user']) ? 'REDACTED' : '';
+        $pass     = isset($urlParts['pass']) ? ':' . 'REDACTED'  : '';
+        $pass     = ($user || $pass) ? "$pass@" : '';
+        $path     = isset($urlParts['path']) ? $urlParts['path'] : '';
+        $query    = isset($urlParts['query']) ? '?' . $urlParts['query'] : '';
+        $fragment = isset($urlParts['fragment']) ? '#' . $urlParts['fragment'] : '';
+
+        return $scheme . $user . $pass . $host . $port . $path . $query . $fragment;
+    }
+
 }
diff --git a/src/Instrumentation/Curl/src/CurlInstrumentation.php b/src/Instrumentation/Curl/src/CurlInstrumentation.php
@@ -57,7 +57,7 @@ public static function register(): void
                 if ($retVal instanceof CurlHandle) {
                     $curlHandleToAttributes[$retVal] = new CurlHandleMetadata();
                     if (($fullUrl = $params[0] ?? null) !== null) {
-                        $curlHandleToAttributes[$retVal]->setAttribute(TraceAttributes::URL_FULL, self::redactUrlString($fullUrl));
+                        $curlHandleToAttributes[$retVal]->setAttribute(TraceAttributes::URL_FULL, CurlHandleMetadata::redactUrlString($fullUrl));
                     }
                 }
             }
@@ -380,41 +380,21 @@ private static function finishMultiSpan(int $curlResult, CurlHandle $curlHandle,
         $span->end();
     }
 
-    private static function redactUrlString(string $fullUrl)
-    {
-        $urlParts = parse_url($fullUrl);
-        if ($urlParts == false) {
-            return;
-        }
-
-        $scheme   = isset($urlParts['scheme']) ? $urlParts['scheme'] . '://' : '';
-        $host     = isset($urlParts['host']) ? $urlParts['host'] : '';
-        $port     = isset($urlParts['port']) ? ':' . $urlParts['port'] : '';
-        $user     = isset($urlParts['user']) ? 'REDACTED' : '';
-        $pass     = isset($urlParts['pass']) ? ':' . 'REDACTED'  : '';
-        $pass     = ($user || $pass) ? "$pass@" : '';
-        $path     = isset($urlParts['path']) ? $urlParts['path'] : '';
-        $query    = isset($urlParts['query']) ? '?' . $urlParts['query'] : '';
-        $fragment = isset($urlParts['fragment']) ? '#' . $urlParts['fragment'] : '';
-
-        return $scheme . $user . $pass . $host . $port . $path . $query . $fragment;
-    }
-
     private static function transformHeaderStringToArray(string $header): array
     {
         $lines = explode("\n", $header);
         array_shift($lines); // skip request line
 
         $headersResult = [];
         foreach ($lines as $line) {
-            $line =  trim($line, "\r");
+            $line = trim($line, "\r");
             if (empty($line)) {
                 continue;
             }
 
             if (strpos($line, ': ') !== false) {
                 /** @psalm-suppress PossiblyUndefinedArrayOffset */
-                list($key, $value) = explode(': ', $line, 2);
+                [$key, $value] = explode(': ', $line, 2);
                 $headersResult[strtolower($key)] = $value;
             }
         }
@@ -444,10 +424,6 @@ private static function setAttributesFromCurlGetInfo(CurlHandle $handle, SpanInt
             $span->setAttribute(TraceAttributes::SERVER_PORT, $value);
         }
 
-        if (($value = $info['primary_port']) != 0) {
-            $span->setAttribute(TraceAttributes::SERVER_PORT, $value);
-        }
-
         /** @phpstan-ignore-next-line */
         if (($requestHeader = $info['request_header'] ?? null) != null) {
             $capturedHeaders = self::transformHeaderStringToArray($requestHeader);

diff --git a/src/Instrumentation/Curl/tests/Integration/CurlInstrumentationTest.php b/src/Instrumentation/Curl/tests/Integration/CurlInstrumentationTest.php
@@ -65,11 +65,24 @@ public function test_curl_setopt(): void
 
         $this->assertCount(1, $this->storage);
         $span = $this->storage->offsetGet(0);
+        $this->assertEquals('http://gugugaga.gugugaga/', $span->getAttributes()->get(TraceAttributes::URL_FULL));
         $this->assertSame('POST', $span->getName());
         $this->assertSame('Error', $span->getStatus()->getCode());
         $this->assertStringContainsString('resolve host', $span->getStatus()->getDescription());
     }
 
+    public function test_curl_setopt_overrides_url(): void
+    {
+        $ch = curl_init('http://example.com');
+        curl_setopt($ch, CURLOPT_POST, 1);
+        curl_setopt($ch, CURLOPT_URL, 'http://gugugaga.gugugaga/');
+        curl_exec($ch);
+
+        $this->assertCount(1, $this->storage);
+        $span = $this->storage->offsetGet(0);
+        $this->assertEquals('http://gugugaga.gugugaga/', $span->getAttributes()->get(TraceAttributes::URL_FULL));
+    }
+
     public function test_curl_setopt_array(): void
     {
         $ch = curl_init();