refactor: report errors through utopia-php/span instead of utopia-php/logger by ChiragAgg5k · Pull Request #9 · appwrite/docker-geo

ChiragAgg5k · 2026-10-04T16:34:38Z

What changed

Removed utopia-php/logger and added utopia-php/span 4.2.0. The error action no longer builds a Utopia\Logger\Log. It sets the throwable on the current request span and adds the attributes the old log carried as tags: error.type, error.code, http.method and http.path. It finishes the span after the response is sent.
An onRequest hook opens an http.request span, and a * shutdown hook finishes it when the request succeeds. Storage is Storage\Coroutine.
Exporters are registered at boot in Server::initSpan():
- Stdout prints server errors as one JSON line on stderr. This replaces the old Console::error lines.
- Sentry is added when GEO_LOGGING_CONFIG holds a sentry://PROJECT_ID:KEY@HOST DSN, parsed with Utopia\DSN\DSN. It keeps the environment (production/staging from GEO_ENV), release (GEO_VERSION) and server name (hostname) the logger sent.
Client errors are marked error.publish=false, so only 5xx and code-0 errors are printed or sent to Sentry. That matches the old behaviour.
build: commit: composer.json now requires PHP >=8.4 instead of >=8.3.0. The Docker image (appwrite/base:1.1.1) already runs PHP 8.5.4, and the lock already contained packages that need 8.4 (utopia-php/queue, utopia-php/pools, PHPUnit 13). span 4.2 needs 8.4 as well. No Dockerfile change.
test: commit: the E2E base now uses http_get_last_response_headers(). This removes the two PHP 8.5 deprecations the suite reported on every run.

Behaviour changes

GEO_LOGGING_PROVIDER is removed. The DSN scheme already names the provider. It was dropped from .env and docker-compose.yml.
Only Sentry is supported. AppSignal, Raygun and LogOwl DSNs, and the legacy key;projectId form that went with GEO_LOGGING_PROVIDER, now print Invalid GEO_LOGGING_CONFIG, error reporting is disabled: ... at boot and report nothing. No provider is configured in this repo (.env has both variables empty), and the appwrite-geo deployment in application-configuration sets only GEO_SECRET. So no deployed environment loses reporting today.
Sentry events are now built by span. The file, line and trace that the logger sent as extras arrive as the exception's stacktrace, and the old httpError action shows up as the http.request transaction.
Server errors on stderr are one NDJSON line, not four [Error] ... lines.

Validation

No unit suite: this PR keeps to the move and the E2E suite the repository already runs.

composer lint: {"result":"pass"}
composer check (PHPStan level 8): [OK] No errors
Docker build plus the CI flow (docker compose up -d --build geo, then the E2E suite in the tests container): E2E OK (9 tests, 23 assertions). CI runs the same E2E suite on this PR.
Live container with GEO_DBIP_PATH=/missing.mmdb and GEO_LOGGING_CONFIG=sentry://123:publickey@sentry.invalid:
- A 500, a 401 and a 404 each returned the same JSON body as before.
- Only the 500 produced output: one stderr span line ("level":"error","error.publish":true,"error.type":"Exception","error.code":0,"http.path":"/v1/ips/:ip") and one Sentry export attempt (Sentry exporter: Could not resolve host: sentry.invalid).
Live container with GEO_LOGGING_CONFIG=raygun://key: printed Invalid GEO_LOGGING_CONFIG, error reporting is disabled: Only the sentry:// scheme is supported, and the server still started.
No delivery to a real Sentry project was tested.

Part of appwrite/appwrite#13828

The image already runs PHP 8.5 (appwrite/base:1.1.1), and the lock was resolved with --ignore-platform-reqs against packages that need 8.4 (utopia-php/queue, utopia-php/pools, PHPUnit 13), so the ">=8.3.0" constraint no longer described what the code can run on. utopia-php/span 4.2, which the next commit adds for error reporting, also requires 8.4.

…/logger utopia-php/logger is being archived (appwrite/appwrite#13828) and the error handler was its only consumer here. Each request now opens a span; the error action sets the throwable on it with error.type, error.code, http.method and http.path (the old log tags), and finishes it once the response is sent. A shutdown hook finishes successful request spans. Exporters are registered at boot: a Stdout exporter prints server errors as one JSON line on stderr, replacing the Console::error lines, and when GEO_LOGGING_CONFIG holds a sentry://PROJECT_ID:KEY@HOST DSN, span's Sentry exporter reports them to Sentry with the same environment, release and server name the logger sent. Client errors are marked error.publish=false so, as before, only 5xx and code-0 errors leave the process. GEO_LOGGING_PROVIDER is removed: the DSN scheme already names the provider, and Sentry is the only exporter span ships. AppSignal, Raygun and LogOwl DSNs and the legacy "key;projectId" form now log a warning at boot and disable error reporting. Adds a unit suite (run in CI) that drives the error action and asserts on what a recording exporter receives.

…_header PHP 8.5 deprecates the implicit $http_response_header local, which made the E2E suite report two deprecations on every run. Use http_get_last_response_headers() instead.

hansi-codes · 2026-10-04T16:36:18Z

🔵 Tier A · Mergeable after minor fixes

The error-reporting migration no longer has automated coverage for exporter delivery and filtering.

Replaces logger-based error reporting with request spans and stdout/Sentry exporters, publishing only server errors. Removes GEO_LOGGING_PROVIDER, raises the PHP requirement to 8.4, and updates E2E response-header handling. The final version retains only the E2E suite rather than the previously added unit coverage.

Latest changes: The newest commits remove the unit-test script and coverage, the explicit PSR-18 dependency and injectable Sentry transport, and reuse of an injected HTTP instance's resource container.

Verdict	New comments	Fixed	Still open
✅ Approved	1	0	0

	Finding	Where
🟡	Retain coverage for error exporter delivery and filtering	`src/Geo/Server/Server.php:36`

Fix with agent prompt

### Issue 1
src/Geo/Server/Server.php:36
**Retain coverage for error exporter delivery and filtering**

With the unit tests removed, no remaining test checks that server errors reach the configured exporters or that client errors are suppressed; `tests/E2E/GeoTest.php` only checks HTTP responses. Please retain automated coverage for this reporting contract so a broken sampler or missing error attachment cannot pass the suite.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

📂 Walkthrough · 6

File	Change
`.env, docker-compose.yml`	Remove GEO_LOGGING_PROVIDER from environment configuration.
`.gitignore`	Ignore PHPUnit's result cache.
`composer.json, composer.lock`	Replace logger with span and require PHP 8.4; retain only the E2E test script.
`src/Geo/Modules/Core/Http/Error.php`	Attach errors and route attributes to spans and finish after responding.
`src/Geo/Server/Server.php`	Configure request spans and sampled stdout/Sentry exporters using the default transport.
`tests/E2E/Base.php`	Use http_get_last_response_headers() to avoid deprecated header access.

_{Reviewed the commits since ee628dd · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.}

hansi-codes

🔵 Tier A · Looks good to merge. Summary

The error action tests used an exporter that sampled everything, so they stayed green if the production filter was removed. ServerTest boots Server with an FPM adapter and a recording PSR-18 client for span's Sentry exporter, runs requests through the real hooks inside a coroutine, and asserts a 500 is delivered while a 401, a 404 and a 200 are not. Server takes an optional client (passed to the Sentry exporter) and reuses the injected Http's resources container, so the test drives the same boot path. Stdout and Sentry now share one sampler; Stdout writes to the STDERR constant, which a test cannot capture in-process, so the Sentry delivery stands in for both.

The refactor commit picked up .phpunit.result.cache from a local unit-test run. It is per-machine state, so remove it and ignore it.

Keep this PR to the logger-to-span move and the E2E suite the repository already runs. The unit tests needed two seams in Server that production never uses (an injectable PSR-18 client for the Sentry exporter and reusing a passed Http's resources), so those and the direct psr/http-client requirement go with them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

hansi-codes

🔵 Tier A · Looks good to merge. Summary

hansi-codes · 2026-10-05T08:21:39Z


        Http::setMode(System::getEnv('GEO_ENV', Http::MODE_TYPE_PRODUCTION));

+        $this->initSpan();


Retain coverage for error exporter delivery and filtering

With the unit tests removed, no remaining test checks that server errors reach the configured exporters or that client errors are suppressed; tests/E2E/GeoTest.php only checks HTTP responses. Please retain automated coverage for this reporting contract so a broken sampler or missing error attachment cannot pass the suite.

Prompt To Fix With AI

This is a comment left during a code review. Path: src/Geo/Server/Server.php Line: 36 Comment: **Retain coverage for error exporter delivery and filtering** With the unit tests removed, no remaining test checks that server errors reach the configured exporters or that client errors are suppressed; `tests/E2E/GeoTest.php` only checks HTTP responses. Please retain automated coverage for this reporting contract so a broken sampler or missing error attachment cannot pass the suite. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{🟡 Minor · testing · Reply if this doesn't apply.}

ChiragAgg5k added 3 commits October 4, 2026 17:33

test: read E2E response headers without the deprecated $http_response…

39e9d18

…_header PHP 8.5 deprecates the implicit $http_response_header local, which made the E2E suite report two deprecations on every run. Use http_get_last_response_headers() instead.

hansi-codes Bot previously approved these changes Oct 4, 2026

View reviewed changes

Comment thread src/Geo/Server/Server.php Outdated

ChiragAgg5k mentioned this pull request Oct 4, 2026

Absorb the remaining Utopia libraries into packages/ appwrite/appwrite#13828

Open

17 of 30 tasks

ChiragAgg5k dismissed hansi-codes[bot]’s stale review via 52cb4d2 October 4, 2026 18:55

ChiragAgg5k and others added 2 commits October 4, 2026 19:55

chore: stop tracking the PHPUnit result cache

ee628dd

The refactor commit picked up .phpunit.result.cache from a local unit-test run. It is per-machine state, so remove it and ignore it.

hansi-codes Bot approved these changes Oct 5, 2026

View reviewed changes

lohanidamodar merged commit 41b555a into appwrite:main Oct 5, 2026
4 checks passed

Navigation Menu

refactor: report errors through utopia-php/span instead of utopia-php/logger - #9

refactor: report errors through utopia-php/span instead of utopia-php/logger#9
lohanidamodar merged 6 commits into
appwrite:mainfrom
ChiragAgg5k:refactor/logger-to-span

ChiragAgg5k commented Oct 4, 2026 •

edited

Loading

Uh oh!

hansi-codes Bot commented Oct 4, 2026 •

edited

Loading

Uh oh!

hansi-codes Bot left a comment

Uh oh!

Uh oh!

hansi-codes Bot left a comment

Uh oh!

hansi-codes Bot Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		Http::setMode(System::getEnv('GEO_ENV', Http::MODE_TYPE_PRODUCTION));

		$this->initSpan();

Conversation

ChiragAgg5k commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Behaviour changes

Validation

Uh oh!

hansi-codes Bot commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔵 Tier A · Mergeable after minor fixes

Uh oh!

hansi-codes Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hansi-codes Bot left a comment

Choose a reason for hiding this comment

Uh oh!

hansi-codes Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChiragAgg5k commented Oct 4, 2026 •

edited

Loading

hansi-codes Bot commented Oct 4, 2026 •

edited

Loading