From caf990c8f70d4ac98167b8548334a8ee55581421 Mon Sep 17 00:00:00 2001 From: Pawel Filipczak Date: Mon, 4 Nov 2024 10:35:10 +0100 Subject: [PATCH] Fixes after code review --- .../Curl/src/CurlHandleMetadata.php | 32 +++++++++++++++++-- .../Curl/src/CurlInstrumentation.php | 30 ++--------------- .../Integration/CurlInstrumentationTest.php | 13 ++++++++ 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/src/Instrumentation/Curl/src/CurlHandleMetadata.php b/src/Instrumentation/Curl/src/CurlHandleMetadata.php index 3d91f273..90f57591 100644 --- 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 index cf8f6d56..299639f1 100644 --- 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,26 +380,6 @@ 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); @@ -407,14 +387,14 @@ private static function transformHeaderStringToArray(string $header): array $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 index 62001163..7707f1f8 100644 --- 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();