Adding magento2 instrumentation by jerrytfleung · Pull Request #570 · open-telemetry/opentelemetry-php-contrib

jerrytfleung · 2026-05-07T02:49:53Z

This pull request introduces initial support for Magento 2 instrumentation in the project. The main changes add the necessary configuration, registration, and repository split logic to treat Magento 2 as a first-class instrumentation target, alongside updates to CI and code quality tooling to support the new integration.

Magento 2 Instrumentation Integration:

Added src/Instrumentation/Magento2/ directory with configuration files for code style (.php-cs-fixer.php), static analysis (.phan/config.php), .gitignore, and .gitattributes. [1] [2] [3] [4]

Composer and Autoloading Updates:

Updated composer.json to:
- Register the Magento 2 source and test namespaces for autoloading and test discovery. [1] [2]
- Include the Magento 2 _register.php file for instrumentation registration.
- Add open-telemetry/opentelemetry-auto-magento2 as a dependency.

Repository and CI Configuration:

Updated .gitsplit.yml to enable splitting the Magento 2 instrumentation into its own repository.
Updated .github/workflows/php.yml to:
- Include Magento 2 in the matrix for CI jobs.
- Test Magento 2 instrumentation with PHP 8.1 and 8.2. [1] [2]

Require the merge of open-telemetry/opentelemetry-php#1952 to provide the necessary prerequisite for the GitHub action of magento instrumentation

Nevay · 2026-05-07T19:15:22Z

+                if ($span instanceof ReadableSpanInterface) {
+                    $spanAttrs = [
+                        HttpAttributes::HTTP_REQUEST_METHOD => $span->getAttribute(HttpAttributes::HTTP_REQUEST_METHOD),
+                        UrlAttributes::URL_SCHEME => $span->getAttribute(UrlAttributes::URL_SCHEME),
+                        NetworkAttributes::NETWORK_PROTOCOL_VERSION => $span->getAttribute(NetworkAttributes::NETWORK_PROTOCOL_VERSION),
+                    ];
+                    /** @psalm-suppress PossiblyInvalidArgument */
+                    $histogram->record($span->getDuration() / ClockInterface::NANOS_PER_SECOND, array_merge($spanAttrs, $responseMeta));
+                }


Should not read values from the span, see https://opentelemetry.io/docs/specs/otel/trace/sdk/#additional-span-interfaces:

The API-level definition for Span’s interface only defines write-only access to the span. This is good because instrumentations and applications are not meant to use the data stored in a span for application logic.

Additionally this won't record metrics if the span is non-recording.

I see. Changed to use the scope to save the $requestMeta.

Yes, the pre/post hook is only triggered when the span is recording.
I am trying to create http.server.request.duration histogram metrics similar to the one in https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/ReactPHP/src/ReactPHPInstrumentation.php#L185

Nevay · 2026-05-07T19:18:46Z

+
+                /** @psalm-suppress MixedAssignment */
+                foreach ($request->getHeaders() as $key => $value) {
+                    $spanBuilder->setAttribute(HttpAttributes::HTTP_REQUEST_HEADER . '.' . $key, implode(',', $value));


https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/#opt-in

Instrumentation that doesn’t support configuration MUST NOT populate Opt-In attributes.

I see. Removed all opt-in attributes.

Nevay · 2026-05-20T19:00:34Z

+                    ->setAttribute(UrlAttributes::URL_FULL, (string) $request->getUri())
+                    ->setAttribute(UrlAttributes::URL_SCHEME, $request->getUri()->getScheme())
+                    ->setAttribute(UrlAttributes::URL_PATH, $request->getUri()->getPath())
+                    ->setAttribute(HttpAttributes::HTTP_REQUEST_METHOD, $request->getMethod())


Must set unknown request methods to _OTHER.

Nevay · 2026-05-20T19:07:43Z

+                    ->setAttribute(ServerAttributes::SERVER_ADDRESS, $request->getUri()->getHost())
+                    ->setAttribute(ServerAttributes::SERVER_PORT, $request->getUri()->getPort() ?? ($request->getUri()->getScheme() === 'https' ? 443 : 80));


Should attempt to set the server address and port according to https://opentelemetry.io/docs/specs/semconv/http/http-spans/#setting-serveraddress-and-serverport-attributes.

Nevay · 2026-05-20T19:09:36Z

+            'launch',
+            pre: static function (Http $http, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) {
+                $factory = new Psr17Factory();
+                $request = (new ServerRequestCreator($factory, $factory, $factory, $factory))->fromGlobals();


Could this reuse / share the implementation with the existing PSR-15 instrumentation? The code doesn't seem to contain Magento specific parts.

I am not sure how to reuse / share those implementation where instrumentation libraries will be finally git split into individual library in packagist.org.
Can you please help to elaborate more on this point? Thanks!

Nevay · 2026-05-20T19:25:02Z

+                }
+                if ($response instanceof HttpResponse) {
+                    $span->setAttribute(HttpAttributes::HTTP_RESPONSE_STATUS_CODE, $response->getStatusCode());
+                    $responseMeta[HttpAttributes::HTTP_RESPONSE_STATUS_CODE] = $response->getStatusCode();


Should set error.type if status code is 5xx.

Sure. Fixed the error.type if the status code is 5xx.

Co-authored-by: Tobias Bachert <github.dev.k9ps1@mail.tobiasbachert.com>

bobstrecansky · 2026-05-21T13:00:29Z

+  "readme": "./README.md",
+  "license": "Apache-2.0",
+  "require": {
+    "php": "^8.3",


Blocker — PHP version conflict with CI matrix. This package requires ^8.3, but .github/workflows/php.yml (lines 92–95) adds matrix entries pinning Magento2 to PHP 8.1 and 8.2. Composer install will fail on both rows, so those CI jobs cannot pass as written.

Every other instrumentation in this repo uses ^8.1. Either:

Relax to "php": "^8.1" to match the rest of the repo, or

Drop the 8.1/8.2 include entries in php.yml (the default matrix already covers 8.1–8.4, so you only need include: entries if you want to exclude versions).

Also, if you stay on ^8.3, update .phan/config.php target_php_version accordingly. If you relax to ^8.1, also drop the #[Override] attributes (they're PHP 8.3-only).

magento/community-edition@dev-2.4-develop supports 8.3+. So I set the PHP require version to be ^8.3 and also set the exclude 8.1 & 8.2 in php.yml.

bobstrecansky · 2026-05-21T13:00:43Z

+    "php": "^8.3",
+    "ext-opentelemetry": "*",
+    "ext-json": "*",
+    "open-telemetry/api": "^1.6",


Blocker — sem-conv version incompatible with the rest of the repo. Every other instrumentation pins open-telemetry/sem-conv: ^1.30. The Attributes\\* split namespace that this instrumentation uses heavily was introduced in sem-conv 1.32+, so requiring ^1.36 here will cause autoload failures for consumers of the root contrib package that resolve to ^1.30.

Either:

Drop to ^1.30 and switch to the flat TraceAttributes constants (matches every other instrumentation), or

Coordinate a repo-wide bump of sem-conv before merging this PR.

The PR description mentions a prerequisite PR on opentelemetry-php — please make the dependency relationship explicit, and confirm whether a coordinated bump across the contrib monorepo is in scope here.

Different instrumentation libraries are using different sem-conv version now. I am not sure why it is a blocker.

^1.38 - https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Laravel/composer.json#L24
^1.36 - https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/PDO/composer.json#L16, https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Slim/composer.json#L16, https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/MongoDB/composer.json#L15
^1.32 - https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Psr15/composer.json#L15, https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Psr6/composer.json#L15, https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/ReactPHP/composer.json#L16
^1.30 - https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/PostgreSql/composer.json#L24

The prerequisite PR in opentelemetry-php is just the extension required in the opentelemetry-php-base for Magento framework. It is already merged and the image is published successfully.

bobstrecansky · 2026-05-21T13:00:55Z

+    "phpunit/phpunit": "^9.5",
+    "vimeo/psalm": "^7.0",
+    "magento/community-edition": "dev-2.4-develop"
+  },


Should fix — magento/community-edition: dev-2.4-develop in require-dev is very heavy. This is a 200+ MB metapackage that pulls in the entire Magento storefront, admin, modules, and front-end build chain. CI install time will be dominated by this.

The unit tests in this PR all use getMockBuilder() / TestFramework\Unit\Helper\ObjectManager and don't appear to need a full Magento app fixture. If magento/framework (already in require) is sufficient to run the unit tests, please drop this. If it's needed for one specific fixture, narrow the dep to the specific module.

At minimum, add a comment explaining why the full edition is required, given the install cost.

Yes, it is heavy but necessary.
magento/framework is an internal library inside magento/community-edition and not public in packagist.org.
I put @Nevay's suggestion from this slack message

bobstrecansky · 2026-05-21T13:01:04Z

+          - project: 'Instrumentation/Magento2'
+            php-version: 8.1
+          - project: 'Instrumentation/Magento2'
+            php-version: 8.2


These matrix entries pin Magento2 to PHP 8.1 and 8.2, but src/Instrumentation/Magento2/composer.json requires "php": "^8.3" — composer install will fail on both rows. See the related comment on composer.json:10. Decide whether to relax the package constraint to ^8.1 or drop these matrix entries.

magento/community-edition@dev-2.4-develop requires ~8.3.0 || ~8.4.0 || ~8.5.0.
I put these lines in the matrix so that the action will exclude 8.1 & 8.2 for Instrumentation/Magento2.

bobstrecansky · 2026-05-21T13:01:21Z

+                    ->setAttribute(CodeAttributes::CODE_LINE_NUMBER, $lineno)
+                    ->setAttribute(UrlAttributes::URL_SCHEME, $request->getUri()->getScheme())
+                    ->setAttribute(UrlAttributes::URL_PATH, $request->getUri()->getPath())
+                    ->setAttribute(HttpAttributes::HTTP_REQUEST_METHOD, strlen($request->getMethod()) > 0 ? $request->getMethod() : '_OTHER')


Blocker — _OTHER semantics are wrong. The HTTP semconv says _OTHER must be used when the method is not in the standard set (GET/POST/PUT/DELETE/HEAD/OPTIONS/PATCH/TRACE/CONNECT), and the original method must be set on http.request.method_original. This strlen() > 0 check only handles empty strings — an arbitrary FOO method will be sent verbatim as the attribute value, producing high-cardinality data.

Suggested fix:

$knownMethods = ['GET','POST','PUT','DELETE','HEAD','OPTIONS','PATCH','TRACE','CONNECT']; $rawMethod = $request->getMethod(); $method = in_array($rawMethod, $knownMethods, true) ? $rawMethod : '_OTHER'; $spanBuilder->setAttribute(HttpAttributes::HTTP_REQUEST_METHOD, $method); if ($method === '_OTHER') { $spanBuilder->setAttribute(HttpAttributes::HTTP_REQUEST_METHOD_ORIGINAL, $rawMethod); }

IMPORTANT: the bucketed $method (not the raw value) must also flow into $requestMeta used by the duration histogram on line ~133, otherwise the metric retains the high-cardinality original. http.request.method_original must NOT appear on the metric (the server histogram has a fixed low-cardinality attribute set per semconv).

I see. I referred to https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/ReactPHP/src/ReactPHPInstrumentation.php#L289 to fix the HTTP_REQUEST_METHOD and added HTTP_REQUEST_METHOD_ORIGINAL

bobstrecansky · 2026-05-21T13:01:33Z

+                if ($exception) {
+                    $span->recordException($exception);
+                    $span->setStatus(StatusCode::STATUS_ERROR, $exception->getMessage());
+                    $responseMeta[ErrorAttributes::ERROR_TYPE] = (string) $exception->getCode();


Blocker — error.type is misused. Per the semantic conventions, error.type is a low-cardinality identifier — typically the exception class name. $exception->getCode() returns the exception's integer code, which is 0 for the vast majority of PHP/Magento exceptions, producing useless "0" metric labels and effectively breaking the request-duration histogram for the most common error case.

Fix:

$responseMeta[ErrorAttributes::ERROR_TYPE] = $exception::class;

Also: reviewer feedback earlier in the thread asked you to set error.type on 5xx responses too — that branch is still missing. Add an else-if for $statusCode >= 500 that sets error.type to the HTTP status (per the spec's HTTP error semantics).

I see. Fixed the error type in the $responseMeta and also fixed for the 5xx response.

bobstrecansky · 2026-05-21T13:01:52Z

+                $parent = Globals::propagator()->extract($request->getHeaders());
+
+                $spanBuilder = $instrumentation->tracer()
+                    ->spanBuilder(sprintf('%s %s', $request->getMethod(), self::getScriptNameFromRequest($request)))


Blocker — Http::launch will double-fire on every request due to Magento's Interceptor pattern. Magento generates an Http\Interceptor extends Http class for every plugin-able class, so this hook fires once for Http::launch() and once for Http\Interceptor::launch() — producing duplicate server root spans and double-counting the http.server.request.duration histogram.

Nevay flagged this earlier in the thread with a screenshot; the suggested guard is not in the current diff. Add to both pre and post callbacks at the very top:

if ($class !== Http::class) { return; }

(Without this guard on post:, you'll detach the wrong scope on the duplicate run.)

Please also verify against a real Magento install that none of the other hooks here double-fire — most target interfaces/abstract classes where the interceptor model behaves differently, but it's worth confirming.

Thanks! Added the Http::launch guard in the pre/post hook.

In fact, other classes such as FrontController, Action, ActionInterface, Manager... also have this double-fire behavior due to the magento interceptor plugin. From magento's interceptor plugin limitation, it basically intercepts many classes.

The guard works well for concrete class like Http::launch, but it doesn't work well with abstract classes / interface like Action and ActionInterface because they are not the actual class calling in the framework.

For those class, as they are internal spans so I am not super worried. I think recommending user to disable interceptor plugin could be a way to handle.

@bobstrecansky & @Nevay What do you think?

bobstrecansky · 2026-05-21T13:02:13Z

+use Magento\Framework\Event\InvokerInterface;
+use Magento\Framework\Event\Manager;
+use Magento\Framework\View\Element\Template;
+use Nyholm\Psr7Server\ServerRequestCreator;


Blocker — undeclared runtime dependencies on nyholm/psr7 and nyholm/psr7-server. This file imports Nyholm\Psr7Server\ServerRequestCreator (here) and Http\Discovery\Psr17Factory (line 7), but neither package is declared in composer.json. Compare to the Wordpress instrumentation which explicitly declares:

"nyholm/psr7": "^1", "nyholm/psr7-server": "^1"

Also recommend switching from Http\Discovery\Psr17Factory to Nyholm\Psr7\Factory\Psr17Factory (which Wordpress uses): the discovery variant adds a php-http/discovery runtime dep with no functional benefit, and your composer.json already disables php-http/discovery in allow-plugins (which means it can't auto-register anyway).

Without these deps declared, the package will only work on hosts that happen to install them transitively.

Sure. Switched to Nyholm\Psr7\Factory\Psr17Factory.

bobstrecansky · 2026-05-21T13:02:25Z

+            pre: static function (Manager $manager, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) {
+                $eventName = (string) ($params[0]);
+                $span = $instrumentation->tracer()
+                    ->spanBuilder('EVENT: ' . $eventName)


Should fix — span naming style. EVENT: (and OBSERVER: below) use uppercase ALL-CAPS prefixes that diverge from the lowercase namespacing every other OTel instrumentation uses (db.query, http.request, etc.). This makes Magento spans visually inconsistent in trace UIs.

Consider event.dispatch {eventName} and observer.dispatch {observerName}, or drop the prefix entirely and use a dedicated attribute like magento.event.name for filtering.

Also: $eventName = (string) ($params[0]) on line ~244 — if a third-party module passes a non-string, the cast silently produces "Array" or worse. Wrap with the same is_string && !== '' guard you use in InvokerInterface::dispatch.

Sure. Done.

bobstrecansky · 2026-05-21T13:02:38Z

+                    ->startSpan();
+                Context::storage()->attach($span->storeInContext(Context::getCurrent()));
+            },
+            post: static function (Manager $manager, array $params) {


Should fix — post-closure swallows exceptions. This signature is post: static function (Manager $manager, array $params) — it doesn't accept the ?Throwable $exception parameter, so if an event listener throws, the span ends without recordException() or STATUS_ERROR. The InvokerInterface::dispatch post-closure below has the same issue.

Every other post-hook in this file handles exceptions; please make these two consistent:

post: static function (Manager $manager, array $params, mixed $return, ?Throwable $exception) { $scope = Context::storage()->scope(); if (!$scope) { return; } $scope->detach(); $span = Span::fromContext($scope->context()); if ($exception) { $span->recordException($exception); $span->setStatus(StatusCode::STATUS_ERROR, $exception->getMessage()); } $span->end(); }

Thanks! I am not aware how to put the third parameter if the function returning void.

Fixed.

bobstrecansky · 2026-05-21T13:02:48Z

+                    ->startSpan();
+                Context::storage()->attach($span->storeInContext(Context::getCurrent()));
+            },
+            post: static function (InvokerInterface $invokerInterface, array $params) {


Same as the Manager::dispatch post-hook above — this signature is missing the ?Throwable $exception parameter, so observer exceptions go unrecorded. Please mirror the exception-handling pattern used in Http::launch, Action::dispatch, etc.

Fixed. Thanks!

bobstrecansky · 2026-05-21T13:03:00Z

+            pre: static function (Bootstrap $bootstrap, array $params, string $class, string $function, ?string $filename, ?int $lineno) use ($instrumentation) {
+                $exception = $params[0] instanceof Throwable ? $params[0] : null;
+                $span = $instrumentation->tracer()
+                    ->spanBuilder('Bootstrap::terminate')


Should fix — Bootstrap::terminate creates a span even on successful requests. Bootstrap::terminate(Throwable $e) is called on every request termination in Magento, not only on errors. The current code always creates a span even when $params[0] is null/non-throwable, producing a useless Bootstrap::terminate span sibling to Http::launch for every successful request.

Either gate span creation on $exception !== null, or consider whether this hook is needed at all — Http::launch already covers the full request lifecycle, including exceptions.

Boostrap::terminate will be called on exception in https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/App/Bootstrap.php#L244 & https://github.com/magento/magento2/blob/2.4-develop/lib/internal/Magento/Framework/App/Bootstrap.php#L275. I also verified there is no Boostrap::terminate span found on successful requests.

Can you help to point me where Bootstrap::terminate is called on every request termination in Magento? Thanks!

bobstrecansky · 2026-05-21T13:03:11Z

+/**
+ * Path to Composer vendor directory
+ */
+return './vendor';


Should fix — this file shouldn't ship in the package. app/etc/vendor_path.php is a Magento installation bootstrap file. Shipping it inside an OTel instrumentation package means anyone who installs opentelemetry-auto-magento2 gets an unexpected app/etc/ directory inside their vendor tree.

If you genuinely need this for tests, move it under tests/ and add tests to .gitattributes export-ignore (which it already is). Otherwise, please remove the file. .gitattributes currently doesn't export-ignore app/, so this will end up in the published archive.

@bobstrecansky I need to install the magento so as to fulfill the composer's dependency requirement.

If I remove the file app/etc/vendor_path.php and keep the app/etc directory, the vendor_path.php will be auto-generated when I run PROJECT=Instrumentation/Magento2 PHP_VERSION=8.4 make all

If I remove the file app/etc/vendor_path.php and the app/etc directory, the composer will complain

In Plugin.php line 323: file_put_contents(/usr/src/myapp/src/Instrumentation/Magento2/app/etc/vendor_path.php): Failed to open stream: No such file or directory

instead.

I removed the file app/etc/vendor_path.php and added app/etc/.gitkeep to keep the directories.

bobstrecansky · 2026-05-21T13:03:24Z

+
+    private static function getScriptNameFromRequest(ServerRequestInterface $request): string
+    {
+        $scriptName = $request->getServerParams()['SCRIPT_NAME'] ?? '/';


Consider — span naming has poor cardinality. Falling back to / and using SCRIPT_NAME (typically /index.php for every Magento request) means most spans end up named GET /index.php. Two suggestions:

Update the span name to include URL_PATH for better differentiation, or

Update the root span name to http.route after FrontController::dispatch resolves the route (this is the spec-recommended naming for HTTP server spans, see HTTP semconv §HTTP server semantics).

Sure. Applied the 1st suggestion.

bobstrecansky · 2026-05-21T13:03:35Z

+            }
+        );
+
+        /** @psalm-suppress DeprecatedClass */


Consider — reuse PSR-15 / shared HTTP server instrumentation. Nevay's earlier comment asked whether this could share implementation with the existing PSR-15 instrumentation, because the Http::launch hook is mostly generic HTTP server bookkeeping (propagator extract, request meta, status code mapping, duration histogram). The same logic is duplicated across Slim, Laravel, Wordpress, and now Magento2.

Not a blocker for this PR, but worth filing a follow-up issue for a shared OpenTelemetry\Contrib\Instrumentation\Common\PsrHttpServerInstrumentation — would be a real maintenance win.

Reusing the logic is great!

I am happy to help but I am not so sure what would a shared OpenTelemetry\Contrib\Instrumentation\Common\PsrHttpServerInstrumentation have.

As I know so far, the instrumentation libraries will be git split for individual composer library, they don't know each other.

@bobstrecansky @Nevay What do you think?

bobstrecansky · 2026-05-21T13:03:47Z

+
+        /** @psalm-suppress DeprecatedClass */
+        hook(
+            Action::class,


Should fix — add explicit SpanKind::KIND_INTERNAL for child spans. Only Http::launch sets KIND_SERVER; every other hook in this file implicitly defaults to KIND_INTERNAL. The convention in this repo (see SlimInstrumentation.php, WordpressInstrumentation.php) is to set the span kind explicitly with ->setSpanKind(SpanKind::KIND_INTERNAL) — it makes intent obvious and avoids any surprise if SDK defaults ever change. Please apply to FrontController::dispatch, Action::dispatch, ActionInterface::execute, Manager::dispatch, InvokerInterface::dispatch, Template::fetchView, View::renderLayout, and Bootstrap::terminate.

Sure. Added.

bobstrecansky · 2026-05-21T13:04:11Z

Review summary — request changes

Thanks for putting this together — adding Magento2 to the contrib monorepo is real work and the test coverage here is thorough. Below is an overall verdict; specific findings are posted as inline comments on the relevant lines.

Must fix (blockers)

PHP version conflict: composer.json requires ^8.3 but CI matrix pins 8.1/8.2 — composer install will fail. (inline on composer.json:10, php.yml:95)
error.type misuse: uses $exception->getCode() (almost always 0); should be $exception::class. Also still missing for 5xx. (inline on Magento2Instrumentation.php:132)
_OTHER semantic violation: only emitted for empty methods; spec says emit for any non-standard method and set http.request.method_original. Histogram attrs also need the bucketed value. (inline on Magento2Instrumentation.php:95)
Http::launch double-fires through Magento's Interceptor pattern (Nevay's earlier flag is unaddressed). Need if ($class !== Http::class) return; guard. (inline on Magento2Instrumentation.php:87)
open-telemetry/sem-conv: ^1.36 is incompatible with the repo baseline of ^1.30. (inline on composer.json:13)
Missing runtime deps: nyholm/psr7, nyholm/psr7-server. (inline on Magento2Instrumentation.php:21)
magento/community-edition in require-dev is a 200MB+ metapackage; unit tests use mocks and likely don't need it. (inline on composer.json:30)

Should fix

Manager::dispatch and InvokerInterface::dispatch post-closures don't accept ?Throwable $exception — listener errors are silently swallowed. (inline)
Bootstrap::terminate creates a span on every request, not just on exception. (inline)
app/etc/vendor_path.php shouldn't ship in the package; .gitattributes doesn't export-ignore app/. (inline)
EVENT:/OBSERVER: span name prefixes diverge from repo convention. (inline)
Add explicit SpanKind::KIND_INTERNAL on child spans. (inline)

Consider

Worth filing a follow-up to factor the generic HTTP server bookkeeping into shared code (as Nevay suggested) — the same logic now exists in Slim, Laravel, Wordpress, and Magento2.
Improve span-name cardinality (http.route after FrontController dispatch).

Risk

MEDIUM — purely additive, but CI as written won't pass, the interceptor issue will produce duplicate spans in production, and two HTTP semconv violations affect metric quality.

Happy to re-review once the blockers are addressed.

…Factory

jerrytfleung · 2026-05-22T22:54:46Z

Review summary — request changes

Thanks for putting this together — adding Magento2 to the contrib monorepo is real work and the test coverage here is thorough. Below is an overall verdict; specific findings are posted as inline comments on the relevant lines.

Must fix (blockers)

PHP version conflict: composer.json requires ^8.3 but CI matrix pins 8.1/8.2 — composer install will fail. (inline on composer.json:10, php.yml:95)

error.type misuse: uses $exception->getCode() (almost always 0); should be $exception::class. Also still missing for 5xx. (inline on Magento2Instrumentation.php:132)

_OTHER semantic violation: only emitted for empty methods; spec says emit for any non-standard method and set http.request.method_original. Histogram attrs also need the bucketed value. (inline on Magento2Instrumentation.php:95)

Http::launch double-fires through Magento's Interceptor pattern (Nevay's earlier flag is unaddressed). Need if ($class !== Http::class) return; guard. (inline on Magento2Instrumentation.php:87)

open-telemetry/sem-conv: ^1.36 is incompatible with the repo baseline of ^1.30. (inline on composer.json:13)

Missing runtime deps: nyholm/psr7, nyholm/psr7-server. (inline on Magento2Instrumentation.php:21)

magento/community-edition in require-dev is a 200MB+ metapackage; unit tests use mocks and likely don't need it. (inline on composer.json:30)

Should fix

Manager::dispatch and InvokerInterface::dispatch post-closures don't accept ?Throwable $exception — listener errors are silently swallowed. (inline)

Bootstrap::terminate creates a span on every request, not just on exception. (inline)

app/etc/vendor_path.php shouldn't ship in the package; .gitattributes doesn't export-ignore app/. (inline)

EVENT:/OBSERVER: span name prefixes diverge from repo convention. (inline)

Add explicit SpanKind::KIND_INTERNAL on child spans. (inline)

Consider

Worth filing a follow-up to factor the generic HTTP server bookkeeping into shared code (as Nevay suggested) — the same logic now exists in Slim, Laravel, Wordpress, and Magento2.

Improve span-name cardinality (http.route after FrontController dispatch).

Risk

MEDIUM — purely additive, but CI as written won't pass, the interceptor issue will produce duplicate spans in production, and two HTTP semconv violations affect metric quality.

Happy to re-review once the blockers are addressed.

Please take another look when you've time. Thanks!

jerrytfleung added 12 commits May 6, 2026 14:49

Added Magento2 instrumentation template

db9dbd8

Updated

ac3f63f

Added Magento2 instrumentation

77f6196

Added Magento2

0dcf2e5

Excluded 8.1 & 8.2 for magento2 instrumentation

d3c4fe7

Removed generated vendor_path.php

7eeb30f

Reverted

95d30ef

Removed .gitkeep

000eeb3

Updated psalm config and vendor_path.php

f0ad0ff

Updated comments and addressed nits

63296c4

nits

5cae4d9

nits

efa413d

jerrytfleung requested a review from a team as a code owner May 7, 2026 02:49

jerrytfleung mentioned this pull request May 7, 2026

Added bcmath, gd, soap extension for dependencies of Magento2 instrumentation open-telemetry/opentelemetry-php#1952

Merged

jerrytfleung changed the title ~~Added magento2 instrumentation~~ Adding magento2 instrumentation May 7, 2026

jerrytfleung commented May 7, 2026

View reviewed changes

Comment thread src/Instrumentation/Magento2/src/Magento2Instrumentation.php

Nevay reviewed May 7, 2026

View reviewed changes

jerrytfleung added 2 commits May 8, 2026 11:20

Removed Opt-In attributes in this PR

9a08bef

Not reading values from the span

3320e2c

jerrytfleung requested a review from Nevay May 8, 2026 21:10

jerrytfleung added 4 commits May 11, 2026 14:56

nits

0c15c71

deleted hidden .DS_Store

e2bdbcf

Added more instrumentation

495b6a8

Updated README.md

b6754f8

Nevay reviewed May 20, 2026

View reviewed changes

jerrytfleung and others added 4 commits May 20, 2026 15:04

Update src/Instrumentation/Magento2/src/Magento2Instrumentation.php

fd90fd9

Co-authored-by: Tobias Bachert <github.dev.k9ps1@mail.tobiasbachert.com>

Update src/Instrumentation/Magento2/src/Magento2Instrumentation.php

55a69cb

Co-authored-by: Tobias Bachert <github.dev.k9ps1@mail.tobiasbachert.com>

Update src/Instrumentation/Magento2/src/Magento2Instrumentation.php

7112311

Co-authored-by: Tobias Bachert <github.dev.k9ps1@mail.tobiasbachert.com>

Addressed comments

daea1f9

bobstrecansky reviewed May 21, 2026

View reviewed changes

jerrytfleung added 10 commits May 21, 2026 10:15

Attempted to set server address and server port according to the spec

5e3aaf3

Added Http::launch class checking

6b015bc

Used Nyholm\Psr7\Factory\Psr17Factory instead of Http\Discovery\Psr17…

0d567b7

…Factory

Fixed HTTP_REQUEST_METHOD

08fa3e6

Fixed error.type

064518f

Changed the name to meet OTEL instrumentation uses

49d4f28

Fixed post hook exception swallowing

f863835

Removed vendor_path.php and added .gitkeep

6a55b53

Updated root span name

89112f7

Added explicit KIND_INTERNAL instead of using the default KIND_INTERNAL

36801ca

jerrytfleung requested review from Nevay and bobstrecansky May 22, 2026 19:31

		->setAttribute(ServerAttributes::SERVER_ADDRESS, $request->getUri()->getHost())
		->setAttribute(ServerAttributes::SERVER_PORT, $request->getUri()->getPort() ?? ($request->getUri()->getScheme() === 'https' ? 443 : 80));

Conversation

jerrytfleung commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

jerrytfleung May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jerrytfleung May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

jerrytfleung commented May 7, 2026 •

edited

Loading

jerrytfleung May 21, 2026 •

edited

Loading

jerrytfleung May 22, 2026 •

edited

Loading

jerrytfleung May 22, 2026 •

edited

Loading