Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces first-class Rust support for the HTTP conformance domain by adding a Rust scenario toolchain (otel-conformance-rust), shared Rust scenario support crates, and a single Cargo workspace containing Actix Web (server) and awc (client) conformance packages with committed coverage outputs.
Changes:
- Add a Python-based Rust launcher (
otel-conformance-rust) plus shared Rust support crates for scenario lifecycle/env handling and OTLP (tonic gRPC) SDK bootstrap. - Add a new
scenarios/http/rustCargo workspace with Actix Web server and awc client scenarios, including conformance package YAMLs and recordeddata.jsoncoverage/violations. - Wire Rust into repo-level docs, lint/type config, CI workflows, Renovate grouping, and ignore Cargo
target/artifacts.
Reviewed changes
Copilot reviewed 33 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/rust/tests/test_rust_scenario.py | Adds unit tests for the Rust launcher’s workspace/package discovery and command construction. |
| tools/rust/src/otel_conformance_rust/init.py | Implements otel-conformance-rust build/run CLI (workspace + package manifest discovery; direct binary execution). |
| tools/rust/scenario/src/lib.rs | Provides Rust scenario helpers for required env var lookup and stdin-EOF shutdown protocol (no OTel dependency). |
| tools/rust/scenario/Cargo.toml | Declares the otel-conformance-scenario crate as a workspace member. |
| tools/rust/scenario-sdk/src/lib.rs | Provides a shared Rust SDK bootstrap crate that installs global tracer/meter providers and shuts them down. |
| tools/rust/scenario-sdk/Cargo.toml | Declares the otel-conformance-scenario-sdk crate and its OTel dependencies via workspace pins. |
| tools/rust/README.md | Documents the shared Rust tooling layout and how scenarios use otel-conformance-rust. |
| tools/rust/pyproject.toml | Defines the Python package and console script for otel-conformance-rust. |
| tools/http/test-client/rust/src/lib.rs | Adds a Rust HTTP contract client library (embed contract.json, respond/drive/verify helpers). |
| tools/http/test-client/rust/README.md | Documents the Rust HTTP test client crate API and testing approach. |
| tools/http/test-client/rust/Cargo.toml | Declares the Rust HTTP test client crate as a workspace member with shared deps. |
| tools/http/test-client/README.md | Adds the Rust test-client to the cross-language test client overview. |
| scenarios/http/rust/rust-toolchain.toml | Pins the Rust toolchain + components for this workspace. |
| scenarios/http/rust/README.md | Documents the Rust HTTP workspace layout and how to run fmt/clippy/test. |
| scenarios/http/rust/Cargo.toml | Declares the Rust HTTP workspace members and shared dependency versions/lints. |
| scenarios/http/rust/Cargo.lock | Adds the committed lockfile pinning all Rust dependencies used by the workspace. |
| scenarios/http/rust/awc/scenarios/src/lib.rs | Implements the “plain” awc workload crate (no OTel dependency) that constructs and sends contract requests. |
| scenarios/http/rust/awc/scenarios/Cargo.toml | Declares the awc workload crate. |
| scenarios/http/rust/awc/opentelemetry-actix-web/client/src/main.rs | Adds the instrumented awc client binary using opentelemetry-instrumentation-actix-web + shared SDK bootstrap. |
| scenarios/http/rust/awc/opentelemetry-actix-web/client/data.json | Adds recorded client-side span coverage and findings from live Weaver runs. |
| scenarios/http/rust/awc/opentelemetry-actix-web/client/conformance.yaml | Defines the Rust awc client conformance package and expected violations. |
| scenarios/http/rust/awc/opentelemetry-actix-web/client/Cargo.toml | Declares the instrumented awc client binary crate and deps. |
| scenarios/http/rust/actix-web/scenarios/src/lib.rs | Implements the “plain” Actix Web server workload crate (no OTel dependency) that answers the contract routes. |
| scenarios/http/rust/actix-web/scenarios/Cargo.toml | Declares the Actix Web workload crate. |
| scenarios/http/rust/actix-web/opentelemetry-actix-web/server/src/main.rs | Adds the instrumented Actix Web server binary with tracing+metrics wrappers and stdin-EOF shutdown. |
| scenarios/http/rust/actix-web/opentelemetry-actix-web/server/data.json | Adds recorded server-side span+metric coverage and findings from live Weaver runs. |
| scenarios/http/rust/actix-web/opentelemetry-actix-web/server/conformance.yaml | Defines the Rust Actix Web server conformance package and expected violations. |
| scenarios/http/rust/actix-web/opentelemetry-actix-web/server/Cargo.toml | Declares the instrumented Actix Web server binary crate and deps. |
| scenarios/http/README.md | Documents the Rust HTTP scenario package layout and adds example invocations. |
| rustfmt.toml | Sets repository-wide rustfmt newline style. |
| README.md | Adds tools/rust/ to the top-level repository structure overview. |
| pyproject.toml | Registers the Rust tool Python package in formatting/import/type-check configuration. |
| .gitignore | Ignores Cargo target/ directory. |
| .github/workflows/runner.yml | Installs Rust tool package in CI and adds Rust fmt/clippy/test + scenario runs prerequisites. |
| .github/renovate.json5 | Adds a Renovate group for non-major Cargo dependency updates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tools/rust/src/otel_conformance_rust/init.py:101
_section_value()only recognizes TOML strings wrapped in double quotes. Cargo manifests may legally use single-quoted (literal) strings (e.g.,name = 'crate'), which would makepackage_manifest()fail to detect a valid package and breakotel-conformance-rustfor such workspaces.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tools/rust/src/otel_conformance_rust/init.py:63
binary()assumes Cargo outputs into<workspace>/target/<profile>/..., but Cargo can be configured to use a different target directory viaCARGO_TARGET_DIR(or a.cargo/config.tomlbuild.target-dir). In that case,otel-conformance-rust buildmay succeed whileotel-conformance-rust runlooks in the wrong place and fails to find/execute the compiled binary.
Consider deriving the actual target directory from Cargo (e.g., cargo metadata’s target_directory) or at least honoring CARGO_TARGET_DIR when set, so build+run stay consistent under common Cargo configurations.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 35 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/runner.yml:95
- The Rust CI job runs
cargo clippy/cargo testwithout--locked, which allows Cargo to updateCargo.lockimplicitly if the lockfile drifts fromCargo.toml. Adding--lockedmakes CI fail fast when the committed lockfile is out of date and keeps builds reproducible.
cargo fmt --check
cargo clippy --workspace -- -D warnings
cargo test --workspace
trask
marked this pull request as ready for review
August 18, 2026 03:28
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-26 01:23 UTC Review the latest changes. Status above doesn't look right?
|
lmolkova
reviewed
Aug 18, 2026
40 tasks
Review finding: `tools/rust/README.md` states as a general rule that each domain owns one Cargo workspace and that the workspace includes these shared crates as members. The same PR makes that impossible beyond the first domain: `tools/rust/scenario/Cargo.toml`, `tools/rust/scenario-sdk/Cargo.toml`, and `tools/http/test-client/rust/Cargo.toml` each hardcode `workspace = ".../scenarios/http/rust"`, which binds them permanently to the HTTP workspace. Cargo refuses to load a package listed as a member of one workspace while its manifest points at another, failing with `package ... is a member of the wrong workspace`, so a second domain's workspace could never include these crates. Fix: reword the paragraph to describe what is true today, that the shared crates are members of the HTTP domain's workspace, rather than stating a per-domain rule the layout cannot satisfy. Analysis: the three shared manifests each set `workspace` to `scenarios/http/rust`, which is what lets them be members of a workspace they do not sit under. A package can name only one workspace, and Cargo rejects a package listed as a member of a second one, so the per-domain rule the README stated could never hold once a second domain added Rust. The paragraph now says which workspace these crates belong to, why they say so themselves, and leaves the choice open for a second domain instead of promising an arrangement Cargo forbids. Upsides: a reader adding Rust to another domain learns the real constraint here rather than copying a rule that fails at the first `cargo` invocation. The paragraph also now matches `scenarios/http/rust/README.md`, which already describes a single workspace. Downsides: the paragraph is two sentences longer, and it no longer states a layout rule for a domain that does not exist yet. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
The comment explained the content-type header by naming the Python and Java clients. That goes stale as more languages land, so state the general fact instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Both Rust paragraphs opened by pointing at the Java section above them. That shape repeats for every language added later, so say what Rust does instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
The committed data.json already records every violation the run found, with the same id and context. No scenario on main declares one, so these scenarios now follow that pattern. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
rustfmt.toml was the only language-specific file at the repository root. It set newline_style = "Unix", which guards against a file written on Windows landing with CRLF. Moving it under scenarios/http/rust would have narrowed it. rustfmt looks for its config by walking up from each source file, not by following Cargo workspace membership, so the three shared crates under tools/ would have stopped picking it up. Each further domain that adds Rust would also need its own copy. Git solves the same problem for every language at once, so this drops the Rust-only setting and configures line endings in .gitattributes instead. Batch files keep their CRLF because Gradle generates gradlew.bat that way. 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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: `LayoutError` and `FileNotFoundError` can currently escape from `main()` (e.g., missing Cargo/Rust toolchain or running before a build), producing a traceback instead of a clear CLI error. Catch these expected failures, print a concise message to stderr, and return a non-zero exit code so CI/setup logs stay actionable. Analysis: The CLI called package discovery and external commands without handling expected setup failures. Catching each expected exception at the command boundary keeps library helpers precise while giving command-line users a concise error. Upsides: Missing manifests, Cargo, and built binaries now produce actionable stderr messages and a non-zero return code without a traceback. Tests cover both exception paths. Downsides: The CLI no longer exposes traceback details for these expected failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: When `FileNotFoundError` comes from trying to execute the scenario binary (e.g., `otel-conformance-rust run` before `build`), the current message prints the full absolute path and gives no hint that running `build` first is required. This makes the most common “missing executable” case hard to diagnose. Consider special-casing missing `cargo` vs missing scenario binary, and print a more actionable message for the latter. Copilot comment: `target_directory()` runs `cargo metadata` with `check=True` while also capturing stdout/stderr. If `cargo metadata` exits non-zero (invalid lockfile, broken manifest, etc.), Python raises `CalledProcessError` and the CLI will crash with a traceback while also hiding Cargo’s error output, instead of reporting a clean CLI error. Prefer `check=False` and convert non-zero exit codes / non-JSON output into a `LayoutError` that includes stderr/stdout, consistent with other subprocess usage in this repo. Analysis: The CLI needs to distinguish a missing Cargo command from an unbuilt scenario and must convert every Cargo metadata failure into the existing user-facing error path. The metadata function now checks exit status and JSON parsing explicitly while preserving Cargo diagnostics. Upsides: Running before `build` now tells users how to fix the problem. Missing Cargo remains clear. Cargo metadata failures and malformed output no longer produce tracebacks or hide their diagnostic text. Downsides: Cargo metadata output is copied into the CLI error and can span several lines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment:
If `wait_for_eof()` returns an error (I/O error or JoinError from `spawn_blocking`), this function returns early and never stops the Actix server task. That can leave the server running on error paths and potentially keep the process alive or leak resources. Stop the server regardless, then propagate the EOF error after shutdown.
```
actix_web::rt::task::spawn_blocking(otel_conformance_scenario::wait_for_eof).await??;
handle.stop(true).await;
Ok(())
```
Analysis: Applying the result operators before shutdown lets either the blocking task or stdin read error bypass the server stop. Saving the nested result allows shutdown to finish before the same errors propagate.
Upsides: The Actix server now stops on both successful and failed EOF waits, which prevents a server task from surviving an error path.
Downsides: Error reporting waits for graceful server shutdown to complete.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: The runner sets OTEL_METRIC_EXPORT_INTERVAL to an effectively-infinite value (so metrics are exported only on the end-of-scenario flush). This SDK bootstrap builds a periodic metrics exporter without consulting that env var, so long-running scenarios may export metrics mid-run (and diverge from the runner’s “flush-at-end” expectation). Consider wiring OTEL_METRIC_EXPORT_INTERVAL into the periodic reader interval (or switching to an autoconfig path that already honors it) to keep Rust behavior consistent with other languages. Analysis: Rust has no SDK autoconfiguration crate in this setup, so the bootstrap must apply the shared runner setting when it constructs the periodic reader. A matching fallback preserves direct scenario runs outside the runner. Upsides: Rust scenarios now defer metric export until shutdown under the runner, matching the other language scenarios. Invalid interval values produce a named configuration error, and unit tests cover parsing. Downsides: The bootstrap owns one more environment-variable mapping that future SDK autoconfiguration could replace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
…ed with CRLF Review finding: The comment above `* text=auto eol=lf` states an invariant this repository does not satisfy: "Git stores every text file with LF and checks it out with LF. A file written on Windows is normalized on commit, so CRLF never reaches the repository." Five tracked files are stored in the repository with CRLF today: `scenarios/http/dotnet/global.json`, `scenarios/http/java/gradlew.bat`, `tools/dotnet/pyproject.toml`, `tools/dotnet/src/otel_conformance_dotnet/__init__.py`, and `tools/dotnet/tests/test_dotnet_scenario.py`. `text=auto` deliberately does not renormalize a blob that was already committed with CRLF, so adding this file leaves those four non-`.bat` files stored with CRLF indefinitely, and `git status` stays clean. Fix: run `git add --renormalize .` and commit the resulting files so the stated invariant holds, or reword the comment to say the policy applies to newly committed content rather than claiming CRLF is absent from the repository. Analysis: `text=auto` normalizes content as it is committed; it leaves a blob that already holds CRLF alone, which is why the working tree stays clean after this file appears. The comment described the rule as an invariant over the whole repository, and four .NET files plus the Gradle batch file already contradict it. Rewording keeps the change inside what this pull request does, whereas renormalizing those .NET files would rewrite content this pull request has no reason to touch. Upsides: The comment now describes what the attribute actually does, so a reader who finds a CRLF file in the repository does not conclude the setting is broken, and it names `git add --renormalize` as the way to convert one. Downsides: Those four .NET files stay stored with CRLF until someone renormalizes them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
…y other language Review finding: The two Rust commands are listed as `awc/.../client` then `actix-web/.../server`, which breaks the ordering every other language block in this list follows. Within each language the list is sorted by instrumented library: js is `express`, `http`, `http`, `undici`; dotnet is `aspnetcore`, `httpclient`; python is `aiohttp` through `wsgi` in alphabetical order. Sorted the same way, `actix-web/opentelemetry-actix-web/server` belongs before `awc/opentelemetry-actix-web/client`. Fix: swap the two Rust lines. Analysis: Every language block in this list is sorted by the instrumented library directory, and later additions were inserted into that order rather than appended, so the ordering is maintained rather than accidental. `actix-web` sorts before `awc`. The workspace `members` list in `scenarios/http/rust/Cargo.toml` and the directory listing in `scenarios/http/rust/README.md` already put Actix Web first, so this list was the one place that disagreed. Upsides: A reader looking for a scenario finds it where the surrounding list says it will be, and the three places this pull request lists the two Rust packages now agree. 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>
Copilot comment: `workspace_root()` only searches for a `[workspace]` manifest in parent directories, but Cargo also supports workspace membership via `workspace = "..."` in a package manifest (common for workspace members located outside the workspace root). This means valid workspace members like `tools/rust/scenario/Cargo.toml` (which declares `workspace = "../../../scenarios/http/rust"`) will be rejected with “no workspace Cargo.toml at or above …” even though `cargo metadata`/`cargo build` would succeed. Consider honoring the package-level `workspace` field (or deriving the workspace root from `cargo metadata`) before falling back to the upward directory search, so the launcher works for out-of-tree workspace members too. Analysis: `workspace_root()` rejected valid external workspace members because it only walked parent directories. It now resolves the package's explicit `workspace` path before continuing the parent search and checks that the referenced manifest declares a workspace. Upsides: The launcher accepts Cargo's out-of-tree workspace layout, including the repository's shared Rust scenario crate. A focused test protects that layout. Downsides: A malformed explicit workspace path now reports that manifest immediately instead of continuing into an unrelated parent workspace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
…ally is
Review finding:
Both new `Set up Rust` steps rely on `actions-rust-lang/setup-rust-toolchain`'s default `cache: true`, and neither sets `cache-workspaces`. The action passes `workspaces: ${{ inputs.cache-workspaces || inputs.rust-src-dir }}` to `Swatinem/rust-cache`, and both inputs are empty here, so rust-cache falls back to its own default, `const workspacesInput = core.getInput("workspaces") || ".";`. That resolves the cached workspace to the repository root and the cached target directory to `<repo root>/target`. The only Cargo workspace is `scenarios/http/rust`, and its build output goes to `scenarios/http/rust/target`, so nothing that `cargo clippy`, `cargo test`, or the scenario `setup: otel-conformance-rust build` compiles is ever saved or restored. Only `~/.cargo` is cached, so both the `lint` job and the `rust` scenarios job recompile actix-web, tonic, and the OpenTelemetry crates from source on every run. Every other language setup step in this workflow scopes its cache explicitly: `cache-dependency-glob` for uv and `cache-dependency-path` for Node. Fix: add `cache-workspaces: scenarios/http/rust` to the `Set up Rust` step in the `lint` job and to the one in the `scenarios` job.
Analysis: The action turns caching on by default and forwards an empty `cache-workspaces` to Swatinem/rust-cache, which then resolves the workspace to the repository root and caches `<repo root>/target`. No Cargo command writes there, so both Rust jobs restored a cache that held none of their build output and recompiled the whole dependency tree every run. Naming the workspace points the cache at `scenarios/http/rust/target`, matching how the uv and Node setup steps in the same workflow scope their own caches.
Upsides: The lint job and the Rust scenarios job reuse compiled dependencies between runs instead of rebuilding actix-web, tonic, and the OpenTelemetry crates from source. The cache key now also tracks the manifests and lockfile of the workspace it caches.
Downsides: The path is stated in two more places, so a second Rust workspace or a move of this one has to update them.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: `shutdown` errors are silently dropped when `run()` fails because `result?` returns early before evaluating `shutdown?`. That can hide SDK flush/shutdown failures during scenario errors. Capture both results and always check `shutdown` before returning the `run` result (or otherwise surface both). Copilot comment: `shutdown` errors are silently dropped when `run()` fails because `result?` returns early before evaluating `shutdown?`. That can hide SDK flush/shutdown failures during scenario errors. Capture both results and always check `shutdown` before returning the `run` result (or otherwise surface both). Analysis: Both entry points saved the scenario result but returned it before checking the SDK shutdown result. Checking shutdown first ensures an SDK flush or shutdown failure is surfaced, then returns the saved scenario result when shutdown succeeds. Upsides: SDK shutdown failures are no longer hidden by scenario failures, and both entry points use the same error precedence. Downsides: When the scenario and SDK shutdown both fail, the shutdown error takes priority and the scenario error is not returned. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
…t failed to start
Review finding:
`main` decides which executable was missing from `error.filename`, but Windows never sets it: on Windows `subprocess` raises `FileNotFoundError` out of `_winapi.CreateProcess`, which fills in `winerror` and `strerror` and leaves `filename` as `None`. Running `subprocess.call(["definitely-not-real-xyz-abc"])` on Windows confirms it: `filename=None`, `winerror=2`. So on Windows `missing` falls back to the literal string `required executable`, the `cargo` branch is unreachable, and both remaining branches are wrong. `otel-conformance-rust build` with no cargo on PATH prints `required executable was not found`, and `otel-conformance-rust run` with no cargo on PATH prints `scenario binary was not found; run `otel-conformance-rust build` first` even though what actually failed was starting `cargo metadata` inside `target_directory`, sending the reader to a build that cannot work either. Windows is a first-class target here: the repository runs this package's tests on `windows-latest`, `binary()` appends `.exe`, and `scenarios/http/README.md` says "the same package declaration works on Windows because the launcher adds `.exe` there". `tools/rust/tests/test_rust_scenario.py` hides the gap because both of its `missing` helpers raise `FileNotFoundError(2, "No such file or directory", command[0])`, supplying a filename that a real Windows failure never carries. Fix: name the program from the command that was about to run instead of from `error.filename`. Bind `command` to an empty list before the `try` block, and in the handler treat an empty `command` (the `cargo metadata` call inside `target_directory` never started) or a `command[0]` whose file name is `cargo` or `cargo.exe` as "cargo was not found", and anything else as the missing scenario binary. Then drop the fabricated filename from both test helpers so they exercise what Windows really raises.
Analysis: `FileNotFoundError.filename` is set only by the POSIX spawn path in CPython. On Windows the error comes straight from `CreateProcess`, so the launcher had no way to tell cargo apart from a scenario binary and picked its message from `arguments.command` alone. Every call site already knows which program it is about to start, so the message now comes from the call rather than from the exception. `target_directory` starts `cargo metadata` and gets its own handler, and the single `subprocess.call` carries the message that matches the command it was given: cargo for `build`, the scenario binary for `run`. The `f"{missing} was not found"` branch disappears with the filename it depended on. Both test helpers now raise the two-argument `FileNotFoundError` that Windows really produces, and a new test covers `run` failing before it reaches the binary. Verified on Windows with an empty PATH: `build` and `run` both report `cargo was not found`, where they previously reported `required executable was not found` and `scenario binary was not found; run `otel-conformance-rust build` first`.
Upsides: The diagnostic is correct on every platform rather than only on Linux and macOS. A missing cargo during `run` no longer sends the reader to a build step that cannot work either. The tests now assert against the exception shape Windows actually raises, so the Windows leg of the `Test` job would catch a regression.
Downsides: `main` gains a nested `try` and a small `_fail` helper, so the two failure messages sit slightly further from the calls that print them.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: `main()` strips all arguments after `run` before handing them to argparse, which prevents `otel-conformance-rust run --help/-h` (and other standard argparse flags) from working because they are forwarded to the scenario binary instead of triggering CLI help. Consider using `parse_known_args()` so argparse can still handle its own flags while leaving unknown flags to be passed through only for the `run` subcommand. Analysis: The launcher now lets argparse process its standard flags. It passes only unknown `run` arguments to the scenario. Other subcommands still reject unknown arguments. Upsides: `run --help` and `run -h` show launcher help. Scenario-specific arguments still reach the binary unchanged. Downsides: Scenario binaries can no longer receive `--help` or `-h` through the launcher. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment:
`cache-workspaces` is hard-coded to `scenarios/http/rust`, but this job already parameterizes the scenario root via `SCENARIO_ROOT` (and discovers `{domain, language}` pairs dynamically). Using the matrix-driven path here avoids future breakage if Rust scenarios are added under another domain.
```
# The Cargo workspace, which is not the repository root the action's
# cache assumes: without this it saves a `target/` no build writes.
cache-workspaces: scenarios/http/rust
```
Analysis: The scenario matrix already sets `SCENARIO_ROOT` for each domain and language pair. The Rust setup now uses that value as its Cargo cache workspace.
Upsides: Rust scenarios in any discovered domain cache the workspace they build. The cache path stays aligned with the scenario root used by the rest of the job.
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>
Copilot comment: `require()` uses `env::var(name).ok()`, which collapses `VarError::NotUnicode` into the same path as a missing variable. That makes the error message/docstring inaccurate (the variable can be present but non-UTF8). Consider using `env::var_os()` + `to_string_lossy()` so non-Unicode env vars don’t get misreported as missing/blank. Analysis: `require()` now reads the operating system value and converts it with replacement characters when needed. It still rejects absent and blank values. Upsides: A present non-Unicode value is no longer reported as missing. A Unix test covers the lossy conversion path. Downsides: Invalid Unicode is replaced before callers parse the value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Copilot comment: `DEFAULT_METRIC_EXPORT_INTERVAL_MILLIS` uses the i32::MAX millisecond value but the rationale isn’t documented here, making it look like an arbitrary magic number. The Python launcher documents this as an “effectively infinite” interval to prevent periodic metric export from splitting a scenario’s metrics across reports; mirroring that rationale here (and expressing the value as i32::MAX) would make the intent clear and keep implementations aligned. Analysis: The constant now uses `i32::MAX` and explains why the interval must outlast the scenario. The shutdown flush remains responsible for exporting the metrics. Upsides: The Rust fallback states the same intent as the other launchers. The named limit replaces an unexplained numeric literal. Downsides: The cast is required because `Duration::from_millis()` accepts `u64`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
…nation Review finding: This new Rust paragraph is inserted in the middle of the Java explanation, which leaves the sentence after it pointing at the wrong antecedent. Before this PR the section read: the `java/armeria/...` tree, then "The `main` classes are per instrumentation because attaching library instrumentation is code rather than a command-line flag; everything they do beyond that is in `scenarios/`.", then "Armeria therefore has four isolated packages: client and server coverage for both the OpenTelemetry Java agent and explicit OpenTelemetry library instrumentation." The word "therefore" drew its conclusion from the sentence immediately before it. This PR puts six lines about Rust crates between them, so line 44 now reads as though Armeria has four isolated packages because of how Rust splits its crates. The document also introduces each language once, in the order Java, .NET (line 60), Python (line 66); Rust is the only one that interrupts another language's explanation instead of following them. Fix: move the Rust paragraph out of the Java block and place it after the Python paragraph, so the Java narrative runs uninterrupted from `main` classes to "Armeria therefore has four isolated packages" and the per-language order stays Java, .NET, Python, Rust. Analysis: The HTTP README introduces each language once, and the Java explanation runs from the `java/armeria/...` tree through the `main` classes sentence to "Armeria therefore has four isolated packages". The Rust paragraph landed between the last two, so "therefore" appeared to draw its conclusion from Rust crate layout. Moving the paragraph after the Python one restores the Java chain and puts Rust where the other languages already are, in the order Java, .NET, Python, Rust. The paragraph itself is unchanged and reads the same in its new place, since it never referred to anything around it. Upsides: "Armeria therefore" follows the sentence it concludes from again. Each language is introduced once, in one order, so a reader looking for Rust finds it beside .NET and Python rather than inside the Java section. Downsides: No material downside identified. The move is six lines of prose with no reference into or out of it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Review finding: The `run` subcommand's help text promises "remaining words are passed through", but this launcher does not pass every remaining word through. `main` calls `parser.parse_known_args(words)`, so the `run` subparser consumes its own `-h`/`--help` (and any unambiguous prefix such as `--hel`) and prints the launcher's usage instead of forwarding it. The PR's own test `test_run_help_is_handled_by_the_launcher` asserts exactly that: `main(["run", "--help"])` exits 0 after printing "usage: otel-conformance-rust run". The sibling launcher `tools/dotnet/src/otel_conformance_dotnet/__init__.py` states the accurate promise for its own behavior, "run the published scenario; what follows is the scenario's own", and it really does split every word after `run` off before argparse sees them. Fix: reword this help string so it matches what the code does, for example "run the compiled scenario; unrecognized words are passed through". Analysis: `main` parses with `parse_known_args`, so the `run` subparser keeps `-h`, `--help`, and any unambiguous prefix of them for itself and forwards only what it does not recognize. The help line said "remaining words are passed through", which a reader of `otel-conformance-rust --help` takes as every word after `run`. Naming unrecognized arguments instead describes what argparse actually forwards, and saying "passed to it" names the scenario as the destination. Only the help string changes; the parsing behavior and the tests that pin it stay as they are. Upsides: The help output matches what the launcher does, so someone who runs `otel-conformance-rust run --help` and gets launcher usage is not reading an exception the help denied. Downsides: The wording no longer matches the sibling .NET launcher's help line, but that launcher really does forward every word after `run`, so the two lines differ because the two launchers do. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: trask <218610+trask@users.noreply.github.com>
Review finding: In the layout block this file adds, six of the seven rows start their description at column 48, but this row starts it at column 47. `actix-web/opentelemetry-actix-web/server/` is followed by five spaces, one fewer than it needs to line up with the six rows around it. The block is a two-column table, so this one short row breaks the alignment a reader scans down. Fix: add one space after `actix-web/opentelemetry-actix-web/server/` so its description begins in the same column as the other six. Analysis: The fenced block in scenarios/http/rust/README.md is a two-column table of workspace members and what each one is. Measured on the raw bytes, six of its seven rows put the description at one-based column 48, while the `actix-web/opentelemetry-actix-web/server/` row put it at column 47. Adding one space brings all seven rows to column 48. The same convention holds in the repository's other layout blocks: every row in scenarios/http/README.md starts its description at column 42, and every row in tools/rust/README.md at column 17. Upsides: The block reads as the aligned two-column table it is meant to be, so the eye follows one column down instead of catching on a row that sits a character to the left. Downsides: No material downside identified. The change is one space inside a fenced text block, so it alters no rendered meaning and no code. 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>
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
force-pushed
the
trask-http-rust-conformance
branch
from
October 26, 2026 01:01
15af220 to
61aec5e
Compare
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 separate Rust conformance scenarios for an Actix Web server and an awc client, both instrumented with
opentelemetry-instrumentation-actix-web0.24.0. The server coverage includes HTTP metrics as well as spans.The recorded coverage exposes three instrumentation findings:
GET 127.0.0.1, instead of the required method-only name.network.protocol.versionattribute.error.type="404"for the 404 response but leave the span status unset.The change also adds the shared Rust launcher, scenario lifecycle, OpenTelemetry SDK bootstrap, and HTTP contract support needed to build and run these scenarios in CI.
It also extends the repository-wide
.gitattributeswith LF normalization for newly committed text and LF checkouts, so no language needs its own line-ending setting. Files already stored with CRLF stay unchanged until explicitly renormalized. Batch files are excluded, which leaves Gradle'sgradlew.batexactly as it is. The explicit LF rule for Composer manifests is preserved.cc @open-telemetry/rust-approvers