Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cURL instrumentation: support partially successful curl_setopt_array #316

Merged
merged 1 commit into from
Nov 23, 2024
Merged

cURL instrumentation: support partially successful curl_setopt_array #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/Instrumentation/Curl/src/CurlInstrumentation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ public static function register(): void
pre: null,
post: static function ($obj, array $params, mixed $retVal) use ($curlHandleToAttributes) {
if ($retVal != true) {
if (curl_error($params[0])) {
foreach ($params[1] as $option => $value) {
if (!curl_setopt($params[0], $option, $value)) {
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we continue here instead, would it permit setting the subsequent options too? (in case there is more than one error)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it would, however I dont see a reason to do that since I think we should only be interested in recording options which would be used later (in the following curl handle execution).
trying to record all valid options might be useful (and could be done without continue too I think), but that is outside of scope of this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with @vikmovcan here. We shouldn't set the options if curl itself wouldn't - so if curl_setopt_array wouldn't set them, neither should we.

}
}
}

return;
}

Expand Down
13 changes: 13 additions & 0 deletions src/Instrumentation/Curl/tests/Integration/CurlInstrumentationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,19 @@ public function test_curl_setopt_array(): void
$this->assertStringContainsString('resolve host', $span->getStatus()->getDescription());
}

public function test_curl_setopt_array_partial_success(): void
{
$ch = curl_init();
curl_setopt_array($ch, [CURLOPT_POST => 1, CURLOPT_URL => 'http://gugugaga.gugugaga/', CURLOPT_SSLVERSION => 1000 ]);
curl_exec($ch);

$this->assertCount(1, $this->storage);
$span = $this->storage->offsetGet(0);
$this->assertSame('POST', $span->getName());
$this->assertSame('Error', $span->getStatus()->getCode());
$this->assertStringContainsString('resolve host', $span->getStatus()->getDescription());
}

public function test_curl_copy_handle(): void
{
$ch = curl_init('http://gugugaga.gugugaga/');
Expand Down
Loading