From 43724db81ea1367ab49e5235113313c526d3d042 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Fri, 6 Sep 2024 10:37:33 +1000 Subject: [PATCH 1/8] test against PHP nightly - build against PHP nightly on PR and daily schedule - use setup-php for linux builds (which drops Alpine builds) --- .github/workflows/build.yml | 28 +++++++++++++++------------- .github/workflows/nightly.yml | 31 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 .github/workflows/nightly.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a51e58f..3517753 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -16,21 +16,23 @@ jobs: fail-fast: false matrix: php: ['8.0', '8.1', '8.2', '8.3'] - os: ['debian', 'alpine'] - container: - image: ghcr.io/open-telemetry/opentelemetry-php-instrumentation/php:${{ matrix.php }}-${{ matrix.os }}-debug steps: - - uses: actions/checkout@v4 - - name: Build - run: | - phpize - ./configure - make - - name: Test - env: - TEST_PHP_ARGS: "-q" #do not try to submit failures - run: make test TESTS=--show-diff + - name: Checkout + uses: actions/checkout@v4 + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + - name: Build + run: | + phpize + ./configure + make + - name: Test + env: + TEST_PHP_ARGS: "-q" #do not try to submit failures + run: make test TESTS=--show-diff macos: runs-on: macos-latest diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml new file mode 100644 index 0000000..3966090 --- /dev/null +++ b/.github/workflows/nightly.yml @@ -0,0 +1,31 @@ +name: Build and test against PHP nightly + +on: + push: + pull_request: + branches: [ main ] + schedule: + - cron: '37 5 * * *' + +defaults: + run: + working-directory: ext + +jobs: + nightly: + if: github.repository == 'open-telemetry/opentelemetry-php-instrumentation' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: shivammathur/setup-php@v2 + with: + php-version: 8.4 + - name: Build + run: | + phpize + ./configure + make + - name: Test + env: + TEST_PHP_ARGS: "-q" + run: make test TESTS=--show-diff From 8eca255a4f09476ddca0eaaabcacc7957d429495 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Sat, 7 Sep 2024 17:34:57 +1000 Subject: [PATCH 2/8] fix 8.4 segfaults --- ext/otel_observer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ext/otel_observer.c b/ext/otel_observer.c index b3138d1..a10e97c 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -9,6 +9,7 @@ #include "php_opentelemetry.h" static int op_array_extension = -1; +static int internal_function_extension = -1; const char *withspan_fqn_lc = "opentelemetry\\api\\instrumentation\\withspan"; const char *spanattribute_fqn_lc = @@ -1149,5 +1150,7 @@ void opentelemetry_observer_init(INIT_FUNC_ARGS) { zend_observer_fcall_register(observer_fcall_init); op_array_extension = zend_get_op_array_extension_handle("opentelemetry"); + internal_function_extension = + zend_get_internal_function_extension_handle("opentelemetry"); } } From f32e6d0ee14b281cdc0611437553edcda7fe0a53 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Sat, 7 Sep 2024 17:43:26 +1000 Subject: [PATCH 3/8] conditionally call function --- ext/otel_observer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ext/otel_observer.c b/ext/otel_observer.c index a10e97c..43b5117 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -9,7 +9,6 @@ #include "php_opentelemetry.h" static int op_array_extension = -1; -static int internal_function_extension = -1; const char *withspan_fqn_lc = "opentelemetry\\api\\instrumentation\\withspan"; const char *spanattribute_fqn_lc = @@ -1150,7 +1149,8 @@ void opentelemetry_observer_init(INIT_FUNC_ARGS) { zend_observer_fcall_register(observer_fcall_init); op_array_extension = zend_get_op_array_extension_handle("opentelemetry"); - internal_function_extension = - zend_get_internal_function_extension_handle("opentelemetry"); + #if PHP_VERSION_ID >= 80400 + zend_get_internal_function_extension_handle("opentelemetry"); + #endif } } From e883528220048f1ae33ef22ca90072d9685bb9fa Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Sat, 7 Sep 2024 17:59:18 +1000 Subject: [PATCH 4/8] format --- ext/otel_observer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/otel_observer.c b/ext/otel_observer.c index 43b5117..d467b0d 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -1149,8 +1149,8 @@ void opentelemetry_observer_init(INIT_FUNC_ARGS) { zend_observer_fcall_register(observer_fcall_init); op_array_extension = zend_get_op_array_extension_handle("opentelemetry"); - #if PHP_VERSION_ID >= 80400 +#if PHP_VERSION_ID >= 80400 zend_get_internal_function_extension_handle("opentelemetry"); - #endif +#endif } } From 219af9b10b006025e2f4d66f7d2199d5e4c9ef72 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Sun, 8 Sep 2024 20:22:44 +1000 Subject: [PATCH 5/8] fix 32bit tests - remove the ability to pass complex items as attributes (since the spec already disallows them, they'd get dropped later anyway) - update alpine dockerfile to also support 32bit builds, which enables local testing against a 32bit platform --- docker-compose.yaml | 11 ++++++++++ docker/Dockerfile.alpine | 19 ++++++++++-------- ext/otel_observer.c | 19 +++++++++++++++++- .../function_params_non_simple.phpt | 20 ++++++------------- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index adce8b8..821718f 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -19,3 +19,14 @@ services: - ./ext:/usr/src/myapp environment: TEST_PHP_ARGS: "-q" + 32bit: + build: + context: docker + dockerfile: Dockerfile.alpine + args: + ALPINE_VERSION: i386/alpine + PHP_VERSION: ${PHP_VERSION:-8.4.0beta4} + volumes: + - ./ext:/usr/src/myapp + environment: + TEST_PHP_ARGS: "-q" diff --git a/docker/Dockerfile.alpine b/docker/Dockerfile.alpine index 983a6de..89c23d6 100644 --- a/docker/Dockerfile.alpine +++ b/docker/Dockerfile.alpine @@ -1,4 +1,5 @@ -FROM alpine:3.16 as builder +ARG ALPINE_VERSION=alpine:3.16 +FROM ${ALPINE_VERSION} as builder WORKDIR /usr/src ENV PHPIZE_DEPS \ @@ -22,23 +23,25 @@ RUN apk add --no-cache \ xz RUN apk add --no-cache \ + bison \ coreutils \ curl-dev \ libxml2-dev \ linux-headers \ + re2c \ readline-dev \ sqlite-dev ARG PHP_VERSION -ENV PHP_URL="https://www.php.net/distributions/php-${PHP_VERSION}.tar.xz" - -RUN echo "$PHP_URL" && curl -fsSL -o php.tar.xz "$PHP_URL" -RUN cd /usr/src \ - && tar -xf php.tar.xz +ENV PHP_URL="https://github.com/php/php-src/archive/refs/tags/php-${PHP_VERSION}.tar.gz" ARG PHP_CONFIG_OPTS="--enable-debug --with-pear --with-zlib" -RUN cd php-${PHP_VERSION} \ - && ./buildconf \ +RUN echo "$PHP_URL" && curl -fsSL -o php.tar.gz "$PHP_URL" \ + && cd /usr/src \ + && mkdir php-src \ + && tar -xzf php.tar.gz -C php-src --strip-components=1 \ + && cd php-src \ + && ./buildconf --force \ && ./configure ${PHP_CONFIG_OPTS} \ && make -j $(nproc) \ && make install diff --git a/ext/otel_observer.c b/ext/otel_observer.c index d467b0d..e2b6efe 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -152,6 +152,23 @@ static bool func_has_withspan_attribute(zend_execute_data *ex) { return attr != NULL; } +/* + * OpenTelemetry attribute values may only be of limited types + */ +static bool is_valid_attribute_value(zval *val) { + switch (Z_TYPE_P(val)) { + case IS_STRING: + case IS_LONG: // Numeric (integer) + case IS_DOUBLE: // Numeric (floating point) + case IS_TRUE: + case IS_FALSE: // Boolean + case IS_ARRAY: + return true; + default: + return false; + } +} + // get function args. any args with the // SpanAttributes attribute are added to the attributes HashTable static void func_get_args(zval *zv, HashTable *attributes, @@ -198,7 +215,7 @@ static void func_get_args(zval *zv, HashTable *attributes, zend_string *arg_name = ex->func->op_array.vars[i]; zend_attribute *attribute = find_spanattribute_attribute(ex->func, i); - if (attribute != NULL) { + if (attribute != NULL && is_valid_attribute_value(p)) { if (attribute->argc) { zend_string *key = Z_STR(attribute->args[0].value); zend_hash_del(attributes, key); diff --git a/ext/tests/span_attribute/function_params_non_simple.phpt b/ext/tests/span_attribute/function_params_non_simple.phpt index f637ab1..fc53f51 100644 --- a/ext/tests/span_attribute/function_params_non_simple.phpt +++ b/ext/tests/span_attribute/function_params_non_simple.phpt @@ -1,5 +1,5 @@ --TEST-- -Check if function non-simple types can be passed as function params +Check if function non-simple types are ignored --SKIPIF-- = 8.1'); ?> --EXTENSIONS-- @@ -28,28 +28,20 @@ function foo( } foo( - ['foo' => 'bar'], - new \stdClass(), - function(){return 'fn';}, - null, + one: ['foo' => 'bar'], + two: new \stdClass(), + three: function(){return 'fn';}, + four: null, ); ?> --EXPECTF-- string(3) "pre" -array(4) { +array(1) { ["one"]=> array(1) { ["foo"]=> string(3) "bar" } - ["two"]=> - object(stdClass)#1 (0) { - } - ["three"]=> - object(Closure)#2 (%d) {%A - } - ["four"]=> - NULL } string(3) "foo" string(4) "post" \ No newline at end of file From f3718ac7e7c984c1a4616d257ba8b66d7f4b07cc Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Sun, 8 Sep 2024 20:27:22 +1000 Subject: [PATCH 6/8] format --- ext/otel_observer.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ext/otel_observer.c b/ext/otel_observer.c index e2b6efe..080db36 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -157,15 +157,15 @@ static bool func_has_withspan_attribute(zend_execute_data *ex) { */ static bool is_valid_attribute_value(zval *val) { switch (Z_TYPE_P(val)) { - case IS_STRING: - case IS_LONG: // Numeric (integer) - case IS_DOUBLE: // Numeric (floating point) - case IS_TRUE: - case IS_FALSE: // Boolean - case IS_ARRAY: - return true; - default: - return false; + case IS_STRING: + case IS_LONG: // Numeric (integer) + case IS_DOUBLE: // Numeric (floating point) + case IS_TRUE: + case IS_FALSE: // Boolean + case IS_ARRAY: + return true; + default: + return false; } } From 12cbaa4b42543f79c2e127d2ddc182c4632eaf9a Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Mon, 9 Sep 2024 08:35:12 +1000 Subject: [PATCH 7/8] update local build to use alpine 3.20 --- docker/Dockerfile.alpine | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/Dockerfile.alpine b/docker/Dockerfile.alpine index 89c23d6..8c49116 100644 --- a/docker/Dockerfile.alpine +++ b/docker/Dockerfile.alpine @@ -1,4 +1,4 @@ -ARG ALPINE_VERSION=alpine:3.16 +ARG ALPINE_VERSION=alpine:3.20 FROM ${ALPINE_VERSION} as builder WORKDIR /usr/src From dac691b8b7877a16a8413114892fdbc5bff26911 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Mon, 9 Sep 2024 09:11:43 +1000 Subject: [PATCH 8/8] remove comments --- ext/otel_observer.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ext/otel_observer.c b/ext/otel_observer.c index 080db36..ff24b93 100644 --- a/ext/otel_observer.c +++ b/ext/otel_observer.c @@ -158,10 +158,10 @@ static bool func_has_withspan_attribute(zend_execute_data *ex) { static bool is_valid_attribute_value(zval *val) { switch (Z_TYPE_P(val)) { case IS_STRING: - case IS_LONG: // Numeric (integer) - case IS_DOUBLE: // Numeric (floating point) + case IS_LONG: + case IS_DOUBLE: case IS_TRUE: - case IS_FALSE: // Boolean + case IS_FALSE: case IS_ARRAY: return true; default: