open-telemetry / opentelemetry-php

The OpenTelemetry PHP Library
https://opentelemetry.io/docs/instrumentation/php/
Apache License 2.0
686 stars 170 forks source link

contrib/otlp tests segfault #1338

Open brettmc opened 1 week ago

brettmc commented 1 week ago

Our CI started failing today with a segfault (and I noticed it locally yesterday, but hoped it was something screwy with my machine).

The segfault is triggered by using ReflectionObject::getProperties on a protobuf message (eg our Opentelemetry\Proto\Common\V1\AnyValue) when the protobuf extension is installed (but not the native version).

We have a test (tests/Unit/Contrib/Otlp/SpanConverterTest.php test_attribute_are_coerced_correctly) that use a data provider containing AnyValue. The fun begins when phpunit processes data providers, and at some point uses reflection on the data, then kaboom. The code it fails on is https://github.com/sebastianbergmann/exporter/blame/main/src/Exporter.php#L132

I can reproduce with some simple code which will segfault:

$value = new \Opentelemetry\Proto\Common\V1\AnyValue(['string_value' => 'foo']);
$r = new \ReflectionObject($value);
$r->getProperties();

Going back to an earlier version of sebastian/exporter resolves the segfault.

I'm thinking that since reflecting protobuf messages is a long-standing issue (there are multiple old "won't fix" issues related to mocking Messages, which I suspect is caused by Reflection), we need to either get sebastian/exporter to not use reflection here, or failing that rewrite the affected test to not use a data provider.

brettmc commented 1 week ago

I was able to hack up sebastian/exporter to make everything green again, so have tried submitting a PR: https://github.com/sebastianbergmann/exporter/pull/66 :crossed_fingers:

brettmc commented 1 week ago

That change got merged, so now just need to wait for a new version to come out (setting to "blocked" until that happens)