Submit Swift test executions through OpenTelemetry - #12
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aa65037d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Three issues need addressing before this is ready: CI cannot resolve the dependencies at the advertised Swift floor, OTLP endpoint selection can send the suite token to an unrelated collector, and child-span forwarding is ineffective on Linux. Details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20842, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only when repository policy or a verified grant allows it; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
There was a problem hiding this comment.
The three previous blockers remain open; the Swift 6.0 dependency adjustment still fails in build #69. I also found a duplicate-retry path introduced by the new processor queue.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20860, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The latest transport wrapper keeps failed-root retries owned by one layer, but the three earlier blockers remain open; I’ve updated their existing threads.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20869, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The Swift 6.0 dependency floor now resolves and build #73 passed. The OTLP credential and Linux child-span blockers remain open, and I found one additional provider-lifecycle blocker inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20873, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The three existing endpoint/context/provider blockers remain open; I’ve updated those threads. The new Swift 5.10 compatibility change also selects a dependency that Swift 5.10 cannot compile.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20875, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The endpoint-credential and Linux-context fixes are resolved. Two blockers remain in the existing threads; build #75 passed, but it only exercises Swift 6.0.3.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20879, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The late-provider forwarding blocker is fixed and covered by the new regression test. The existing Swift 5.10 compatibility blocker remains open; build #76 still only exercises Swift 6.0.3.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20884, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The last open concern is resolved: build #77 passed the test and release-build commands under both Swift 5.10.1 and 6.0.3. I’m leaving this comment-only because the change replaces the execution transport and changes suite-token routing, which is outside the repository’s L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20885, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
🤖 Review (Amp / Fable 5 high)I read the full diff, compared it line-by-line with Ruby's VerdictSolid, well-designed port of the Ruby OTel approach — arguably better in places — but it will break some existing users automatically, because the breakage ships through SwiftPM's default version range, and the changes to toolchain floor, deployment target, and dependency graph are all breaking. 1. Likely to break existing users?The README tells consumers to use
Public API is handled well: Recommendation: given the auto-flow through 2. Parity with Ruby's otel_only?Very close — a faithful port, and in some ways a superset. Matching: Deliberate/structural differences worth knowing:
3. Sensible range of Swift/XCTest/XCUITest versions?Mostly yes, verified rather than assumed:
4. Conflicts with existing OpenTelemetry tracing in Swift codebases?Direct opentelemetry-swift usage in app code is uncommon — iOS teams mostly get it indirectly via vendor SDKs (Honeycomb, Embrace, Elastic, Splunk RUM distributions all build on opentelemetry-swift), and server-side Swift overwhelmingly uses swift-otel (swift-distributed-tracing based), which is a completely separate library the collector neither conflicts with nor captures spans from. So the population is small but nonzero, and growing on mobile. Where conflicts actually bite, in order of severity:
Net: three things before merge — a release-versioning decision (1.0.0 vs documented auto-upgrade), a warning for the now-ignored |
🤖 Delta review: 08e2099 → c00e7b6 (Amp / Fable 5 high)This delta addresses all three pre-merge items from my previous review. I re-verified locally: the full suite passes at c00e7b6 on Swift 6.3.2 / macOS, including the new 1. Versioning → 2.0.0-beta.1 (0903df8) ✅Resolves the auto-upgrade hazard twice over: Follow-up for release time (not this PR): because 2.
|
Previous concerns have been addressed.
There was a problem hiding this comment.
The prior blockers remain resolved, but a standard-header-only configuration can still send a non-Buildkite credential to the Buildkite endpoint; details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20916, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
The existing OTLP header-routing blocker remains open; the latest revision only changes export-backoff diagnostics and tests.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20917, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The standard-header routing concern is fixed. One new endpoint-configuration blocker remains inline: standard protocol variables can still disable a trusted Buildkite export.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 21112, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
7540800 to
bd04171
Compare
There was a problem hiding this comment.
The existing endpoint-configuration blocker remains open; no new findings this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 21489, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Previous concerns have been addressed.
There was a problem hiding this comment.
The endpoint-configuration blocker is fixed, and I found no new issues. This remains comment-only because the change replaces the execution transport and changes suite-token routing, beyond the repository’s L1 approval ceiling.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 21494, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
6374d37 to
50d5c2e
Compare
|
🤖 Rebased
The final replay onto |
Replace the collector's proprietary JSON execution uploads with OpenTelemetry test.execution root spans accepted by the Test Engine OTLP endpoint. Keep execution and run tagging APIs compatible by emitting buildkite.tag.* attributes, preserve XCTest failure details as span status and exception events, and forward sampled application spans beneath each test root. Export each completed root synchronously and retain failed requests in the exporter's in-memory queue so later tests or the bundle flush can retry. This also lets the Test Engine Client relay acknowledge completed tests before an XCTest runner can restart. Support standard OTLP HTTP environment configuration, retain the suite token authentication path, and document the remaining in-memory delivery limitations. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Pin OpenTelemetry to its Swift 6.0-compatible releases and constrain swift-metrics below the Swift 6.1 tools-version bump. Own failed root-span requeueing in the synchronous execution processor instead of depending on the newer exporter API. Retry pending roots with the next execution or final flush, and cover the retry batch behavior. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
OpenTelemetry 2.4 requires swift-metrics 2.10, whose package manifest needs Swift 6.1. Pin the last compatible OpenTelemetry pair and swift-metrics 2.8 so the collector remains buildable on its declared Swift 6.0 floor. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Attach CoreMetrics to the collector target so downstream SwiftPM consumers inherit the swift-metrics 2.8 constraint instead of resolving the Swift 6.1-only 2.9+ manifests. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
OpenTelemetry 2.3's HTTP exporter returns before its URLSession request completes, and its flush does not wait for in-flight requests. Wrap its HTTP client with a synchronous transport and only report success after a 2xx response so XCTest cannot move on or exit before the relay owns a completed execution. Keep failed roots in the collector's existing retry queue and prevent the upstream exporter from retaining a duplicate copy. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Pin OpenTelemetry to the last release before its exact gRPC dependency raised the tools version to Swift 6.1. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Lower the package manifest and metrics dependency requirements so the collector can be resolved by Xcode 15.4 while continuing to compile its sources in Swift 5 language mode. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Keep application spans attached to XCTest execution roots on Linux by installing an imperative execution-aware context manager, while preserving closure-scoped OpenTelemetry context. Reattach the forwarding processor at test start so application tracer providers registered after collector load are also covered. Do not synthesize Buildkite credentials for standard OTLP endpoints; only the default and collector-specific trusted endpoint receive the suite token and run key. Preserve custom run-environment overrides and execution-name affixes when translating the former upload model into OTLP attributes. Exercise provider replacement, Linux child forwarding, endpoint trust, custom overrides, and name affixes. Correct the documented Swift floor to 5.10. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Preserve the platform's existing context manager while observing active-span reads, allowing the collector to attach execution forwarding to providers registered during test setup or the test body before their first span starts. Also correct the documented minimum Swift version to 5.10. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Run collector tests and the release build under both Swift 5.10.1 and 6.0.3 so the documented minimum remains continuously verified. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Set the reported collector version to 2.0.0-beta.1 and record that release target in the changelog and README without changing the currently published installation version. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Report BUILDKITE_ANALYTICS_BASE_URL during collector initialization and direct users to the supported OTLP endpoint settings. Add a warning log level and regression coverage. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Preserve synchronous handoff while the configured endpoint is healthy, then use a capped 10, 20, 30, and 60 second retry schedule after failures. Keep queued executions in memory and let the bundle flush make an immediate final attempt. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Log executions queued during an active export backoff and directly assert that a successful export resets the next retry delay to 10 seconds. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Require an explicit standard OTLP endpoint before applying standard exporter headers. Ignore and warn about those headers on endpoints that receive Buildkite credentials so unrelated process-wide credentials cannot be sent to Buildkite or replace suite authentication. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
Validate standard OTLP protocol variables only for explicitly configured standard endpoints. Trusted Buildkite endpoints keep their fixed HTTP/protobuf transport when unrelated process-wide OTLP protocol settings are present. Amp-Thread-ID: https://ampcode.com/threads/T-01a037c7-e3e7-70e4-b18f-a43cec542d56 Co-authored-by: Amp <amp@ampcode.com>
50d5c2e to
497c5a8
Compare
Previous concerns have been addressed.
There was a problem hiding this comment.
The prior endpoint-configuration concern remains resolved. One new test-isolation blocker is reproducible across all three macOS CI jobs; details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 21500, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
| @@ -6,14 +6,24 @@ final class CollectorTests: XCTestCase { | |||
| let environment = EnvironmentValues(values: [:]) | |||
There was a problem hiding this comment.
Blocking: Build #104 fails this test on all three macOS jobs because EnvironmentValues(values: [:]) falls back to the runner's actual environment; that supplies exporter configuration, so the new XCTAssertNil is false. Please isolate this no-configuration case from both environment and Info.plist lookups.
| let environment = EnvironmentValues(values: [:]) | |
| let environment = EnvironmentValues( | |
| values: [:], | |
| getFromEnvironment: { _ in nil }, | |
| getFromInfoDictionary: { _ in nil } | |
| ) |
Why
The Swift collector currently buffers proprietary JSON executions inside the XCTest runner. When the runner is restarted or killed, completed results that have not reached
/v1/uploadsdisappear; the XCUITest reproduction retains only 2 of 5 executions. An internal resilience assessment recommends handing each completed execution to the longer-lived Test Engine Client process through its OTLP relay.What
/v1/uploadsclient. Every completed XCTest is now a parentlesstest.executionroot span marked withbuildkite.execution.via=otlp, with failures represented as span status and exception events.tagExecutionAPI through OTLP resource/span attributes. Application spans created during a test are exported as children, including on Linux and when the application registers its tracer provider after collector startup.OTEL_EXPORTER_OTLP[_TRACES]_*HTTP configuration without disclosing the Buildkite suite token to standard endpoints. The default and collector-specific trusted endpoint retain suite-token authentication.Related end-to-end example: buildkite/test-engine-client-examples#68