Repository navigation
Conversation
Capture raw OTLP for isolated scenario assertions while forwarding the package stream to one Weaver live check. Keep HTTP servers alive across sequential contract actions and pass selected action JSON through the runner. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reconciliation failures can erase coverage, and overlapping required/optional metrics create impossible checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves conformance execution to package-wide Weaver checks with isolated OTLP capture windows and persistent HTTP server batches.
Changes:
- Adds package-level execution, capture, reporting, and violation reconciliation.
- Introduces JSON action handoff and persistent
jsonl-v1HTTP execution. - Migrates HTTP contracts, helpers, package declarations, tests, and documentation.
File summaries
| File | Description |
|---|---|
.github/workflows/runner.yml |
Installs capture dependencies and uploads telemetry reports. |
.gitignore |
Updates generated-report description. |
tools/runner/src/opentelemetry/conformance/__init__.py |
Exports package and capture APIs. |
tools/runner/src/opentelemetry/conformance/_checks.py |
Checks captured telemetry and package findings. |
tools/runner/src/opentelemetry/conformance/_cli.py |
Runs and reports package-wide checks. |
tools/runner/src/opentelemetry/conformance/_coverage.py |
Reduces normalized captures. |
tools/runner/src/opentelemetry/conformance/_env.py |
Defines JSON action handoffs. |
tools/runner/src/opentelemetry/conformance/_otlp_capture.py |
Captures and forwards OTLP exports. |
tools/runner/src/opentelemetry/conformance/_persistent.py |
Implements persistent action windowing. |
tools/runner/src/opentelemetry/conformance/_report.py |
Persists normalized capture bundles. |
tools/runner/src/opentelemetry/conformance/_semconv.py |
Reduces captured semantic-convention data. |
tools/runner/src/opentelemetry/conformance/_server.py |
Supports injected server environments. |
tools/runner/src/opentelemetry/conformance/_session.py |
Manages package execution and finalization. |
tools/runner/src/opentelemetry/conformance/_spec.py |
Adds protocols, variants, actions, and package additions. |
tools/runner/src/opentelemetry/conformance/pytest_plugin.py |
Collects one atomic package test. |
tools/runner/tests/test_cli.py |
Tests package-level CLI behavior. |
tools/runner/tests/test_http_packages.py |
Validates HTTP package migration. |
tools/runner/tests/test_pytest_plugin.py |
Tests atomic pytest execution. |
tools/runner/tests/test_report.py |
Tests normalized report persistence. |
tools/runner/tests/test_semconv.py |
Adapts semantic coverage tests. |
tools/runner/tests/test_spec.py |
Tests new specification features. |
tools/runner/README.md |
Documents package-wide execution. |
tools/runner/pyproject.toml |
Adds OTLP capture dependencies. |
tools/http/test-client/README.md |
Documents injected action execution. |
tools/http/test-client/contract.yaml |
Adds client/server expectation variants. |
tools/http/test-client/python/src/otel_http_test_client/__init__.py |
Decodes injected JSON actions. |
tools/http/test-client/js/src/index.js |
Updates helper description. |
tools/http/test-client/js/src/contract.js |
Replaces YAML loading with JSON decoding. |
tools/http/test-client/js/test/actions.js |
Adds test action fixtures. |
tools/http/test-client/js/test/client-workload.test.js |
Tests selected JSON actions. |
tools/http/test-client/js/test/contract.test.js |
Tests action decoding and validation. |
tools/http/test-client/js/test/server-workload.test.js |
Injects server action fixtures. |
tools/http/test-client/js/README.md |
Documents JSON action handling. |
tools/http/test-client/js/package.json |
Removes YAML dependency. |
tools/http/test-client/js/package-lock.json |
Removes locked YAML package. |
tools/http/test-client/java/src/main/java/io/opentelemetry/conformance/http/HttpContract.java |
Decodes and validates JSON actions. |
tools/http/test-client/java/src/main/java/io/opentelemetry/conformance/http/HttpServerWorkload.java |
Supports injected exchanges. |
tools/http/test-client/java/src/test/java/io/opentelemetry/conformance/http/TestActions.java |
Adds Java action fixtures. |
tools/http/test-client/java/src/test/java/io/opentelemetry/conformance/http/HttpClientWorkloadTest.java |
Uses injected test exchanges. |
tools/http/test-client/java/src/test/java/io/opentelemetry/conformance/http/HttpContractTest.java |
Tests JSON contract decoding. |
tools/http/test-client/java/src/test/java/io/opentelemetry/conformance/http/HttpServerWorkloadTest.java |
Tests injected server exchanges. |
tools/http/test-client/java/build.gradle.kts |
Removes YAML support. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http/HttpContract.cs |
Decodes and validates JSON actions. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http/HttpServerWorkload.cs |
Supports injected exchanges. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http/OpenTelemetry.Conformance.Http.csproj |
Removes embedded YAML support. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http.Tests/TestActions.cs |
Adds .NET action fixtures. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http.Tests/HttpClientWorkloadTests.cs |
Uses injected test exchanges. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http.Tests/HttpContractTests.cs |
Tests JSON action decoding. |
tools/http/test-client/dotnet/OpenTelemetry.Conformance.Http.Tests/HttpServerWorkloadTests.cs |
Tests injected server exchanges. |
tools/http/test-client/dotnet/Directory.Packages.props |
Removes YamlDotNet. |
tools/http/test-client/dotnet/README.md |
Documents JSON action handling. |
tools/http/runner/README.md |
Documents persistent HTTP execution. |
scenarios/http/README.md |
Documents package-wide HTTP checks. |
scenarios/http/python/wsgi/opentelemetry-wsgi/server/conformance.yaml |
Migrates WSGI server package. |
scenarios/http/python/urllib3/opentelemetry-urllib3/client/conformance.yaml |
Selects client expectations. |
scenarios/http/python/urllib/opentelemetry-urllib/client/conformance.yaml |
Selects client expectations. |
scenarios/http/python/tornado/opentelemetry-tornado/server/conformance.yaml |
Migrates Tornado server package. |
scenarios/http/python/starlette/opentelemetry-starlette/server/conformance.yaml |
Migrates Starlette server package. |
scenarios/http/python/requests/opentelemetry-requests/client/conformance.yaml |
Selects client expectations. |
scenarios/http/python/pyramid/opentelemetry-pyramid/server/conformance.yaml |
Migrates Pyramid server package. |
scenarios/http/python/httpx/opentelemetry-httpx/client/conformance.yaml |
Selects client expectations. |
scenarios/http/python/flask/opentelemetry-flask/server/conformance.yaml |
Migrates Flask server package. |
scenarios/http/python/fastapi/opentelemetry-fastapi/server/conformance.yaml |
Migrates FastAPI server package. |
scenarios/http/python/falcon/opentelemetry-falcon/server/conformance.yaml |
Migrates Falcon server package. |
scenarios/http/python/django/opentelemetry-django/server/conformance.yaml |
Migrates Django server package. |
scenarios/http/python/asgi/opentelemetry-asgi/server/conformance.yaml |
Migrates ASGI server package. |
scenarios/http/python/aiohttp/opentelemetry-aiohttp-server/server/conformance.yaml |
Migrates aiohttp server package. |
scenarios/http/python/aiohttp/opentelemetry-aiohttp-client/client/conformance.yaml |
Selects client expectations. |
scenarios/http/js/undici/opentelemetry-undici/client/conformance.yaml |
Selects client expectations. |
scenarios/http/js/http/opentelemetry-http/server/conformance.yaml |
Migrates Node HTTP server package. |
scenarios/http/js/http/opentelemetry-http/client/conformance.yaml |
Selects client expectations. |
scenarios/http/js/express/opentelemetry-express/server/conformance.yaml |
Migrates Express server package. |
scenarios/http/java/vertx-http-client/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/undertow/opentelemetry-javaagent/server/conformance.yaml |
Migrates Undertow server package. |
scenarios/http/java/tomcat/opentelemetry-javaagent/server/conformance.yaml |
Migrates Tomcat server package. |
scenarios/http/java/spring-webflux/opentelemetry-javaagent/server/conformance.yaml |
Migrates WebFlux server package. |
scenarios/http/java/spring-webflux/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/servlet/opentelemetry-javaagent/server/conformance.yaml |
Migrates Servlet server package. |
scenarios/http/java/restlet/opentelemetry-javaagent/server/conformance.yaml |
Migrates Restlet server package. |
scenarios/http/java/reactor-netty/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/ratpack/opentelemetry-javaagent/server/conformance.yaml |
Migrates Ratpack server package. |
scenarios/http/java/ratpack/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/pekko-http/opentelemetry-javaagent/server/conformance.yaml |
Migrates Pekko server package. |
scenarios/http/java/okhttp/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/netty/opentelemetry-javaagent/server/conformance.yaml |
Migrates Netty server package. |
scenarios/http/java/netty/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/jodd-http/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/jetty-httpclient/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/java-http-server/opentelemetry-javaagent/server/conformance.yaml |
Migrates Java HTTP server package. |
scenarios/http/java/java-http-client/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/http-url-connection/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/helidon/opentelemetry-javaagent/server/conformance.yaml |
Migrates Helidon server package. |
scenarios/http/java/grizzly/opentelemetry-javaagent/server/conformance.yaml |
Migrates Grizzly server package. |
scenarios/http/java/async-http-client/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/armeria/opentelemetry-library/server/conformance.yaml |
Migrates Armeria library server package. |
scenarios/http/java/armeria/opentelemetry-library/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/armeria/opentelemetry-javaagent/server/conformance.yaml |
Migrates Armeria agent server package. |
scenarios/http/java/armeria/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/apache-httpclient/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/apache-httpasyncclient/opentelemetry-javaagent/client/conformance.yaml |
Selects client expectations. |
scenarios/http/java/akka-http/opentelemetry-javaagent/server/conformance.yaml |
Migrates Akka HTTP server package. |
scenarios/http/java/gradle/libs.versions.toml |
Removes Jackson YAML dependency. |
scenarios/http/dotnet/httpclient/opentelemetry-http/client/conformance.yaml |
Selects client expectations. |
scenarios/http/dotnet/httpclient/opentelemetry-http/client/data.json |
Updates captured metric coverage. |
scenarios/http/dotnet/aspnetcore/opentelemetry-aspnetcore/server/conformance.yaml |
Migrates ASP.NET Core server package. |
scenarios/database/contracts/postgresql.yaml |
Removes unrelated queue metric expectations. |
scenarios/database/contracts/mariadb.yaml |
Removes unrelated queue metric expectations. |
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
- Files reviewed: 111/113 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pull request dashboard statusWaiting on the author · refreshed 2026-10-17 01:48 UTC Move out of draft to request review. Status above doesn't look right?
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
|
What is the benefit of performing package-wide telemetry checks? |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep what both sides meant to do in every conflicted file. .github/workflows/runner.yml: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. scenarios/http/README.md: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/http/test-client/README.md: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/http/test-client/python/src/otel_http_test_client/__main__.py: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/runner/pyproject.toml: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/runner/src/opentelemetry/conformance/_session.py: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/runner/src/opentelemetry/conformance/_spec.py: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/runner/tests/test_session.py: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation. tools/runner/tests/test_spec.py: The head side introduces package-wide telemetry capture, independent client and server contracts, JSON action handoffs, and package report paths. The base side adds Ruby, PHP, and Go HTTP scenarios, OTLP/HTTP transport, and graceful server cleanup. The resolution keeps the package-wide runner model while routing OTLP/HTTP through the capture proxy, preserving EOF-first cleanup, and including every added language in CI and documentation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: When final persistent partitioning fails (for example, on an ambiguous metric interval), the controller returns empty per-scenario windows, but `result.executed` remains true here. That marks the package complete, overwrites each capture with an empty report, and lets `_write_data()` replace `data.json` with empty coverage even though raw telemetry was received. Preserve the failed batch capture and/or keep the run incomplete when reconciliation fails so a fail-closed run cannot erase observed coverage. Analysis: Final reconciliation applies an empty partition to every result when captured telemetry cannot be assigned safely. Marking those results as executed makes the session treat that empty fallback as complete coverage. The controller now marks every result incomplete when reconciliation fails, so the session does not rebuild data.json from empty windows. Upsides: A fail-closed persistent run preserves the previous coverage data and reports the reconciliation failure without treating the package as complete. Downsides: The per-scenario diagnostic captures remain empty because ambiguous telemetry cannot be assigned safely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: An optional metric can also already be required by the contract (or be listed once bare and once with `required: false`). Subtracting every optional name then makes that required metric appear missing even when it was emitted, so the accepted configuration can never pass. Only subtract optional names that are not also required (or reject the overlap while loading the spec). Analysis: The accepted specification can place one metric in both sets. Optionality should permit an otherwise undeclared metric, but it must not remove a required declaration. The metric check now subtracts only names that are optional and not required. Upsides: Overlapping declarations remain satisfiable when emitted and still fail when the required metric is absent. Downsides: Overlapping declarations remain accepted instead of being rejected as redundant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Action-table caches can return stale contracts after validation failures, PHP accepts invalid HTTP statuses, and persistent attribution uses the wrong request boundary.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
- Files reviewed: 157/161 changed files
- Comments generated: 4
- Review effort level: Balanced
Copilot comment: The implementation does not currently isolate on the instant the driver sent the HTTP request as documented here. `PersistentController` records `action.sent_unix_nano` when the runner writes the control record, later receives the driver's `started_unix_nano` as `requested_unix_nano`, but `partition_persistent_exports()` builds boundaries from `sent_unix_nano`. Telemetry stamped during that IPC gap is therefore attributed to the new action even though its request has not started, which can contaminate its exact checks. Build action boundaries from the driver-reported request start instead. Analysis: The control-record send precedes the measured HTTP request and cannot define its telemetry window. Partitioning now uses the driver's request-start timestamp when available and keeps the control-record timestamp only as a fallback for actions that fail before reporting request timing. Upsides: Telemetry emitted during inter-process control delivery remains in the preceding window, so exact action checks start when the measured request starts. Downsides: Failed actions without a driver timestamp still use the less precise control-record timestamp for diagnostic partitioning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: The cache key is updated before `decodeTable()` succeeds. After a valid table has populated `$exchanges`, one failed decode leaves that old value paired with the invalid raw string, and the next call with the same invalid input returns stale exchanges instead of rejecting it. Publish both cache fields only after validation completes. Copilot comment: The cache key is published before every action has been validated. If parsing this table raises after a previous valid table was cached, a second call with the same invalid JSON returns the previous `@exchanges` instead of raising, so the helper can silently serve traffic from the wrong contract. Build the new table first and publish the key and value only after the whole parse succeeds. Analysis: Both helpers published a new raw cache key before all actions decoded successfully. A failed decode could therefore pair that new key with the old exchange list. Each helper now builds the complete exchange list before publishing either cache field. Upsides: Repeated invalid input consistently fails and can no longer retrieve exchanges from a different contract. Downsides: A successful decode temporarily holds the previous and replacement exchange lists until the cache assignment completes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot comment: Unlike the other action decoders, this accepts any integer response status, including 0 or 700. Because the runner only checks that an action is JSON-representable, a malformed/custom contract reaches this helper and is accepted even though the action schema requires an HTTP status in 100–599. Validate the range before constructing the exchange. Analysis: The PHP decoder checked only the value type, while the shared action contract limits HTTP response statuses to 100 through 599. It now rejects integers outside that range before constructing an exchange. Upsides: Malformed custom contracts fail consistently across language helpers instead of reaching the PHP workload with an invalid status. Downsides: No material downside identified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The broad cross-language lifecycle change and remaining PHP dependency cleanup warrant final human validation.
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
- Files reviewed: 158/162 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot comment: Removing the YAML reader leaves `symfony/yaml` as an unused runtime dependency in `composer.json` and `composer.lock`, unlike the dependency cleanup for the other language helpers. Every PHP scenario will still install Symfony YAML even though the package now consumes only JSON; remove that requirement and refresh the lock file. Analysis: The PHP contract helper now decodes only JSON, and no PHP source imports Symfony YAML. The direct requirement and its transitive packages have been removed from the helper lock and both scenario locks. Upsides: PHP scenarios no longer install unused YAML parsing packages, which reduces installation work and dependency exposure. Downsides: The regenerated path-package references change in both scenario lock files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Re-running a scenario can produce package-wide findings that no longer correspond to the overwritten per-scenario capture used for coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
- Files reviewed: 159/166 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot comment: `selected_names` may contain duplicates (the CLI's repeatable `--scenario` permits this), and already-run names are not rejected either. Re-running a scenario sends both executions to the package-wide Weaver process but overwrites the single `scenarios/<name>.json` capture; `_ran` is a set, so the run can still be considered complete and `data.json` is then built from only the last capture while its findings come from both executions. Reject duplicate/already-run scenarios (or preserve every execution under distinct report identities) before starting the batch. Analysis: A package session has one report identity per scenario, so it cannot represent repeated executions without losing one capture. Selection validation now rejects both duplicate names and names already recorded by the session before any selected scenario starts. Upsides: Scenario reports, package-wide findings, and reduced data always describe the same single execution of each scenario. Downsides: Callers can no longer intentionally rerun one scenario in the same package session and must open a new session instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The declared minimum protobuf dependency combination cannot execute the new capture serialization API.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
- Files reviewed: 159/166 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot comment: The new capture serialization calls `MessageToDict(..., always_print_fields_with_no_presence=True)`, but this dependency range still permits `opentelemetry-proto==1.27`, which requires protobuf 3/4; those protobuf releases do not support that keyword. A valid installation at the declared minimum will therefore fail with `TypeError` as soon as it writes a capture. Please raise the proto/protobuf minimums to a combination that provides this API (and declare protobuf directly because the runner imports it directly). Analysis: `always_print_fields_with_no_presence` is available in protobuf 5.26 and later, while protobuf 5.27 also imports correctly on the runner's Python 3.14 CI target. The runner now declares protobuf directly at 5.27 or later and requires the first OpenTelemetry proto line that accepts protobuf 5. Upsides: Every valid dependency resolution provides the serialization API the runner calls, and the direct protobuf import is represented in package metadata. Downsides: Installations pinned to OpenTelemetry proto 1.27 or protobuf 4 must upgrade. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-language lifecycle and timestamp-attribution changes warrant final human integration review despite no additional concrete defects found.
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
- Files reviewed: 160/166 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ession Review finding: This module-level import breaks the whole Test job. The job installs with `--no-deps` and the comment this pull request rewrites in `.github/workflows/runner.yml` still says test-utils is deliberately left out, so `opentelemetry.test.weaver_live_check` is not importable there. pytest cannot import `test_session.py`, reports `ModuleNotFoundError: No module named 'opentelemetry.test'`, and aborts collection for every listed test directory, so no runner test runs at all. Both `Test (ubuntu-latest, Python 3.14)` and `Test (windows-latest, Python 3.14)` fail this way on this head, and I reproduced it locally with the same install command. Only `test_weaver_inactivity_timeout_is_disabled_for_the_package` needs the module, because it patches `WeaverLiveCheck` on it and `_new_live_check` imports it lazily. Fix: drop the module-level import and give that one test a stub module registered in `sys.modules` under `opentelemetry.test.weaver_live_check`, so the lazy import inside `_new_live_check` resolves to the stub without the real package being installed. Analysis: `ConformanceSession._new_live_check` imports `WeaverLiveCheck` inside the method precisely so the runner does not need `opentelemetry-test-utils` at import time. A module-level `import opentelemetry.test.weaver_live_check` in the test file defeats that, because pytest imports the test module during collection and the collection error aborts the whole session for every directory on the command line, not just this file. Registering a `ModuleType` stub in `sys.modules` under the full dotted name satisfies the lazy `from ... import ...`: CPython finds the fully qualified name in `sys.modules` and never imports the `opentelemetry.test` parent package. `monkeypatch.setitem` restores `sys.modules` afterwards. Installing `opentelemetry-test-utils` in the Test job was the alternative, but it would defeat the dependency-light job the workflow comment describes and would stop that job catching an eager import like this one. Upsides: the Test job collects and runs again on both operating systems; the test still proves that `_new_live_check` passes `inactivity_timeout=0`; the dependency-light Test job keeps catching eager imports of the optional dependency. Downsides: the stub duplicates the constructor contract that the real `WeaverLiveCheck` defines, so a change to its signature would not be caught here. The Types job, which installs the real dependency, still checks that call site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…etattr Review finding: `ctypes.get_last_error` and `ctypes.FormatError` exist only on Windows, and pyright runs on Linux in the `Types` job, where `tools/http/test-client/python/src` is inside the strict `include` list. These two lines are the only source of that job's failure: seven errors at 303:5, 303:13, 303:20, 304:19, 304:40 twice, and 304:47, reported as `reportAttributeAccessIssue`, `reportUnknownVariableType`, and `reportUnknownMemberType`. The same function's caller already avoids exactly this, at line 238: `win_dll = getattr(ctypes, "WinDLL")`. Fix `_raise_windows_error` the same way, reading `get_last_error` and `FormatError` through `getattr` so the Windows-only attributes are not resolved statically. The runtime behaviour is identical. Analysis: `typeshed` guards `get_last_error` and `FormatError` with `sys.platform == "win32"`, and the root `pyproject.toml` sets no `pythonPlatform`, so the `Types` job resolves them against Linux and reports seven errors. `getattr` with a string name is a plain Python attribute lookup, not a Win32 call, so it cannot disturb the thread-local error code that `get_last_error` reads; `kernel32` is loaded with `use_last_error=True`, which is what makes that code available in the first place. Setting `pythonPlatform = "Windows"` would fix this file by weakening the analysis of every other file in the repository, and a `pyright: ignore` comment would itself become an error on Windows because `reportUnnecessaryTypeIgnoreComment` is enabled. Reading both names through `getattr` matches what the same class already does for `WinDLL`. Upsides: the `Types` job passes; the repository keeps checking every other file against Linux; the Windows Job Object code still reports the real Win32 error code and message. Downsides: pyright no longer checks the arguments or the return types of these two calls, so a mistake in either would surface only at runtime on Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review finding: `test_final_shutdown_telemetry_is_reconciled` can never pass. `PersistentController.run` reads `batch_end = time.time_ns()` before it calls `self._capture.drain(...)`, so the span this callback stamps during the drain is always later than the batch end the partition is reconciled against. The last window is `(start, batch_end)`, the span falls past it, and `partition_persistent_exports` raises `span is unassignable`. Running the test gives `assert 'span is unassignable: point=[...], actions=[(0, ...), (..., ...)]' is None`; the span's timestamps sit a few microseconds after the recorded batch end. Nothing about this depends on the platform or on timer resolution. CI has not caught it because the collection error in `test_session.py` aborts the run before `test_persistent.py` executes. The test double is the unrealistic part: real shutdown telemetry is stamped by the instrumentation before the measured process exits, and `batch_end` is read after that. Fix the fake to stamp the span at a moment inside the action window — for example capture `time.time_ns()` from an `on_snapshot` callback during the settle loop and emit that timestamp at drain — so the test still proves that telemetry delivered during the final drain is reconciled into the action that recorded it. Analysis: `PersistentController.run` closes the driver's stdin, reads `stopped`, waits for the process to exit, and only then reads `batch_end = time.time_ns()` and drains the capture. The old fake stamped its span from inside `drain`, which is strictly after `batch_end`, so `_assign_interval` could never place it in the last window. That ordering is correct for the product: the runner refuses to attribute telemetry stamped after the measured process is known to be gone, and moving `batch_end` past the drain would stretch the last window across forwarding latency and admit impossible post-exit timestamps. Real shutdown telemetry is stamped by the instrumentation while the process is still running and is only delivered later, when the exporter flushes. The driver now writes its shutdown timestamp after stdin reaches end of file and before it reports `stopped`, and the drain callback emits a span carrying that timestamp, which is what the capture would deliver. Upsides: the test passes deterministically on every platform, and it now proves what it claims, that telemetry stamped before exit but delivered during the final drain is reconciled into the action that recorded it. The product's refusal to bound a window on the runner's own clock is left alone. Downsides: the driver and the callback now share a temporary file, so the test is a little longer and depends on that handshake. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ent contract
Review finding:
Renaming `tools/http/test-client/contract.yaml` to this file leaves `tools/go/tests/test_go_scenario.py` reading the old path. `test_http_client_runs_each_contract_request_with_its_index` calls `checkout.joinpath("tools", "http", "test-client", "contract.yaml").read_text(...)` at line 127 and now fails with `FileNotFoundError`. That directory is in the Test job's pytest arguments, and I reproduced the failure locally.
The test is not in this pull request's diff, so nothing updated it with the rename. Fix: point it at `tools/http/contracts/client.yaml`.
Analysis: the HTTP client contract moved to `tools/http/contracts/client.yaml` when the client and server sides became separate contracts, and this test still read the old path. `tools/go/tests` is one of the directories the Test job passes to pytest, so the test runs there; the job has not reported it only because a collection error in another module aborted the whole run first. The renamed file still carries the same `scenarios` list, so `load_spec` supplies every field the test asserts on and repointing the path is the whole fix. The package-wide test in `tools/runner/tests` does not cover the Go run command, the per-scenario descriptions, or the indices this test checks, so deleting it would lose real coverage.
Upsides: the Go test suite passes again and keeps checking that each contract request reaches the Go client with its own index and description.
Downsides: No material downside identified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…equest Review finding: `_wait_for_bootstrap` returns the largest closing timestamp among the declared metrics, and `partition_persistent_exports` then refuses any later metric interval that straddles it. Instruments in one collection cycle do not share a timestamp: the Go SDK reads the clock separately for each aggregation, and each aggregation's next delta point starts at its own previous reading. An instrument stamped a few microseconds before the declared one therefore produces a following interval that begins just before this boundary and is rejected. The `Scenarios (http-go)` job fails exactly this way for every Go server package: `metric 'http.server.request.body.size' overlaps action boundaries [1789079178385559837]: point=[1789079178385510344, 1789079178485533083]`. The boundary is the readiness boundary from `http.server.request.duration`, and the rejected interval starts 49 microseconds earlier, in the same collection cycle. The first action fails and the rest of the batch is reported unexecuted. Fix: take the boundary from the earliest closing timestamp in the collection that closed at or after the readiness request, rather than the latest among the declared metrics, so no instrument in that cycle can start its next interval before the boundary. Analysis: the Go SDK's `pipeline.produce` walks the instruments one at a time, and each delta aggregation calls `now()` for itself, emits `[start, t]`, then sets `start = t`. One collection therefore ends the instruments of a single handler microseconds apart, and each instrument's next interval begins at its own reading. Ending readiness at the declared metric's reading puts the boundary in the middle of that spread, so a sibling instrument read earlier produces a following interval that straddles it. Taking the earliest reading instead only narrows the spread; it does not remove it, and it still misses an instrument that recorded nothing during readiness. What actually divides readiness from the first action is the readiness request, because every reading in the collection that answered it comes after it. `partition_persistent_exports` already documents `bootstrap_unix_nano` as "when the driver reported readiness", and the branch for a batch that declares no metric already returns exactly that, so both branches now agree on the boundary and differ only in what they wait for. The wait is what enforces the isolation: holding the first action until a declared metric has closed at or after readiness proves the collection carrying readiness is over, so no later point can aggregate readiness together with an action. Upsides: the Go server packages run their actions instead of aborting the batch on the first one. The boundary matches what the partition function documents, and the two branches of `_wait_for_bootstrap` no longer disagree about what the boundary means. A point that really does span readiness and the first action is still rejected, because it straddles the readiness request. Downsides: the boundary is now the runner's own reading of when it sent readiness rather than a timestamp the instrumentation reported, so it no longer detects a point that begins after readiness was sent and still aggregates the readiness measurement. Waiting for the declared metric to close is what rules that out, and it is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: Scenarios (http-go): undeclared metrics emitted: ['http.server.request.body.size', 'http.server.response.body.size'] Scenarios (http-php): timed out isolating readiness telemetry; expected one of ['http.server.request.duration'] to close at or after the readiness request Scenarios (http-ruby-net_http-opentelemetry-instrumentation-net_http-client): url.full expected on every span, set on 0/1 Cause: The package-wide checks required an optional HTTP metric, repeated semantic-convention validation in the request contract, and left package-specific Go metrics and Ruby internal spans undeclared. Fix: Allow contracts to declare optional metrics, leave url.full to the semantic-convention live check, and declare the extra telemetry each affected package emits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: Test (ubuntu-latest, Python 3.14): subprocess.TimeoutExpired: Command '['/opt/hostedtoolcache/Python/3.14.7/x64/bin/python', '-m', 'otel_http_test_client', '--persistent', '--serve', ...] timed out after 30 seconds Cause: SIGTERM entered the normal error cleanup path, which waited the full 30-second graceful shutdown timeout before killing an unresponsive descendant. Fix: Use a dedicated signal exit and kill the measured process tree immediately when the driver receives SIGTERM. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check:
Scenarios (http-dotnet): timed out isolating readiness telemetry; expected one of ['http.server.active_requests'] to close at or after the readiness request
Scenarios (http-php): 1 undeclared span(s): ['{closure}']
Cause: The readiness gate waited for an active-request UpDownCounter snapshot to close like an interval, and the Slim package did not declare its per-request internal closure span.
Fix: Let observed snapshot metrics satisfy readiness isolation because they cannot cross action boundaries, and declare Slim's internal span as package-specific telemetry.
Co-authored-by: Copilot <223556219+Copilot@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: Once the persistent Slim batch completed, package-wide reduction observed the request and response body-size attributes that the aborted run could not record. Fix: Commit the exact data.json blob generated by the pinned CI run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: Scenarios (http-dotnet): span overlaps action intervals: point=[1789084327321860200, 1789084327369560300], actions=[(0, 1789084327357355832), (1789084327357355832, 9223372036854775808)] Scenarios (http-php): the committed data.json files are out of date — run the scenarios and commit the result Cause: The readiness gate started the first action as soon as it saw a snapshot metric, before a readiness span already in flight had finished. The completed Guzzle batch also exposed two body-size attributes absent from its stale coverage. Fix: Settle the full bootstrap window after snapshot metrics and commit the exact Guzzle data.json blob generated by CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Failing check: Scenarios (http-dotnet): timed out sealing action sequence 1 Scenarios (http-js): span overlaps action intervals: point=[1789084648977000000, 1789084648978039679], actions=[..., (1789084648977141487, 9223372036854775808)] Cause: Snapshot metrics had no interval boundary to satisfy action sealing, and millisecond-resolution JavaScript span starts could round just before the driver request boundary while their ends remained in the measured action. Fix: Let observed snapshot metrics satisfy an action directly, and move only spans starting within one millisecond of a boundary into the window containing their end. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trask
changed the title
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Several committed coverage files were not regenerated after raw capture began retaining type-mismatched attributes.
Review details
Files not reviewed (1)
- tools/http/test-client/js/package-lock.json: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tools/runner/src/opentelemetry/conformance/_coverage.py:70
- Reading attributes from raw captures now retains values Weaver rejected for a type mismatch, but the committed reductions have not all been regenerated. For example,
scenarios/http/python/urllib/opentelemetry-urllib/client/data.jsonreports atype_mismatchforexception.escapedwhile omitting that captured attribute fromhttp.client;scenarios/gen-ai/python/agent-framework/native/data.jsonsimilarly omitsgen_ai.response.finish_reasons. The scenario workflow diffs regenerateddata.json, so these packages will become dirty. Please rerun and commit all affected scenario reductions, not only the PHP ones.
- Files reviewed: 163/169 changed files
- Comments generated: 0 new
- Review effort level: Balanced
… section
Review finding:
This example excerpt contradicts the contract it quotes. Every other line of this YAML block is copied verbatim from tools/http/contracts/server.yaml as this PR writes it (the same description, the same driver: runner, the same readiness exchange, the same second scenario), but server.yaml declares the metric as an optional mapping:
metrics:
- name: http.server.request.duration
required: false
whereas this line writes the bare-name form metrics: [http.server.request.duration]. Those two forms mean different things in the schema this PR introduces: _spec.py::_parse_metrics puts a bare name in ScenarioSpec.metrics (required, so check_scenario_telemetry fails a package that does not emit it) and puts a {name, required: false} entry in optional_metrics (permitted but never required). Requiring the duration metric is exactly what this PR deliberately avoids for server packages. A reader who copies this excerpt as the shape of a server contract gets a stricter contract than the one shipped, and nothing in this README mentions required at all. Fix: quote the real server.yaml form, so the excerpt shows the optional-metric syntax the contracts actually use.
Review finding:
This PR makes the claim on this line false and leaves the README contradicting itself. The sentence says readiness is needed by "Every language helper", "so a contract without it fails to load". After this PR no language helper loads a contract at all: the runner parses the contract and injects the actions as JSON through OTEL_CONFORMANCE_SCENARIO_ACTION and OTEL_CONFORMANCE_SCENARIO_ACTIONS, and the helpers decode that table. This same README says so a few paragraphs later, and tools/runner/tests/test_http_packages.py asserts the runner owns the table precisely so a helper cannot answer from its own installed contract copy. Only the runner and otel-http-drive still read a contract file. The consequence clause is wrong too: _spec.py treats a missing readiness key as an empty action table rather than a load failure, and its strict key check does not require readiness. Fix: say that the runner reads the contract and that readiness is what gives the action table its first entry, instead of attributing the requirement to the language helpers.
Analysis: The contract-shape section of tools/http/test-client/README.md describes the contracts this PR introduces, and two of its statements do not match what the PR built. The YAML excerpt quotes tools/http/contracts/server.yaml but writes the metric as a bare name, which _parse_metrics in _spec.py records as required, while server.yaml declares it with required: false so check_scenario_telemetry permits it without demanding it. The prose above the excerpt attributes the readiness requirement to the language helpers and claims a contract without it fails to load; after this PR the helpers decode an injected JSON action table and read no contract file, which the same README states further down, and _spec.py records a missing readiness key as an empty action table rather than failing the load. The excerpt now matches server.yaml in the parts it quotes, a short paragraph explains what the bare-name and mapping forms mean, and the readiness sentence names what actually depends on the exchange: the runner puts it first in the injected action table, a client run polls it against the mock server, and a server run has otel-http-drive wait on it.
Upsides: A reader who copies the excerpt gets the contract the repository ships rather than a stricter one that would require every server package to emit http.server.request.duration. The README no longer contradicts its own statement that only the runner and otel-http-drive read a contract file. The optional-metric form this PR introduces is explained where a reader first meets it, instead of only in tools/runner/README.md.
Downsides: The excerpt grows by two lines and the section by one short paragraph. No code path, test or contract file changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…conflict-fix-loop-worker-again # Conflicts: # .github/workflows/runner.yml # scenarios/http/php/guzzle/opentelemetry-guzzle/composer.lock # scenarios/http/php/slim/opentelemetry-slim/composer.lock # tools/http/test-client/dotnet/Directory.Packages.props # tools/http/test-client/js/package-lock.json # tools/runner/src/opentelemetry/conformance/_checks.py Co-authored-by: trask <218610+trask@users.noreply.github.com>
Co-authored-by: trask <218610+trask@users.noreply.github.com>
…-v4-yet-again # Conflicts: # tools/http/test-client/go/go.mod # tools/http/test-client/go/go.sum Co-authored-by: trask <218610+trask@users.noreply.github.com>
Integrate Git registry resolution and instrumentation-scope policies into the single package Weaver lifecycle. Retain JSON-only PHP helpers and refresh the local path-package lock metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Persistent HTTP scenarios export metrics periodically. Require a completed final flush without failing solely because it raced an in-flight export. Retain the shared timeout, error propagation, and trace/log flush ordering for both SDK paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the stale name-resolution scope finding; regenerated client telemetry no longer emits that scope. Both dotnet package runs reproduce the committed coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reuse the span-kind normalizer at the raw-capture boundary so classifiers distinguish client and internal agent spans. Regenerate GenAI observations, retain newly captured attributes, and document the package-wide Weaver report. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Integrate Weaver 0.28 shutdown and distinct ports into the package lifecycle. Preserve JSON-only HTTP helpers while accepting migrated SQL contracts and retaining the incoming Guzzle upgrade. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <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.
Run one Weaver live check per package while the runner captures raw OTLP in a separate window for each scenario. Scenario assertions use those captures, and Weaver reports package-wide semantic convention violations.
HTTP packages now point to an independent client or server contract:
A contract's
driversets the process lifecycle.instrumentationruns one process per action.runnerkeeps one measured server alive and drives its actions in order through thejsonl-v1protocol. Language helpers receive runner-selected actions as JSON instead of reading YAML.expected_violationsnow belongs at package level, where it reconciles with the aggregate Weaver report. Each pytest package item owns the full lifecycle, and diagnostic output contains per-scenario capture reports plus one aggregate Weaver report.