Skip to content

fix: add cross-platform perf reader implementation and fix type mismatch #1969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Nikki371
Copy link

Issue

Errors encountered during local make build on non Linux System.

Although the expectation is to use docker images or virtual OS, thought this might help clear the obvious build issues.

# go.opentelemetry.io/auto/internal/pkg/process
internal/pkg/process/allocate.go:42:13: invalid operation: pagesize * nCPU (mismatched types uint64 and int)
internal/pkg/process/allocate.go:63:14: cannot use nCPU (variable of type int) as uint64 value in struct literal
make: *** [build] Error 1
CGO_ENABLED=0 go build -o otel-go-instrumentation ./cli/...
# go.opentelemetry.io/auto/internal/pkg/instrumentation/bpffs
internal/pkg/instrumentation/bpffs/bpfsfs_other.go:12:47: undefined: process.TargetDetails
internal/pkg/instrumentation/bpffs/bpfsfs_other.go:16:28: undefined: process.TargetDetails
internal/pkg/instrumentation/bpffs/bpfsfs_other.go:20:30: undefined: process.TargetDetails
make: *** [build] Error 1
CGO_ENABLED=0 go build -o otel-go-instrumentation ./cli/...
# go.opentelemetry.io/auto/internal/pkg/instrumentation/probe
internal/pkg/instrumentation/probe/probe.go:76:26: undefined: perf.Record
internal/pkg/instrumentation/probe/probe.go:78:24: undefined: perf.Reader
internal/pkg/instrumentation/probe/probe.go:236:23: undefined: perf.NewReader
internal/pkg/instrumentation/probe/probe.go:267:27: undefined: perf.ErrClosed
internal/pkg/instrumentation/probe/probe.go:321:27: undefined: perf.ErrClosed
internal/pkg/instrumentation/probe/probe.go:353:27: undefined: perf.ErrClosed
make: *** [build] Error 1

Environment

  • OS: Apple M2 (macOS : 15.3.1 (24D70))
  • Go Version: 1.24
  • Version: v.0.21.0

To Reproduce

Steps to reproduce the behavior:

  1. Clone opentelemetry-go-instrumentation to local
  2. run make build

Expected behavior

Expected make build to be successful on non linux systems too as the mock implementation is already present

Changes

  • Created platform-specific implementations for perf reader with Linux implementation using github.com/cilium/ebpf/perf and stub implementations for non-Linux platforms
  • Fixed type mismatches by updating process.TargetDetails to process.Info in bpffs package
  • Changed GetCPUCount return type from int to uint64 for consistency
  • Improved documentation for kernel lockdown mode constants
  • Updated import statements to use the new internal perf package

…tches

- Created platform-specific implementations for perf reader with Linux implementation
  using github.com/cilium/ebpf/perf and stub implementations for non-Linux platforms
- Fixed type mismatches by updating process.TargetDetails to process.Info in bpffs package
- Changed GetCPUCount return type from int to uint64 for consistency
- Improved documentation for kernel lockdown mode constants
- Updated import statements to use the new internal perf package
Copy link

linux-foundation-easycla bot commented Mar 11, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@Nikki371 Nikki371 marked this pull request as ready for review March 11, 2025 12:17
@Nikki371 Nikki371 requested a review from a team as a code owner March 11, 2025 12:17
Copy link
Contributor

@RonFed RonFed left a comment

Choose a reason for hiding this comment

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

I'm not sure about the perf change.
It broke because of cilium/ebpf#1650
From the cilium slack it looks like it broke other stuff as well.
I'm not sure if we need to create these stubs here or it should be handled upstream in the cilium library.

@RonFed
Copy link
Contributor

RonFed commented Apr 9, 2025

Opened cilium/ebpf#1746
cc @MrAlias

@RonFed
Copy link
Contributor

RonFed commented Apr 18, 2025

Seeing the issue linked above is resolved, I think we can remove the perf stubs from this PR.

@MrAlias
Copy link
Contributor

MrAlias commented Apr 18, 2025

Seeing the issue linked above is resolved, I think we can remove the perf stubs from this PR.

Should we update to a commit hash version of that package to support this?

@RonFed
Copy link
Contributor

RonFed commented Apr 19, 2025

Should we update to a commit hash version of that package to support this?

we could wait for an official release, not sure how much time that would take.
I don't mind either way we choose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants