Repository navigation
chore: absorb telemetry into packages/telemetry - #13731
Conversation
feat: opentelemetry adapter
feat(testing): ship testing adapter
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Feat swoole support
Implements an asynchronous gauge whose value is collected via a callback at export time, following the OpenTelemetry observable instrument pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… calls Register the OTel-level callback once in the constructor and delegate to a single replaceable closure, so calling observe() again replaces rather than adds another callback. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…try adapter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add ObservableGauge metric type
chore(telemetry): drop unused ext-opentelemetry requirement
docs: enforce documentation style with Vale
fix(telemetry): register instruments on first write
…ion-suggest fix(telemetry): make ext-protobuf a suggestion, not a requirement
chore: stop committing package lock files
…trics fix(telemetry): omit empty metrics from OTLP exports
…ad-pooled-connection fix(telemetry): retry an export once when the pooled connection is reset mid-request
perf(telemetry, circuit-breaker): record less per call on the cache path
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fifth package on the plan in rfc/monorepo.md. Imported from the mirror with history (the preceding subtree merge), src/Telemetry flattened to src/, tests/Telemetry flattened to tests/ under Utopia\Telemetry\Tests\, the OTLP transport tests that bind a loopback Swoole server moved to tests/E2E/, the package PHPStan config standalone at its own level max, rector.php standalone with phpunitCodeQuality dropped. Nine vendored packages require utopia-php/telemetry; the root now replaces it and hoists its four third-party requirements (nyholm/psr7, open-telemetry/exporter-otlp, open-telemetry/sdk, symfony/http-client). The lock drops only utopia-php/telemetry. Appwrite ran 0.4.10; the mirror head is one merge past the 0.4.11 tag and the only public-surface change since 0.4.8 is an added exporter wrapper. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public function testConstructorParsesEndpointCorrectly(): void | ||
| { | ||
| $transport = new Swoole('https://otel.example.com:4318/v1/metrics?foo=bar'); | ||
|
|
||
| $this->assertSame(ContentTypes::PROTOBUF, $transport->contentType()); |
There was a problem hiding this comment.
These tests claim to cover endpoint parsing, headers, timeouts, pool sizing, socket buffers, query strings, and default ports, but merely assert the unrelated content type. They would remain green if every named behavior were broken. The same implementation-coupled pattern appears in LazyInstrumentTest.php, which asserts adapter storage arrays and object identity. This violates the repository directive to test observable behavior rather than mirror source parameters or configuration, so the requirement must be satisfied before merging.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/telemetry/tests/Adapter/OpenTelemetry/Swoole/TransportTest.php
Line: 47-51
Comment:
**Tests provide false coverage**
These tests claim to cover endpoint parsing, headers, timeouts, pool sizing, socket buffers, query strings, and default ports, but merely assert the unrelated content type. They would remain green if every named behavior were broken. The same implementation-coupled pattern appears in `LazyInstrumentTest.php`, which asserts adapter storage arrays and object identity. This violates the repository directive to test observable behavior rather than mirror source parameters or configuration, so the requirement must be satisfied before merging.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pre-existing in the imported library at mirror commit 852e76c (the 0.4.11 release). An absorb PR relocates the package with its history and does not change library behaviour or tests, matching #13723, #13798, #13804 and #13829. Strengthening these transport tests belongs in its own change against packages/telemetry.
There was a problem hiding this comment.
That’s fair. Since this PR only relocates the imported package and preserves the tests from mirror commit 852e76c, the weak assertions are pre-existing and not an absorption regression. I’ll withdraw this as a merge blocker; strengthening the transport (and analogous lazy-instrument) tests is appropriately handled in a follow-up change against packages/telemetry.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
Prettier flags the file on every commit that stages it, which blocks merging main into feature branches; quotes and trailing newline only.
# Conflicts: # composer.json # composer.lock # mongo-init.js
…ent traffic test curl under Swoole's coroutine hook fails on CI without reaching the API, so the six creates never happened and every receiver timed out waiting for events. Race the creates through the client's curl_multi batch instead, keep the five receivers as coroutines, and move every assertion outside the scheduler so a failure is reported instead of killing the process. The same pattern was removed from the client in 3194210.
5 participants
Summary
Fifth package on the plan in
rfc/monorepo.md, one package per PR. Three commits:Add 'packages/telemetry/' from commit 852e76c8…: subtree import from the mirror, full history; the mirror ruleset updated by the sameabsorbrun.chore(telemetry): mirror plumbing:mirror.ymlpointed here, README banner refreshed.refactor: load telemetry from packages/:src/Telemetryandtests/Telemetryflattened, tests underUtopia\\Telemetry\\Tests\\, the OTLP transport tests that bind a loopback Swoole server moved totests/E2E/with atest:e2escript (no compose file needed), standalone PHPStan config at the package's own level max, standalone Rector config. Nine vendored packages requireutopia-php/telemetry; the root now replaces it and hoists its four third-party requirements (nyholm/psr7,open-telemetry/exporter-otlp,open-telemetry/sdk,symfony/http-client). The lock drops onlyutopia-php/telemetry.Drift, checked
Appwrite ran 0.4.10. The mirror head is one merge past the 0.4.11 tag (a per-call cost optimisation). Since 0.4.8 the only public-surface change is an added exporter wrapper class; nothing removed or re-signed, so the nine dependants are unaffected.
Known local-only failure
bin/monorepo test telemetry's e2e tier segfaults on macOS (Swoole 6 + PHP 8.5) in one retry-timing test, reproduced on the untouched import; the Linux runner passed it on #13721, which carried the same files.Merging
Merge with a merge commit, not a squash, so the subtree annotation survives for the split.
Verification
bin/monorepo validate: all packages valid;bin/monorepo check telemetrycleanbin/monorepo test telemetry: unit tier 24 tests green (e2e tier: see above)vendor/bin/phpunit --testsuite packages: 223 tests, 4 skippedUtopia\\Telemetry;composer validatevalid;dump-autoloadwithout PSR-4 warnings🤖 Generated with Claude Code