pyroscope.ebpf: otel profiler implementation#2920
Conversation
d9663da to
3779c4e
Compare
3779c4e to
7c540d2
Compare
7c540d2 to
ca84882
Compare
|
💻 Deploy preview deleted. |
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
marcsanmi
left a comment
There was a problem hiding this comment.
LGTM 👏 , just left some nits!
| | `python_enabled` | `bool` | A flag to enable or disable python profiling. | `true` | no | | ||
| | `ruby_enabled` | `bool` | A flag to enable or disable Ruby profiling. | `true` | no | | ||
| | `same_file_cache_size` | `int` | Deprecated (no-op), previously controlled the size of the elf file -> symbols table LRU cache. | `8` | no | | ||
| | `sample_rate` | `int` | How many times per second to collect profile samples. | `97` | no | |
There was a problem hiding this comment.
I think the new default is 19?
| | `go_table_fallback` | `bool` | Deprecated (no-op), previously enabled symbol lookup in `.sym` / `.dynsym` sections when `.gopclntab` lookup failed. | `false` | no | | ||
| | `hotspot_enabled` | `bool` | A flag to enable ordisable hotspot profiling. | `true` | no | | ||
| | `perl_enabled` | `bool` | A flag to enable or disable Perl profiling. | `true` | no | | ||
| | `php_enabled` | `bool` | A flag to enable or disable PHP profiling. | `true` | no | |
There was a problem hiding this comment.
Should go_enabled be also documented?
There was a problem hiding this comment.
The current revision of the go consumes shit ton of memory and cpu and it works much better without it (i.e. using our symbolizer). We may want to add go here after we update our fork to include open-telemetry/opentelemetry-ebpf-profiler#456 which is not straightforward because it removes the rust lib and we rely on it. tldr - yes, but later.
| | `perl_enabled` | `bool` | A flag to enable or disable Perl profiling. | `true` | no | | ||
| | `php_enabled` | `bool` | A flag to enable or disable PHP profiling. | `true` | no | | ||
| | `pid_cache_size` | `int` | Deprecated (no-op), previously controlled the size of the PID -> proc symbols table LRU cache. | `32` | no | | ||
| | `pid_map_size` | `int` | The size of eBPF PID map. | `2048` | no | |
There was a problem hiding this comment.
This is no longer used? Do you keep it for backwards compatibility? I think we should either update it to deprecated or just remove it.
There was a problem hiding this comment.
Nice catch. deprecated. We may want to expose tracer.MapScaleFactor in the future.
| time.Sleep(c.cfg.ReporterInterval) | ||
| continue | ||
| } | ||
| defer ctlr.Shutdown() |
There was a problem hiding this comment.
Do we need to cleanup FileObserver to avoid resource leaking?
defer func() {
ctlr.Shutdown()
if c.cfg.FileObserver != nil {
c.cfg.FileObserver.Cleanup()
}
}()There was a problem hiding this comment.
thanks, nice catch
PR Description
This PR changes the implementation of
pyroscope.ebpfcompoenent from grafana/pyroscope/ebpf to opentelemetry-ebpf-profiler - a fork of opentelemetry-ebpf-profiler.The notable changes of the fork: native frame symbolization, dynamic profiling policy, target discovery labels.
The fork uses a rust library for symbol extraction. See https://github.com/open-telemetry/opentelemetry-ebpf-profiler/tree/main/rust-crates.
In this PR I put all
pyroscope.ebpfcode behind apyroscope_ebpfgo tag.Some component arguments became no-op as there are no corresponding knobs in the new implementations. These args a deprecated
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist