Skip to content

feat(tool): support passing .go files to otelc go build#483

Open
amazingakai wants to merge 7 commits into
open-telemetry:mainfrom
amazingakai:support-go-files
Open

feat(tool): support passing .go files to otelc go build#483
amazingakai wants to merge 7 commits into
open-telemetry:mainfrom
amazingakai:support-go-files

Conversation

@amazingakai
Copy link
Copy Markdown
Contributor

Description

Support passing .go source files to otelc go build command

Motivation

Currently passing .go files to otelc go build fails, while this is supported by go build command.


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)

@amazingakai amazingakai requested a review from a team as a code owner May 10, 2026 13:27
@github-actions github-actions Bot added the scope:feat A new feature being added label May 10, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2026

Codecov Report

❌ Patch coverage is 61.25000% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.70%. Comparing base (2c74fda) to head (88fe92e).

Files with missing lines Patch % Lines
tool/internal/setup/setup.go 55.38% 23 Missing and 6 partials ⚠️
tool/internal/pkgload/pkgload.go 86.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
- Coverage   63.70%   63.70%   -0.01%     
==========================================
  Files          62       62              
  Lines        4794     4854      +60     
==========================================
+ Hits         3054     3092      +38     
- Misses       1486     1503      +17     
- Partials      254      259       +5     
Flag Coverage Δ
tool 63.70% <61.25%> (-0.01%) ⬇️

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.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for passing .go source files directly to otelc go build, aligning otelc behavior more closely with go build when users build from an explicit file list (which creates the synthetic command-line-arguments package).

Changes:

  • Parse build targets into package patterns vs .go file targets and load packages accordingly.
  • Support file-target builds by accepting command-line-arguments packages and injecting otelc.runtime.go onto the go build command line.
  • Extend unit tests to cover file targets and the new target-splitting behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
tool/internal/setup/setup.go Adds target splitting, file-target package loading/filtering, and runtime file injection for file-based builds.
tool/internal/setup/setup_test.go Adds/updates tests for file targets, mixing targets, and default package loading.

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

Comment thread tool/internal/setup/setup.go Outdated
Comment thread tool/internal/setup/setup.go Outdated
Comment thread tool/internal/setup/setup.go Outdated
Comment on lines +412 to +417
restArgs := args[1:]
if _, fileTargets, err2 := splitBuildTargets(restArgs); err2 == nil && len(fileTargets) > 0 {
// add otelc.runtime.go manually to command line for file targets
dir := filepath.Dir(fileTargets[0])
otelcRuntimePath := filepath.Join(dir, OtelcRuntimeFile)
restArgs = append(restArgs, otelcRuntimePath)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really true, If user does:
go build -C cmd main.go
It would do:
go build -C cmd main.go otelc.runtime.go
Similarly for:
go build -C cmd /home/.../absolute_path_to/main.go
It would do:
go build -C cmd /home/.../absolute_path_to/main.go cmd /home/.../absolute_path_to/otelc.runtime.go
In both cases, it should be correct, unless I'm missing something.

Copy link
Copy Markdown
Contributor

@txabman42 txabman42 May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-C wasn't supported before. LoadPackages never sets Dir for packages.Config. I've created an issue for tackling this #507

Comment thread tool/internal/setup/setup.go
Comment thread tool/internal/setup/setup.go
Comment thread tool/internal/setup/setup.go Outdated
Comment thread tool/internal/setup/setup.go Outdated
Comment thread tool/internal/setup/setup.go Outdated
Comment on lines +412 to +417
restArgs := args[1:]
if _, fileTargets, err2 := splitBuildTargets(restArgs); err2 == nil && len(fileTargets) > 0 {
// add otelc.runtime.go manually to command line for file targets
dir := filepath.Dir(fileTargets[0])
otelcRuntimePath := filepath.Join(dir, OtelcRuntimeFile)
restArgs = append(restArgs, otelcRuntimePath)
Copy link
Copy Markdown
Contributor

@txabman42 txabman42 May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-C wasn't supported before. LoadPackages never sets Dir for packages.Config. I've created an issue for tackling this #507

@amazingakai
Copy link
Copy Markdown
Contributor Author

Thank you for the review, I will try to address the comments soon

@amazingakai amazingakai force-pushed the support-go-files branch 2 times, most recently from 53e16f3 to 577b856 Compare May 18, 2026 11:05
@amazingakai
Copy link
Copy Markdown
Contributor Author

This PR is ready for review again @y1yang0 @txabman42

I also don't think there is a performance regression here (or at least I don't see a reason for there to be). Not sure why the Windows integration tests are timing out.

Benchmark results on my local machine look effectively unchanged:

Before:

overhead_test.go:67: baseline: plain=4.116s  otelc=5.151s  overhead=+25.2%
overhead_test.go:67: largeidle: plain=13.270s  otelc=14.614s  overhead=+10.1%
overhead_test.go:67: multi: plain=16.276s  otelc=24.962s  overhead=+53.4%

After:

overhead_test.go:67: baseline: plain=4.095s  otelc=5.182s  overhead=+26.5%
overhead_test.go:67: largeidle: plain=13.200s  otelc=14.697s  overhead=+11.3%
overhead_test.go:67: multi: plain=16.301s  otelc=24.945s  overhead=+53.0%

@txabman42
Copy link
Copy Markdown
Contributor

@amazingakai

Not sure why the Windows integration tests are timing out.

Rerun just fixed it. Seems that runner was slow, probably bad neighbours.

Copy link
Copy Markdown
Contributor

@txabman42 txabman42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just some nitpicks

Comment thread tool/internal/setup/setup.go Outdated
Comment thread tool/internal/setup/setup.go
@amazingakai
Copy link
Copy Markdown
Contributor Author

Done. Thank you for the review ❤️

@txabman42 txabman42 requested a review from a team May 19, 2026 08:48
Copy link
Copy Markdown
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kakkoyun kakkoyun requested a review from y1yang0 May 19, 2026 09:02
@kakkoyun
Copy link
Copy Markdown
Member

@amazingakai This needs a rebase.

@amazingakai
Copy link
Copy Markdown
Contributor Author

@kakkoyun Done.

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

Labels

scope:feat A new feature being added

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants