fix(rule): add missing path field to call rule test data by gyanranjanpanda · Pull Request #492 · open-telemetry/opentelemetry-go-compile-instrumentation

gyanranjanpanda · 2026-05-12T02:16:51Z

Summary

Fixes #475.

The wrap_sizeof_call and wrap_sizeof_with_strings test rules were missing the path field needed to specify where wrapper functions are located. This change adds the path field to InstCallRule and updates the affected golden test YAML files.

Changes

1. `tool/internal/rule/call_rule.go`

Added Path field to InstCallRule, mirroring the existing field on InstFuncRule and InstFileRule:

// Path is the module path where wrapper functions referenced in the
// replace template are located (e.g. "github.com/foo/bar/hook").
Path string `json:"path" yaml:"path"`

2. Test YAML fixes

call-rule-only/rules.yml — added path: testdata/golden/call-rule-only
call-rule-with-import/rules.yml — added path: testdata/golden/call-rule-with-import

3. Unit test

Added "replace with path" test case in call_rule_test.go validating the Path field round-trips correctly through YAML parsing.

Thorough Review

Per the issue's request, I audited every rules.yml in testdata/golden/:

Func rules (15 rules) — all already have path ✅
File rules (2 rules) — all already have path ✅
Call rules with template — wrap_sizeof_call and wrap_sizeof_with_strings were missing path (now fixed)
Call rules with append_args — don't need path (no external functions)
Other rule types (raw, struct, decl, directive) — don't use path ✅
call-rule-template-features uses a self-contained IIFE, no path needed ✅

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8184425f28

ℹ️ 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".

chatgpt-codex-connector · 2026-05-12T02:19:31Z

+	// Path is the module path where wrapper functions referenced in the
+	// replace template are located (e.g. "github.com/foo/bar/hook").
+	Path string `json:"path" yaml:"path"`


Wire call rule path into instrumentation

This only records path; call-rule instrumentation never reads it (applyCallRule applies r.Imports but has no handling for r.Path). When a call rule follows this new field/comment and keeps Wrapper in the package named by path instead of defining it in the target package, the generated code is still just Wrapper(...) with no helper import or copied/linknamed code, so the instrumented package fails to compile. Either wire Path into call-rule dependency/import handling or reject/omit it so it is not a parsed-but-ignored setting.

Useful? React with 👍 / 👎.

gyanranjanpanda · 2026-05-12T04:53:34Z

@y1yang0 could u review this waiting for your feed back

Add the missing path field to InstCallRule and wire it into the instrument phase so wrapper functions from the path are compiled into the target package. Changes: - Add Path field to InstCallRule struct (mirrors InstFuncRule/InstFileRule) - Wire Path into instrument phase: loadCallRuleAuxFiles scans the path for Go files marked with //go:build ignore, strips the tag, renames the package, updates importcfg, and adds them to the compilation - Use collision-safe naming: otelc.<ruleName>.<basename>.go - Move Wrapper from call-rule-only/source.go to hook.go with fmt import to verify that importcfg updating works end-to-end - Remove path from call-rule-with-import (inline IIFE, no helper needed) - Add path to call-rule-only/rules.yml (uses Wrapper from hook file) - Document path in docs/rules.md field table and important notes - Add unit test verifying Path round-trips through YAML parsing Fixes: open-telemetry#475

amazingakai

Duplicate of #482. Please also see my comment there and on the original issue.

codecov · 2026-05-12T07:09:36Z

Codecov Report

❌ Patch coverage is 65.21739% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.85%. Comparing base (08d86e9) to head (2d73b5f).

Files with missing lines	Patch %	Lines
tool/internal/instrument/apply_call.go	68.18%	7 Missing and 7 partials ⚠️
tool/internal/instrument/instrument.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   62.83%   62.85%   +0.02%     
==========================================
  Files          62       62              
  Lines        4765     4811      +46     
==========================================
+ Hits         2994     3024      +30     
- Misses       1527     1535       +8     
- Partials      244      252       +8

Flag	Coverage Δ
tool	`62.85% <65.21%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR extends call-wrapping instrumentation rules to support a path field so call-rule replace templates can reference wrapper/helper functions supplied from auxiliary Go files, and updates the relevant golden test data and documentation.

Changes:

Added Path to InstCallRule and a YAML round-trip unit test for it.
Instrumentation phase now loads //go:build ignore helper files from call-rule path into the target compilation (similar to file rules).
Updated golden test fixtures for the call-rule-only scenario to move Wrapper into an auxiliary hook.go and reflect the generated output.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tool/internal/rule/call_rule.go	Adds `Path` to call rules (YAML/JSON) for locating auxiliary wrapper code.
tool/internal/rule/call_rule_test.go	Adds a test case asserting `path` parses/round-trips correctly.
tool/internal/instrument/instrument.go	Loads call-rule auxiliary files before applying call-site transformations.
tool/internal/instrument/apply_call.go	Implements scanning/loading of `//go:build ignore` helper files from `path`.
tool/internal/instrument/testdata/golden/call-rule-only/source.go	Removes inline `Wrapper` from the source fixture.
tool/internal/instrument/testdata/golden/call-rule-only/rules.yml	Adds `path` for the call rule to locate helper code.
tool/internal/instrument/testdata/golden/call-rule-only/hook.go	New ignored helper file providing `Wrapper`.
tool/internal/instrument/testdata/golden/call-rule-only/call_rule_only.otelc.3374256065.hook.go.golden	New golden output for the generated/compiled helper file.
tool/internal/instrument/testdata/golden/call-rule-only/call_rule_only.main.go.golden	Updates expected transformed output (no inline wrapper).
docs/rules.md	Documents the new `path` field for call rules and its semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+	// Path is the module path where wrapper functions referenced in the
+	// replace template are located (e.g. "github.com/foo/bar/hook").


 | `replace` | string | No (one of `replace`/`append_args` required) | Wrapper template with `{{ . }}` placeholder for the original call. Must produce a valid Go expression. |
 | `append_args` | `[]string` | No (one of `replace`/`append_args` required) | Go expression strings appended as additional arguments to the matched call |
 | `variadic_type` | string | No | Element type for the ellipsis IIFE wrapper (e.g. `grpc.DialOption`). Required when any matched call uses `...` spread. |
+| `path` | string | No | Module path containing helper Go files (marked with `//go:build ignore`) that provide wrapper functions referenced in `replace`. These files are compiled into the target package at instrumentation time. Not needed when the wrapper is an inline IIFE or already available in the target. |


+	// Include a stable suffix to prevent collisions when multiple call rules
+	// at different paths both contain a file with the same basename.
+	suffix := util.CRC32(ruleName + ":" + file)
+	newFile := filepath.Join(ip.workDir, fmt.Sprintf("otelc.%s.%s.go", suffix, stem))
+	if err = ast.WriteFile(newFile, root); err != nil {


+	// Only load files marked with //go:build ignore to avoid pulling in
+	// test source files or other non-hook code at the same path.
+	if !strings.Contains(content, "//go:build ignore") {
+		return nil
+	}
+	root, err := ast.NewAstParser().ParseSource(stripBuildIgnoreTag(content))


+	// Load auxiliary files from call rules that specify a path. These files
+	// contain wrapper functions referenced in replace templates. Loading them
+	// here (alongside file rules) ensures the functions are compiled into the
+	// target package before the call-site replacements are applied.
+	if err := ip.loadCallRuleAuxFiles(ctx, rset); err != nil {
+		return err
+	}


gyanranjanpanda requested a review from a team as a code owner May 12, 2026 02:16

github-actions Bot added the scope:fix A bug that is being fixed label May 12, 2026

chatgpt-codex-connector Bot reviewed May 12, 2026

View reviewed changes

gyanranjanpanda force-pushed the fix/add-missing-path-field-475 branch 2 times, most recently from 2a487d4 to 5414852 Compare May 12, 2026 04:45

gyanranjanpanda force-pushed the fix/add-missing-path-field-475 branch from 8e5bfcc to 43a4a09 Compare May 12, 2026 06:02

Merge branch 'main' into fix/add-missing-path-field-475

2d73b5f

y1yang0 self-requested a review May 12, 2026 06:11

amazingakai reviewed May 12, 2026

View reviewed changes

NameHaibinZhang requested a review from Copilot May 12, 2026 11:52

Copilot started reviewing on behalf of NameHaibinZhang May 12, 2026 11:53 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

txabman42 mentioned this pull request May 12, 2026

The wrap_sizeof_call test rule is missing the path field #475

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rule): add missing path field to call rule test data#492

fix(rule): add missing path field to call rule test data#492
gyanranjanpanda wants to merge 2 commits into
open-telemetry:mainfrom
gyanranjanpanda:fix/add-missing-path-field-475

gyanranjanpanda commented May 12, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 12, 2026

Uh oh!

gyanranjanpanda commented May 12, 2026

Uh oh!

amazingakai left a comment

Uh oh!

codecov Bot commented May 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// Path is the module path where wrapper functions referenced in the
		// replace template are located (e.g. "github.com/foo/bar/hook").

Conversation

gyanranjanpanda commented May 12, 2026

Summary

Changes

1. tool/internal/rule/call_rule.go

2. Test YAML fixes

3. Unit test

Thorough Review

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 12, 2026

Choose a reason for hiding this comment

Uh oh!

gyanranjanpanda commented May 12, 2026

Uh oh!

amazingakai left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 12, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. `tool/internal/rule/call_rule.go`