Codex/implement php apm with c for linux gek86k by omerfdmrl · Pull Request #5 · omerfdmrl/php-apm

omerfdmrl · 2026-03-25T08:15:18Z

No description provided.

omerfdmrl · 2026-03-25T08:22:12Z

@@ -0,0 +1 @@
+config.m4


[INFO] Adding a .aignore entry for config.m4 will prevent this file from being processed by tools that respect .aignore. Ensure that config.m4 does not contain any required configuration for the build or deployment, as it will now be ignored.

omerfdmrl · 2026-03-25T08:22:38Z

+- **Native C extension** for low-level telemetry (CPU, wall time, memory, sampling, errors/exceptions).
+- **PHP userland layer** that converts native telemetry to OpenTelemetry traces and metrics.
+
+The extension targets **PHP 8.1+** on Linux and uses **CLOCK_THREAD_CPUTIME_ID** for CPU and **CLOCK_MONOTONIC** for wall time.


[SUGGESTION] Consider clarifying platform support: the extension currently targets Linux; note whether Windows/macOS are supported or if there are plans for cross‑platform builds.

omerfdmrl · 2026-03-25T08:22:46Z

+```ini
+extension=apm
+apm.enabled=1
+apm.enable_sampling=1


[SUGGESTION] The apm.sample_interval_us setting is in microseconds; consider adding a note about the unit and typical values to help users configure it correctly.

omerfdmrl · 2026-03-25T08:23:02Z

@@ -0,0 +1,7 @@
+PHP_ARG_ENABLE(apm, whether to enable apm support,


[ERROR] The repository's .aignore now lists config.m4, which means this file will be ignored by tools that respect .aignore. As a result, the PHP extension may not be configured or built correctly. Consider removing config.m4 from .aignore or adjusting the ignore rules.

omerfdmrl · 2026-03-25T08:23:31Z

+use OpenTelemetry\SDK\Trace\SpanExporter\OtlpHttpExporter;
+use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
+
+require __DIR__ . '/vendor/autoload.php';


[SUGGESTION] The script assumes a vendor/autoload.php exists relative to this file. In many setups the example may be run from a different working directory, causing a fatal error. Consider using an absolute path or checking file existence before requiring.

omerfdmrl · 2026-03-25T08:23:44Z

+require __DIR__ . '/vendor/autoload.php';
+
+$resource = ResourceInfo::create(['service.name' => 'php-apm-service']);
+$exporter = new OtlpHttpExporter('http://localhost:4318/v1/traces');


[SUGGESTION] The OtlpHttpExporter URL is hard‑coded to localhost:4318. In production this will likely need to be configurable via environment variable or config file.

omerfdmrl · 2026-03-25T08:25:43Z

+#define PHP_APM_H
+
+#include <stdint.h>
+#include <stdio.h>


[SUGGESTION] The <stdio.h> header is included but not used anywhere in this file. Consider removing it to reduce compilation overhead and avoid unnecessary dependencies.

omerfdmrl · 2026-03-25T08:28:44Z

@@ -0,0 +1 @@
+config.m4


[INFO] Adding .aignore to block config.m4 is fine. Ensure that this pattern does not unintentionally prevent needed configuration files from being accessed in the future.

omerfdmrl · 2026-03-25T08:28:49Z

[INFO] The addition of .aignore to exclude config.m4 seems appropriate. Verify that the build or deployment processes respect .aignore and that no required files are inadvertently ignored.

omerfdmrl · 2026-03-25T08:29:00Z

[INFO] The new README provides a clear overview of the hybrid PHP APM agent, build instructions, and configuration details. Consider adding a brief licensing notice and a link to contribution guidelines for completeness, but otherwise the documentation looks solid.

omerfdmrl · 2026-03-25T08:29:08Z

+
+- Sampling is enabled by default; function-level timing is gated to avoid overhead.
+- Stack samples are captured in a ring buffer and flushed in userland.
+- Flamegraphs can be generated from sampled stacks (folded stack format).


[SUGGESTION] Consider adding a License section (e.g., MIT) at the end of the README to clarify usage rights.

omerfdmrl · 2026-03-25T08:29:22Z

@@ -0,0 +1,7 @@
+PHP_ARG_ENABLE(apm, whether to enable apm support,
+[  --enable-apm           Enable apm support])
+


[INFO] The description for the --enable-apm flag is concise, but consider adding a brief note about the default behavior (disabled) for clarity to users.

omerfdmrl · 2026-03-25T08:29:36Z

@@ -0,0 +1,7 @@
+PHP_ARG_ENABLE(apm, whether to enable apm support,


[INFO] Consider specifying the default value (e.g., "no") in PHP_ARG_ENABLE to make the default behavior explicit for users.

omerfdmrl · 2026-03-25T08:30:04Z

+require __DIR__ . '/vendor/autoload.php';
+
+$resource = ResourceInfo::create(['service.name' => 'php-apm-service']);
+$exporter = new OtlpHttpExporter('http://localhost:4318/v1/traces');


[SUGGESTION] The OTLP endpoint URL is hard‑coded. Consider making it configurable via an environment variable or configuration file to allow different deployment environments.

omerfdmrl · 2026-03-25T08:30:17Z

+
+require __DIR__ . '/vendor/autoload.php';
+
+$resource = ResourceInfo::create(['service.name' => 'php-apm-service']);


[SUGGESTION] The service name is hard‑coded. Consider making it configurable (e.g., via env var) so the same code can be reused across services.

omerfdmrl · 2026-03-25T08:30:26Z

+    $span->recordException($e);
+    $span->setStatus(StatusCode::STATUS_ERROR, $e->getMessage());
+} finally {
+    $metrics = apm_get_request_metrics();


[WARNING] Ensure the APM extension is loaded before calling apm_get_request_metrics() etc., otherwise this will cause a fatal error if the extension is missing.

omerfdmrl · 2026-03-25T09:01:40Z

[INFO] Added .aignore to exclude config.m4 from repository scans. This helps keep sensitive configuration files out of version control and complies with the project's ignore policies.

omerfdmrl · 2026-03-25T09:02:00Z

+- **Native C extension** for low-level telemetry (CPU, wall time, memory, sampling, errors/exceptions).
+- **PHP userland layer** that converts native telemetry to OpenTelemetry traces and metrics.
+
+The extension targets **PHP 8.1+** on Linux and uses **CLOCK_THREAD_CPUTIME_ID** for CPU and **CLOCK_MONOTONIC** for wall time.


[SUGGESTION] The extension currently targets Linux only. It would be helpful to note any platform limitations or future plans for macOS/Windows support.

omerfdmrl · 2026-03-25T09:02:07Z

+
+```sh
+phpize
+./configure --enable-apm


[SUGGESTION] Consider adding a comment that the value is in microseconds to avoid confusion for users adjusting the sampling interval.

omerfdmrl · 2026-03-25T09:02:17Z

+extension=apm
+apm.enabled=1
+apm.enable_sampling=1
+apm.sample_interval_us=10000


[SUGGESTION] Consider adding a comment that the value is in microseconds to avoid confusion for users adjusting the sampling interval.

omerfdmrl · 2026-03-25T09:02:59Z

+use OpenTelemetry\SDK\Trace\SpanExporter\OtlpHttpExporter;
+use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
+
+require __DIR__ . '/vendor/autoload.php';


[SUGGESTION] Consider checking that the autoload file exists before requiring it to avoid a fatal error when the example is run outside of a Composer-installed environment.

omerfdmrl · 2026-03-25T09:03:13Z

+require __DIR__ . '/vendor/autoload.php';
+
+$resource = ResourceInfo::create(['service.name' => 'php-apm-service']);
+$exporter = new OtlpHttpExporter('http://localhost:4318/v1/traces');


[SUGGESTION] The OTLP endpoint URL is hard‑coded. Consider making it configurable via an environment variable or configuration file to allow different deployment environments.

omerfdmrl · 2026-03-25T09:04:27Z

+
+static uint32_t apm_ring_next(uint32_t value, uint32_t capacity)
+{
+    value++;


[WARNING] The apm_ring_next function assumes capacity is non-zero. If APM_G(buffer_size) were set to 0, this could cause a division by zero or infinite loop. Consider adding a guard to handle zero capacity gracefully.

omerfdmrl · 2026-03-25T09:04:59Z

+    return apm_clock_now(CLOCK_THREAD_CPUTIME_ID);
+}
+
+static void apm_log(const char *format, ...)


[SUGGESTION] apm_log logs every call as E_NOTICE which may flood logs in production. Consider using a lower log level or making logging configurable.

omerfdmrl · 2026-03-25T09:05:36Z

+    apm_ring_push_sample(&entry, &cpu_point);
+}
+
+static void apm_sampler_signal_handler(int signo)


[CRITICAL] The signal handler apm_sampler_signal_handler invokes apm_capture_sample, which uses many PHP runtime functions (e.g., zend hash operations, memory allocation). These are not async-signal-safe and can cause undefined behavior or crashes. Consider redesigning sampling to use a safe flag set in the handler and perform the capture in the main request loop.

omerfdmrl · 2026-03-25T09:10:35Z

@@ -0,0 +1,59 @@
+# PHP APM (Hybrid C + PHP OpenTelemetry)


[SUGGESTION] Consider adding a brief "License" section to clarify the project's licensing terms for downstream users.

omerfdmrl · 2026-03-25T09:10:43Z

+## Build and install
+
+```sh
+phpize


[SUGGESTION] Consider adding a note about required build dependencies (e.g., php-dev, autoconf, libtool) before running phpize and configure, to help users set up the environment correctly.

omerfdmrl · 2026-03-25T09:11:34Z

+require __DIR__ . '/vendor/autoload.php';
+
+$resource = ResourceInfo::create(['service.name' => 'php-apm-service']);
+$exporter = new OtlpHttpExporter('http://localhost:4318/v1/traces');


[SUGGESTION] Consider reading the OTLP endpoint from an environment variable (e.g., OTEL_EXPORTER_OTLP_ENDPOINT) instead of hard‑coding http://localhost:4318/v1/traces. This makes the example more flexible for different deployment environments.

omerfdmrl · 2026-03-25T09:11:50Z

+use OpenTelemetry\SDK\Trace\SpanExporter\OtlpHttpExporter;
+use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
+
+require __DIR__ . '/vendor/autoload.php';


[SUGGESTION] The autoload path assumes a vendor directory next to this script. Consider using Composer's autoload via require __DIR__.'/../vendor/autoload.php'; or checking for its existence to avoid runtime errors when the example is run from a different location.

omerfdmrl · 2026-03-25T09:12:57Z

+{
+    apm_globals->enabled = 1;
+    apm_globals->enable_sampling = 1;
+    apm_globals->enable_function_timing = 0;


[WARNING] Consider validating that apm.sample_interval_us is greater than zero. A zero interval would create a timer with no delay, potentially causing a busy loop and high CPU usage.

omerfdmrl · 2026-03-25T09:17:56Z

[INFO] The PR adds a new .aignore file that blocks config.m4. Ensure that this file is not required for the build or runtime, as it will now be ignored by tooling that respects .aignore.

omerfdmrl · 2026-03-25T09:18:07Z

+
+This repository provides a production-grade **hybrid** PHP APM agent:
+
+- **Native C extension** for low-level telemetry (CPU, wall time, memory, sampling, errors/exceptions).


[SUGGESTION] Consider adding a brief explanation of what "hybrid" means in this context (i.e., combination of native C extension and PHP userland) for readers unfamiliar with the term.

omerfdmrl · 2026-03-25T09:18:27Z

+use OpenTelemetry\SDK\Trace\SpanExporter\OtlpHttpExporter;
+use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
+
+require __DIR__ . '/vendor/autoload.php';


[SUGGESTION] The script assumes a vendor/autoload.php exists relative to this file. Ensure the example includes installation instructions or uses a more robust autoload path to avoid runtime errors.

omerfdmrl · 2026-03-25T09:19:53Z

+
+ZEND_DECLARE_MODULE_GLOBALS(apm)
+
+static uint64_t apm_clock_now(clockid_t clock_id)


[WARNING] The apm_clock_now function returns 0 on clock_gettime failure, which is indistinguishable from a valid timestamp of 0. Consider returning an error indicator or logging the failure to avoid silent incorrect timing data.

omerfdmrl · 2026-03-25T09:20:30Z

+    }
+}
+
+static void apm_ring_init(uint32_t capacity)


[WARNING] apm_ring_init should validate that capacity is greater than zero before allocating memory. A zero capacity would allocate zero‑length buffers and later cause dereferencing NULL pointers in apm_ring_push_sample. Consider adding a guard and fallback to a safe default size.

omerfdmrl · 2026-03-25T09:21:09Z

+    apm_globals->enabled = 1;
+    apm_globals->enable_sampling = 1;
+    apm_globals->enable_function_timing = 0;
+    apm_globals->sample_interval_us = 10000;


[WARNING] The INI setting apm.buffer_size is assigned directly without validation. If a user configures a zero or excessively large value, memory allocation in apm_ring_init may fail or allocate zero‑length buffers, leading to crashes. Consider adding a sanity check (e.g., enforce a minimum of 1 and a reasonable maximum).

omerfdmrl · 2026-03-25T09:21:34Z

+    APM_G(ring).head = 0;
+    APM_G(ring).tail = 0;
+    APM_G(ring).dropped = 0;
+    APM_G(ring).samples = ecalloc(capacity, sizeof(apm_sample_entry));


[WARNING] Memory allocation with ecalloc for samples and cpu_points is not checked for failure. If allocation returns NULL, subsequent dereferences will segfault. Consider checking the return value and handling OOM gracefully.

omerfdmrl · 2026-03-25T09:22:49Z

+    zend_bool enabled;
+    zend_bool enable_sampling;
+    zend_bool enable_function_timing;
+    zend_long sample_interval_us;


[WARNING] sample_interval_us is defined as zend_long (signed). Since it represents a duration in microseconds, a negative value would be nonsensical and could cause timer calculations to behave incorrectly. Consider using an unsigned type (e.g., zend_ulong or uint64_t) and validate the value in the INI handler.

omerfdmrl added 2 commits February 2, 2026 23:31

Implement hybrid APM agent with sampling

6865b80

Add .aignore file to ignore config.m4

33a2c12

omerfdmrl commented Mar 25, 2026

View reviewed changes

		@@ -0,0 +1,7 @@
		PHP_ARG_ENABLE(apm, whether to enable apm support,

		@@ -0,0 +1,7 @@
		PHP_ARG_ENABLE(apm, whether to enable apm support,
		[ --enable-apm Enable apm support])


		require __DIR__ . '/vendor/autoload.php';

		$resource = ResourceInfo::create(['service.name' => 'php-apm-service']);


		This repository provides a production-grade hybrid PHP APM agent:

		- Native C extension for low-level telemetry (CPU, wall time, memory, sampling, errors/exceptions).


		ZEND_DECLARE_MODULE_GLOBALS(apm)

		static uint64_t apm_clock_now(clockid_t clock_id)

		@@ -0,0 +1 @@
		config.m4

		@@ -0,0 +1 @@
		config.m4

Conversation

omerfdmrl commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omerfdmrl commented Mar 25, 2026

Uh oh!

omerfdmrl commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omerfdmrl commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omerfdmrl commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers