Skip to content

fix(tool): support path helpers for call rules#482

Open
ecleon26 wants to merge 2 commits into
open-telemetry:mainfrom
ecleon26:fix-call-rule-path-helpers
Open

fix(tool): support path helpers for call rules#482
ecleon26 wants to merge 2 commits into
open-telemetry:mainfrom
ecleon26:fix-call-rule-path-helpers

Conversation

@ecleon26
Copy link
Copy Markdown

@ecleon26 ecleon26 commented May 10, 2026

Description

Adds path support to call rules so replacement wrappers can reference helper functions that live outside the target source file.

This change:

  • adds Path parsing to InstCallRule
  • detects unqualified helper calls used by call-rule replace / append_args
  • materializes matching helper files from the configured path into the target package during instrumentation
  • updates importcfg for helper-file imports
  • updates the affected golden fixtures, including wrap_sizeof_call and wrap_sizeof_with_strings
  • documents the new call-rule path behavior

Motivation

Call-rule YAML fixtures can specify wrapper expressions such as Wrapper({{ . }}), but InstCallRule previously had no Path field. As a result, adding path: to YAML would be silently ignored, and call-rule replacements could only reference helpers already defined in the target source file or symbols available through imports.

This fixes the missing code path so helper functions can be located and compiled into the target package when a call rule needs them.

Fixes #

Checklist

  • PR title follows conventional commits format
  • Code formatted: make format
  • Linters pass: make lint
  • Tests pass: make test
  • Tests added for new functionality
  • Tests follow testing guidelines
  • Documentation updated (if applicable)

@ecleon26
fix: support path helpers for call rules
d7c1961 
Add path parsing and helper file materialization for call rules so wrapper replacements can reference helper functions outside the target source file.

Assisted-by: ChatGPT 5
@ecleon26 ecleon26 requested a review from a team as a code owner May 10, 2026 08:13
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 10, 2026 •
edited
Loading

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: ecleon26 / name: narrative26 (d7c1961)

@github-actions github-actions Bot added the scope:fix A bug that is being fixed label May 10, 2026
@ecleon26
Copy link
Copy Markdown
Author

ecleon26 commented May 10, 2026

@y1yang0

@amazingakai
Copy link
Copy Markdown
Contributor

amazingakai commented May 12, 2026

Thank you for the contribution.

We already support helper usage through the existing imports field. For example, if we want to use our own wrapper/helper code around a function, we can define it inside github.com/open-telemetry/opentelemetry-go-compile-instrumentation/pkg/instrumentation/..., add it through imports, and then reference it directly inside replace.
From my understanding, the main missing piece right now is support for this in golden tests, since we would need compileArgs/createImportCfg to compile and resolve those helper imports correctly there as well (please see my comment on the original issue).

@amazingakai amazingakai mentioned this pull request May 12, 2026
Open
@ecleon26
Copy link
Copy Markdown
Author

ecleon26 commented May 12, 2026

That makes sense to me. I agree that broadening path to call/raw/decl rules may blur the existing rule semantics, since those rules already have an imports mechanism for external helper code.

For call rules specifically, it seems cleaner to reference helpers through an imported package alias in replace, for example wrapper.Wrapper({{ . }}), and declare that package through imports. That keeps helper resolution explicit and consistent with raw/decl-style expression injection.

So I think the better fix may be limited to golden-test infrastructure: either teach compileArgs / createImportCfg how to resolve testcase-local helper packages, or avoid testcase-local wrapper imports if that adds too much test-only complexity. I do not think we should add path support across more rule types unless we want a broader rule-model change.

@txabman42 txabman42 mentioned this pull request May 12, 2026
Open
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 74.80916% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.60%. Comparing base (0216630) to head (e99d2e5).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
tool/internal/instrument/apply_call.go 77.50% 16 Missing and 11 partials ⚠️
tool/internal/instrument/apply_func.go 45.45% 3 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##           main     #482       +/-   ##
=========================================
+ Coverage      0   63.60%   +63.60%     
=========================================
  Files         0       62       +62     
  Lines         0     4927     +4927     
=========================================
+ Hits          0     3134     +3134     
- Misses        0     1528     +1528     
- Partials      0      265      +265
Flag Coverage Δ
tool 63.60% <74.80%> (?)

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.

gyanranjanpanda added a commit to gyanranjanpanda/opentelemetry-go-compile-instrumentation that referenced this pull request May 21, 2026
@gyanranjanpanda
fix(test): teach golden test infra to resolve non-stdlib rule imports
b094c09 
Extends createImportCfg to dynamically resolve import paths declared in
rule imports fields, not just standard library packages. This fixes the
gap identified in issue open-telemetry#475 where call rules that reference helper
packages through the imports field could not be validated in golden tests.

Changes:
- Extract collectRuleImports to gather import paths from all rule types
- Extend createImportCfg to resolve additional packages via go list
- Factor out resolvePackageExport for reuse across stdlib and rule imports
- Add call-rule-local-import golden test demonstrating the imports pattern
- Add testhelper/wrapper package as a reusable helper for golden tests

This addresses the maintainer feedback on PR open-telemetry#482 and issue open-telemetry#475:
call rules should use the existing imports mechanism rather than
adding a path field. The test infrastructure now supports this pattern.

Closes open-telemetry#475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

scope:fix A bug that is being fixed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ecleon26 @amazingakai @kakkoyun