Repository navigation
ci(instrumentation): enforce contrib manifest name matches TracedPackage - #5831
dd-apm-ecosystems[bot] wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a2342d19c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if _, ok := traced[m.Name]; !ok { | ||
| offending = append(offending, fmt.Sprintf("%s: name %q", relPath, m.Name)) |
There was a problem hiding this comment.
Reject names belonging to another integration
When a contributor copies an existing manifest while adding a new integration and forgets to update its top-level name, this membership check still passes because the copied name is already present in traced. The manifest therefore need not match its own integration's TracedPackage, defeating the stated check in a likely creation workflow; associate each manifest with its corresponding package entry or at least reject duplicate manifest names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 808df61. The check now looks up TracedPackage by contrib path (so a copied name fails against that integration's own package entry) and also rejects duplicate manifest names, including the case where TracedPackage was copied too.
There was a problem hiding this comment.
More details
Static verification confirmed that all 71 existing manifest names satisfy the new check, while packages[].name remains unconstrained. The targeted Go test remains unverified because the module proxy returned HTTP 502 for otelc.
🤖 Bits Code Review · Commit 7a2342d
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 3 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 808df61 | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-11-09 15:10:13 Comparing candidate commit 808df61 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 415 metrics, 1 unstable metrics.
|
A set-membership check still passes when a new manifest copies another integration's name. Require the name to match that contrib path's TracedPackage, and reject duplicate names. Co-authored-by: Cursor <cursoragent@cursor.com>
What does this PR do?
Adds
TestManifestNameMatchesTracedPackagetoinstrumentation/contrib_test.go. It walks everycontrib/**/manifest.yaml, reads the top-levelname, and fails if that value is not theTracedPackageof a package registered ininstrumentation/packages.go.The manifest
nameidentifies the integration (the instrumented library).packages[].namelists the published packages and is intentionally allowed to differ, so only the top-levelnameis checked. The test also fails if no manifests are found, so a wrong walk root can't silently pass.Motivation
Reviewers have repeatedly had to ask contributors to change a new manifest's top-level
nameto the integration'sTracedPackage, "like every other manifest". Both times the contributor agreed and pushed a fix commit. The check is a pure structural lookup, so it shouldn't depend on reviewer attention.Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.Self-verified:
gofmt -l instrumentation/is clean.4a91f43c…) matches the locally tested file.contrib/tree andTracedPackagevalues frompackages.goat1bdbf936. It passes: all 71 current manifests already satisfy the rule, so no existing file needs changing.contrib/envoyproxy/go-control-plane/manifest.yamlnametogithubproxy.fjygbaifeng.eu.org/envoyproxy/go-control-plane/envoyand confirmed the test fails, naming the manifest. I then reverted the change locally; it is not in this diff.go test ./instrumentation/ormake lintend to end. The sandbox's module proxy returned 502 for theotelcdependency, so the root module wouldn't build. CI will run the real thing.Automation Rationale (context for reviewers)
(a) Contributing occurrences (all new this run; human-raised, human-accepted):
manifest-name-traced-package|pr5654|4358986357: PR chore(contrib): backfill AppSec processor manifests #5654 (authordd-evgenii-vasilev). ReviewerrarguelloFflagged thatnameshould be theTracedPackage. The author replied "Good point, updated" (comment 4359881237) and pushed commit0b036cfc.manifest-name-traced-package|pr5656|4358986654: PR chore(contrib/aws/datadog-lambda-go): add instrumentation manifest #5656 (authordd-evgenii-vasilev). ReviewerrarguelloFasked whynamediffered fromTracedPackage, noting "All the other manifests use theTracedPackagevalue". The author replied "Good point, updated" (comment 4359882056) and pushed commit21041828.Table C had no
do_not_implementorimplement_differentlyrows, so no prior maintainer feedback applies.(b) Why this is low-false-positive and mechanical: The check is a set-membership test on one YAML scalar against a fixed set of Go string constants. It needs no semantic judgment. All 71 existing manifests pass today, so adoption causes zero failures. The legitimate difference between
nameandpackages[].nameis left unchecked.(c) Tool and CI wiring: I added a native Go test next to the existing convention tests (
TestNoSetTagServiceName,TestNoTracerServiceName) ininstrumentation/contrib_test.go. It runs withgo test ./instrumentation/....instrumentation/**is claimed by thecorecomponent in.github/ci-components.yml(gate@everything), so it runs in the merge-gating test jobs. No new workflow, script, or golangci-lint config was needed, because manifests are YAML andpackages.gois an in-package map that a generic linter can't cross-reference. The test usesgo.yaml.in/yaml/v3, which is already a direct dependency in the rootgo.mod.(d) Verification command:
go test ./instrumentation/ -run TestManifestNameMatchesTracedPackage -v(e) Not shaped by an
implement_differentlyrow; none exist.Related automation PRs for other themes: #5612 (naming map), #5774 (typed
ext.Component) and #5708 (configinverter) are still open and independent of this change.