Skip to content

rlimit: allow compiling rlimit on non-linux systems#624

Closed
korniltsev wants to merge 1 commit intoopen-telemetry:mainfrom
grafana:korniltsev/rlimit_nonlinux
Closed

rlimit: allow compiling rlimit on non-linux systems#624
korniltsev wants to merge 1 commit intoopen-telemetry:mainfrom
grafana:korniltsev/rlimit_nonlinux

Conversation

@korniltsev
Copy link
Copy Markdown
Contributor

Similar to #612

This will eventually help allowing running some unit tests (for example coredump tests) without spawning a full linux OS on non linux systems (at least darwin)

@korniltsev korniltsev requested review from a team as code owners July 16, 2025 10:32
@florianl
Copy link
Copy Markdown
Member

Would you mind handling all cases and tests, where rlimit is used as dependency?
Thinking about tracer/tracepoints.go, tracer/tracer.go and others.

@korniltsev
Copy link
Copy Markdown
Contributor Author

Would you mind handling all cases and tests

I would prefer to attempt doing this separately.

  1. My goal is to allow running some useful subset of unit tests faster and more convenient. and the tracer package is more of a "integration" beast so it falls out my goal slightly.
  2. It would require to "mock" the github.com/elastic/go-perf within the current repo or maybe create stubs within the go-perf itself, or maybe some other way. I will need to think about it.
./tracer.go:86:40: undefined: perf.Event
./tracer.go:206:27: undefined: perf.Event
./tracer.go:1119:28: undefined: perf.Attr
./tracer.go:1121:17: undefined: perf.CPUClock
./tracer.go:1133:26: undefined: perf.Open
./tracer.go:1133:51: undefined: perf.AllThreads

Anyways, I am not against it, and all for it, just will do it later.

@korniltsev
Copy link
Copy Markdown
Contributor Author

Or did you mean to handle this in the dependent code and not touch the rlimit at all?

@florianl
Copy link
Copy Markdown
Member

I would prefer to attempt doing this separately.

To me, a rollback of this change would be easier if such dependent packages would also be updated accordingly.

Or did you mean to handle this in the dependent code and not touch the rlimit at all?

I don't have a preference on this.

@korniltsev
Copy link
Copy Markdown
Contributor Author

On a second thought I would rather do it once in rlimit package, than twice in tracer and processmanager/ebpf.

@florianl Would you prefer to keep the current approach of the current PR or would you prefer me to open an alternative PR for the processmanager/ebpf only. tracer will be handled later anyways.

@korniltsev
Copy link
Copy Markdown
Contributor Author

Just hand another idea
We likely will not need any workarounds in the rlimit package if we split the processmanager/ebpf package in half. One package for the EbpfHandler interface and maybe some others, and then the rest could go to a separate implementation specific package. And so the tracer would depend on the implementation. And coredump tests should compile just fine without dependency on the new implementation package. WDYT?

@korniltsev
Copy link
Copy Markdown
Contributor Author

Just hand another idea
We likely will not need any workarounds in the rlimit package if we split the processmanager/ebpf package in half. One package for the EbpfHandler interface and maybe some others, and then the rest could go to a separate implementation specific package. And so the tracer would depend on the implementation. And coredump tests should compile just fine without dependency on the new implementation package. WDYT?

Something like this:
#625

This allows compiling and starting coredump tests on darwin already

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.

2 participants