feat(rule): add wrap_struct_literal join-point for struct literal instrumentation#459
Conversation
…trumentation Implements issue open-telemetry#202 by adding a new wrap_struct_literal rule type that enables template-based wrapping of struct instantiations at compile time. Changes: - Define InstStructLiteralRule with struct_literal, match, and template fields - Integrate struct literal rules into the rule engine (parsing, matching) - Implement AST transformation via applyStructLiteralRule using dst walker - Support value-only, pointer-only, and any match modes - Handle aliased imports for accurate struct type resolution - Add glob-based target matching with exclude patterns for all rule types - Add comprehensive unit tests for rule creation and AST transformation Assisted-by: Claude Opus 4.6
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3d5e96a28
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Filter rules by target using pattern matching | ||
| relevantRules := make([]rule.InstRule, 0) | ||
| for _, r := range allRules { | ||
| matched, err := path.Match(r.GetTarget(), dep.ImportPath) |
There was a problem hiding this comment.
runMatch now uses path.Match to evaluate both target and excludes, but path.Match treats / as a separator and * does not cross it. That means common glob intents like target: "*", target: "github.com/myorg/*", or similar excludes will not match multi-segment import paths (for example, github.com/myorg/lib/subpkg), so rules are silently skipped in real projects. Since this commit introduces glob-based targeting, this behavior makes the feature effectively unusable for most package paths unless every / level is explicitly encoded.
Useful? React with 👍 / 👎.
amazingakai
left a comment
amazingakai
left a comment
There was a problem hiding this comment.
Thank you for looking into this. For context, my comment was meant to start a discussion rather than serve as a ready proposal. I think it might be too soon to implement this without some maintainer input on the design first.
| relevantRules := make([]rule.InstRule, 0) | ||
| for _, r := range allRules { | ||
| matched, err := path.Match(r.GetTarget(), dep.ImportPath) | ||
| if err != nil { | ||
| sp.Error("Invalid target pattern", "target", r.GetTarget(), "err", err) | ||
| continue | ||
| } | ||
| if !matched { | ||
| continue | ||
| } | ||
|
|
||
| // Check excludes | ||
| excluded := false | ||
| for _, excl := range r.GetExcludes() { | ||
| exclMatch, exclErr := path.Match(excl, dep.ImportPath) | ||
| if exclErr != nil { | ||
| sp.Warn("Invalid exclude pattern", "pattern", excl, "error", exclErr) | ||
| continue | ||
| } | ||
| if exclMatch { | ||
| excluded = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !excluded { | ||
| relevantRules = append(relevantRules, r) | ||
| } | ||
| } |
| InstBaseRule `yaml:",inline"` | ||
|
|
||
| StructLiteral string `json:"struct_literal" yaml:"struct_literal"` // The type name of the struct literal to be matched | ||
| Match string `json:"match" yaml:"match"` // "value-only", "pointer-only", or "any" |
There was a problem hiding this comment.
Orchestrion also supported field level advising, mutually exclusive with match. Might be worth adding but it should be discussed first.
See: https://datadoghq.dev/orchestrion/contributing/aspects/join-points/struct-literal/
|
This PR is also missing golden tests and documentation. |
…en tests, and revert premature glob match logic
|
could @amazingakai check this if there any other isuue pls let me know |
|
Too early to review now, we should first discuss the necessity and design. |
3 participants
Summary
Implements #202 — adds a new
wrap_struct_literaljoin-point that enables template-based wrapping of struct instantiations at compile time.Motivation
The current instrumentation framework supports wrapping function calls (
wrap_call) and injecting hooks into function bodies (func), but has no mechanism to intercept struct literal creation. This is needed for instrumenting patterns like&http.Server{}or&http.Transport{}where the instrumentation point is the instantiation itself, not a function call.Design
Rule Schema
Match Modes
&http.Server{}http.Server{}pointer-onlyvalue-onlyany(default)Architecture
The implementation follows the established rule engine pattern:
InstStructLiteralRule) — mirrorsInstCallRulewith struct-specific fieldsdst.CompositeLitnodes, resolves import aliases, and applies the template wrapChanges
New files
tool/internal/rule/struct_literal_rule.gotool/internal/rule/struct_literal_rule_test.gotool/internal/instrument/apply_struct_literal.gotool/internal/instrument/apply_struct_literal_test.goModified files
tool/internal/rule/base.goStructLiteralRulestoInstRuleSet,ExcludestoInstBaseRuletool/internal/instrument/instrument.goInstStructLiteralRuletoapplyStructLiteralRuletool/internal/setup/match.gowrap_struct_literalrule type, glob-based target matchingtool/internal/setup/match_test.gorunMatchsignatureTesting
make allcompletes successfully (build → format → lint → test)Checklist
make allpassesInstCallRule,applyCallRule)