Skip to content

test(bdd): propagate trace context in watch probes - #1333

Closed
mikeyrcamp wants to merge 5 commits into
mainfrom
mcamp/fix/pr1310-traceparent
Closed

test(bdd): propagate trace context in watch probes#1333
mikeyrcamp wants to merge 5 commits into
mainfrom
mcamp/fix/pr1310-traceparent

Conversation

@mikeyrcamp

@mikeyrcamp mikeyrcamp commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

TL;DR

Propagate valid W3C Trace Context metadata on both secure multi-region WatchStargates probes and keep the exact-command wiring coverage synchronized.

Additional Details

CodeRabbit identified that the Region B grpcurl probe in #1310 omitted traceparent metadata. The adjacent Region A probe is also an outbound gRPC call, so this companion applies the repository trace-propagation convention consistently to both calls.

Each probe uses a distinct, valid W3C traceparent value. The feature behavior, TLS authority, certificate trust, and response assertions are unchanged.

This Pull Request is stacked on #1310 and targets its feature branch.

Before

flowchart LR
  A["Region A outbound grpcurl Watch"] --> C["No W3C trace context"]
  B["Region B outbound grpcurl Watch"] --> C
  C --> D["Cross-region trace correlation breaks"]
Loading

After

flowchart LR
  A["Region A grpcurl Watch"] -->|Valid traceparent A| C["Correlated Watch flows"]
  B["Region B grpcurl Watch"] -->|Distinct valid traceparent B| C
  D["Exact-command wiring tests"] -->|Lock command| A
  D -->|Lock command| B
Loading

For the Reviewer

Please verify the two grpcurl -H additions and their byte-for-byte wiring expectations in tests/bdd/godog_test.go.

Original review thread: #1310 (comment)

For QA

Validated in an isolated development environment:

  • focused secure multi-region wiring test
  • go test -short -count=1 ./...
  • go vet ./...
  • golangci-lint v2.12.2 with revive only: 0 issues
  • shell syntax checks for the changed feature commands and Region B installation script
  • git diff --check

The configured full lint command still reports two pre-existing staticcheck capitalization findings in tests/bdd/dsl/manifests.go; this companion does not modify that file. A live cluster run was not performed for this review-only change.

Issues

Relates to #1307

Dependencies

No new or updated third-party dependencies. No license or NOTICE changes are required.

Checklist

  • I am familiar with the Contributing Guidelines.
  • I have signed off my commits for Developer Certificate of Origin (DCO) compliance.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@mikeyrcamp
mikeyrcamp requested a review from a team as a code owner August 28, 2026 22:08
@mikeyrcamp
mikeyrcamp requested a review from estroz August 28, 2026 22:08
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 825b2f8e-c435-499b-9406-8d1001ac9cf8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Independent review of current head 737c5f82 found no Critical, Important, or Minor issues.

Both outbound Watch probes now carry distinct, valid W3C version-00 traceparent values. The grpcurl -H quoting is correct for the feature commands, and byte-for-byte exact-command coverage locks both probes to the intended metadata and arguments.

The focused wiring test, full short BDD suite, Go vet, shell checks, DCO, targeted sensitive-string checks, and diff hygiene are green. The before/after Mermaid diagrams are also verified.

CodeRabbit skipped review because this PR targets a stacked, non-default base. #1333 is ready for stacked integration into #1310; #1310 still requires its current-main rebase and normal review. No merge was performed.

@sbaum1994
sbaum1994 force-pushed the test/bdd-1292-secure-multiregion branch 4 times, most recently from d1881dd to 34b2e39 Compare August 30, 2026 08:13
@mikeyrcamp
mikeyrcamp force-pushed the mcamp/fix/pr1310-traceparent branch from 737c5f8 to 9f6484d Compare August 30, 2026 12:05
@sbaum1994
sbaum1994 force-pushed the test/bdd-1292-secure-multiregion branch from 34b2e39 to 104766c Compare August 31, 2026 07:08
sbaum1994 and others added 5 commits August 31, 2026 09:14
Signed-off-by: Mike Camp <mcamp@nvidia.com>
Replace embedded Pylon polling with the shared topology assertion while leaving regional router identity checks explicit in the feature.

Refs #1307
Refs #1358

Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
Refs #1292

Signed-off-by: Stephanie Baum <sbaum@nvidia.com>
Relates to #1307

Signed-off-by: Mike Camp <mcamp@nvidia.com>
@mikeyrcamp
mikeyrcamp force-pushed the mcamp/fix/pr1310-traceparent branch from 9f6484d to ca30c18 Compare August 31, 2026 13:15
Base automatically changed from test/bdd-1292-secure-multiregion to main August 31, 2026 14:13
@mikeyrcamp

Copy link
Copy Markdown
Contributor Author

Closing as duplicated/superseded by PR #1310, which has merged the same multi-region trace-context coverage into main.

@mikeyrcamp mikeyrcamp closed this Aug 31, 2026
@sbaum1994

Copy link
Copy Markdown
Collaborator

Superseded by #1310, which merged into main as ac46f30.

That merge already carries this change, in a more general form than the two hardcoded headers here:

  • tests/bdd/scripts/observe-watch-stargates.sh generates a fresh trace and span ID per invocation and passes -H "traceparent: $traceparent" on the outbound WatchStargates call, so both region A and region B probes are covered.
  • tests/bdd/dsl/registration_test.go adds TestObserveWatchStargatesScriptSendsW3CTraceContext, asserting the emitted header is a valid W3C trace context value.
  • tests/bdd/PLAN.md documents the shared step as propagating W3C trace context.

This branch cannot be rebased onto main. Its only commit (9f6484d) patches two inline /bin/bash -c grpcurl blobs in multi-cluster-helmfile-llm-registration-multiregion.feature. main replaced both with the shared DSL step:

When I successfully observe WatchStargates at "127.0.0.1:50071" with TLS authority "llm-request-router.nvcf.svc.cluster.local" using CA secret "stargate-quic-tls" in namespace "nvcf" and context "k3d-ncp-local-cp" for "3" seconds

Cherry-picking onto current main conflicts in the feature file and godog_test.go, and resolving the conflict removes the change, since the lines it targets no longer exist.

Closing as already delivered. No action needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants