Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the remaining Rust HTTP server conformance scenario using Tower instrumentation with Axum.
Changes:
- Adds shared Axum routes and an instrumented server.
- Registers dependencies and captured conformance results.
- Updates Rust HTTP documentation.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scenarios/http/rust/tower/scenarios/src/lib.rs |
Defines the Axum workload routes. |
scenarios/http/rust/tower/scenarios/Cargo.toml |
Configures the workload crate. |
scenarios/http/rust/tower/opentelemetry-instrumentation-tower/server/src/main.rs |
Runs the instrumented server and SDK lifecycle. |
scenarios/http/rust/tower/opentelemetry-instrumentation-tower/server/data.json |
Records observed telemetry coverage and findings. |
scenarios/http/rust/tower/opentelemetry-instrumentation-tower/server/conformance.yaml |
Defines the conformance scenario. |
scenarios/http/rust/tower/opentelemetry-instrumentation-tower/server/Cargo.toml |
Configures the instrumented binary. |
scenarios/http/rust/README.md |
Documents the Tower scenario structure. |
scenarios/http/rust/Cargo.toml |
Registers workspace crates and dependencies. |
scenarios/http/rust/Cargo.lock |
Locks the added dependencies. |
scenarios/http/README.md |
Adds the scenario to HTTP documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
40 tasks
trask
force-pushed
the
trask-rust-tower-server
branch
from
August 30, 2026 01:15
191adb0 to
456cfe1
Compare
trask
changed the title
trask
force-pushed
the
trask-rust-tower-server
branch
from
August 30, 2026 02:42
10725e3 to
165255f
Compare
trask
marked this pull request as ready for review
August 30, 2026 15:12
Pull request dashboard statusWaiting on maintainers · refreshed 2026-10-25 03:25 UTC Merge when ready. Status above doesn't look right?
|
trask
force-pushed
the
trask-rust-tower-server
branch
from
August 31, 2026 02:04
165255f to
f22be6a
Compare
trask
force-pushed
the
trask-rust-tower-server
branch
from
August 31, 2026 16:44
f22be6a to
b464567
Compare
trask
force-pushed
the
trask-rust-tower-server
branch
from
August 31, 2026 18:10
db1857b to
b97967f
Compare
lmolkova
approved these changes
Sep 6, 2026
| ), | ||
| ) if { | ||
| _http_span_kind(input.sample.span) == "server" | ||
| _http_attr_value(input.sample.span, "url.path") == "/users/456" |
Member
There was a problem hiding this comment.
This exact match makes the rule a no-op for the other thing the query request exists to catch: an instrumentation that leaks the query into url.path fails the equality, so no finding is reported and nothing else checks url.path. Matching on the path before any ? would report the missing attribute and let you flag the leak too.
| }); | ||
|
|
||
| let wait = tokio::task::spawn_blocking(otel_conformance_scenario::wait_for_eof).await?; | ||
| shutdown_sender.send(()).map_err(|()| { |
Member
There was a problem hiding this comment.
nit: when the send fails the server task has already ended with the real error, and that error is dropped in favor of a generic BrokenPipe. Await server in that branch and return its error instead.
4 tasks done
trask
force-pushed
the
trask-rust-tower-server
branch
2 times, most recently
from
September 10, 2026 21:15
a9efb7e to
c769a27
Compare
trask
force-pushed
the
trask-rust-tower-server
branch
from
September 29, 2026 02:53
fc272a5 to
f29afdd
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: The shared contract sends `/users/123?fields=name&verbose=true`, but this coverage has no `url.query`; because this list is the union of observed span attributes, none of the server spans recorded it. HTTP server semantic conventions mark `url.query` conditionally required whenever a query is received, yet the findings array also omits that violation, so this scenario silently underreports an instrumentation gap. Extend the HTTP validation to check this contract-specific condition, regenerate this file, and include the gap in the scenario documentation. Analysis: The policy could not distinguish the query request from the query-free request after instrumentation omitted `url.query`, because both requests used `/users/123`. The contract now uses `/users/456` for the query exchange. The policy can identify that request by `url.path` and report the missing conditionally required attribute. The regenerated Tower coverage and Rust scenario documentation record the gap. Upsides: Coverage now reports a real Tower instrumentation gap. The distinct request path avoids false positives on conforming server spans that receive no query string. Tests cover both missing and present `url.query` cases. Downsides: The shared query exchange now uses user ID 456, so each language's contract test expectation changes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: Changing this request to `/users/456` creates additional signal names for route-less servers, but their committed coverage was not regenerated. The ASGI snapshot currently contains only `GET /users/123` and its receive/send child findings (`scenarios/http/python/asgi/opentelemetry-asgi/server/data.json:153-179`), and WSGI likewise only contains `GET /users/123` (`scenarios/http/python/wsgi/opentelemetry-wsgi/server/data.json:86-94`). The Python scenario CI reruns every package and rejects coverage diffs, so these snapshots will change when the query request is emitted as `/users/456`. Please regenerate and commit the affected existing server `data.json` files. Analysis: ASGI and WSGI omit `http.route`, so their span names use each concrete request path. The distinct query path adds `GET /users/456` findings while preserving the findings for the query-free `/users/123` request. Regenerating both scenarios records the additional server and ASGI child-span findings. Upsides: The committed coverage now matches the shared contract for every route-less server scenario. Python scenario CI no longer finds stale snapshots. Downsides: The coverage files contain more findings because each concrete user path remains a separate unbounded-cardinality span name until those instrumentations emit `http.route`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Review finding: This line adds tokio's `sync` feature for the new Tower server's `oneshot::channel`, but the same new binary also calls `tokio::net::TcpListener::bind` in `scenarios/http/rust/tower/opentelemetry-instrumentation-tower/server/src/main.rs`, and `net` is not in the declared feature list. `tokio::net` is gated behind tokio's `net` feature, so the crate compiles only because axum 0.8.9's `tokio` feature (enabled two lines above) transitively turns on `tokio/net`. That makes a direct API use depend on another dependency's feature choice. A fix is to add `net` to the tokio feature list so the feature the code uses directly is declared where it is used. Analysis: Tokio exports `TcpListener` inside `cfg_net!`, so `tokio::net` exists only when the `net` feature is on. The workspace declaration listed `macros`, `rt-multi-thread` and `sync`, and axum 0.8.9's `tokio` feature turns on `tokio/net` for the whole build, which is what let the Tower server compile. Listing `net` beside `sync` states the feature the scenario binary uses directly. Cargo resolves the same versions, so `Cargo.lock` is unchanged. Upsides: The Tower server no longer depends on axum's feature list to reach `tokio::net`, so dropping or narrowing the axum dependency fails at the manifest rather than deep in a compile error. The declaration now matches the same rule the `sync` entry follows. Downsides: No material downside identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Review finding: The new rule hardcodes the contract path '/users/456' (and repeats the full request string '/users/456?fields=name&verbose=true' in its message), duplicating a value whose single source of truth is tools/http/test-client/contract.json. Nothing ties the two together: no test reads contract.json, and the equality check fails silently rather than erroring, so editing the query request's path in contract.json would quietly stop the url.query check from ever firing and every affected coverage file would simply lose a finding. A fix is a small test in tools/http/runner/tests that loads tools/http/test-client/contract.json and asserts it still contains a GET request whose path starts with '/users/456?', so the drift fails loudly. Review finding: The query request's path changed from '/users/123?...' to '/users/456?...' purely so the HTTP policy can identify that request, but the exchange's own 'description' field still only says the request is about a query string. contract.json documents itself as the description read by every language rather than restated elsewhere, so a reader or future editor of this file has no way to know the '456' is load-bearing and could normalise it back to '/users/123' for consistency, silently disabling the url.query check. A fix is to add a clause to that description saying the path differs from the other /users request so validation can identify this one when url.query is missing. Analysis: The policy recognises the contract's query request by matching url.path against a literal path, and the finding message repeats the full request target. Rego evaluates a failed equality as "no finding", so a contract edit produces silence rather than an error. The policy test now reads contract.json, takes the single GET request carrying a query, and builds its sample spans from that path and query instead of from literals; it also asserts the finding message quotes the contract's exact request target. Any spelling difference between the contract and the policy now fails test_contract_query_request_requires_url_query rather than passing quietly. The contract entry's own description records that its path is deliberately distinct, which is the field this repository already uses to carry a request's purpose to every language that reads the contract. Upsides: A contract edit that would disable the url.query check fails a test instead of silently removing a finding from every affected coverage file. The reason the path is distinct is recorded in the file that holds the value, not only in two READMEs a maintainer editing JSON may not open. Downsides: The policy test now depends on tools/http/test-client/contract.json, so a structural change to the contract file breaks a runner test as well as the test-client tests. The assertion that exactly one GET request carries a query string is an assumption the contract could outgrow, and adding a second query request would require choosing which one the policy identifies anyway. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Failing check: Scenarios (http-php): the committed data.json files are out of date — run the scenarios and commit the result Cause: The query-string contract added a required url.query finding for the PHP Slim server, but its committed coverage data did not include that finding. Fix: Add the generated missing-url.query finding so the committed PHP Slim coverage matches the scenario output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Failing check: Scenarios (http-go): the committed data.json files are out of date — run the scenarios and commit the result Cause: The query-string contract added a required url.query finding for five Go servers, but their committed coverage data did not include that finding. Fix: Regenerate the affected Go coverage files so they match each scenario's output. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Failing check: Lint: ClientTest#test_drives_measured_requests_through_the_supplied_sender [test/client_test.rb:19]: Cause: The contract now uses user 456 for the query-string request, but the Ruby client test still expected user 123. Fix: Update the deliberate request expectation to match the contract's distinct query-request path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Failing check: Lint: contract_test.go:53: sent [GET /users/123 GET /users/456?fields=name&verbose=true POST /items GET /status/404 GET /status/500], want [GET /users/123 GET /users/123?fields=name&verbose=true POST /items GET /status/404 GET /status/500] Cause: The contract now uses user 456 for the exact query-string request, but two Go client tests still used user 123. Fix: Update the deliberate request and exact-lookup expectations to the contract's user 456 path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
trask
force-pushed
the
trask-rust-tower-server
branch
from
October 24, 2026 23:04
f29afdd to
7e8d0e3
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the remaining Rust HTTP server conformance scenario from #139 for
opentelemetry-instrumentation-tower0.18.0 through its Axum integration. It covers route templates, query strings, request bodies, error responses, server spans, and all four standard HTTP server metrics.The shared query request now uses
/users/456so validation can identify a missing conditionally requiredurl.queryattribute. Coverage records that gap, plus missingclient.address,server.address, span-levelnetwork.protocol.version, anderror.typeon error spans.Stacked on #62.