-
Notifications
You must be signed in to change notification settings - Fork 586
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
feat(ebpf): add pyperf #2201
feat(ebpf): add pyperf #2201
Conversation
… only filename instead of full paht
ebpf/python/pyperf.go
Outdated
if len(s.events) == 0 { | ||
return nil | ||
} | ||
eventsCopy := make([]*PerfPyEvent, len(s.events)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better pattern would be to take a slice as a parameter and grow it if needed then returning it, this would also to reuse the slice for the caller.
I also wonder if []*PerfPyEvent
shouldn't be []PerfPyEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason I chose to use a pointer is because PerfPyEvent is quite big - 320 bytes
and there could be quite a lot of them 100 times per second * 15 seconds * 16 cores * 320 bytes =~7M
And i did not want to move this memory twice. It is likely a premature optimization already. At the same time I dont see this function hot, so I suggest we keep it as is and optimize if we have a problem or any evidence it can be optimized
it := m.Iterate() | ||
|
||
v := uint32(0) | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not for it.Next(k, &v) {
?
func (s *session) updateSampleRate(sampleRate int) error { | ||
if s.options.SampleRate == sampleRate { | ||
return nil | ||
func (s *session) readEvents(events *perf.Reader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test for session.go and I think it would be great to think about it since you have many locks and goroutine, this means most likely add a interface for the perf package used.
Just to make sure it calls correctly perf and doesn't leak and has a race.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree, I want this a lot, however mocking out the whole cilium/ebpf library is not trivial, there's a lot of structures used, like maps, programs, perf reader. Lets create an issue and improve later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think we should have more test for the session.go file.
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
* Rename deploy-monitoring (#2551) * Tweak compaction configuration and performance. (#2564) * Tweak compaction configuration and performance. * Fixes config tests * review feedback * Fixes compaction range block grouping (#2570) * Update `make docs` procedure (#2567) Co-authored-by: grafanabot <bot@grafana.com> * docs: improves naming in rideshare example (#2571) * Revert "fix: remove "Flamegraph" from timeline chart title (#2565)" (#2566) * Revert "fix: remove "Flamegraph" from timeline chart title (#2565)" This reverts commit b4e6cef. * fix: add time range * Add support for dashboards datasource filter (#2560) This commit add support for dashboards datasource filter using a regex named 'datasource_regex' provided in the config.libsonnet file. It's done in a similar way than for grafana/mimir. Signed-off-by: julien.girard <julien.girard@ovhcloud.com> * Increase default compaction interval (#2575) * feat(ebpf): add pyperf (#2201) * Document the new configuration flag PYROSCOPE_AGENT_ENABLED (#2536) * Ignore block within 3h in the store-gateway (#2579) * chore(ebpf) py312 support (#2577) * add py312 testdata * chore(ebpf): py 3.12 support * unhardcode more offsets * ingest: drop empty pprof profiles (#2580) * ingest: drop empty pprof profiles * add testcase * Make sure loki extracts the right labels (#2582) * docs: fix the "Type mismatch" error in "Rust client configuration" (#2581) --------- Signed-off-by: julien.girard <julien.girard@ovhcloud.com> Co-authored-by: Christian Simon <simon@swine.de> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: grafanabot <bot@grafana.com> Co-authored-by: Dmitry Filimonov <dmitry.filimonov@grafana.com> Co-authored-by: Darren Janeczek <38694490+darrenjaneczek@users.noreply.github.com> Co-authored-by: Julien Girard <6936729+bubu11e@users.noreply.github.com> Co-authored-by: Tolya Korniltsev <korniltsev.anatoly@gmail.com> Co-authored-by: Franz Wimmer <7378020+zalintyre@users.noreply.github.com> Co-authored-by: Daxin Wang <46807570+dxsup@users.noreply.github.com>
This PR adds a python profiling to ebpf module
It is based on https://github.com/iovisor/bcc/blob/master/examples/cpp/pyperf/PyPerfBPFProgram.cc , rewriten without bcc, added support for other python versions and musl libc
Future improvements:
fix fork/execve race - if process forks we may observe it before exec, therefore selecting wrong profiling type, for example python before execve /bin/sh or the other way around