Skip to content
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

Merged
merged 97 commits into from Oct 25, 2023
Merged

feat(ebpf): add pyperf #2201

merged 97 commits into from Oct 25, 2023

Conversation

korniltsev
Copy link
Collaborator

@korniltsev korniltsev commented Aug 3, 2023

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
  • fix class name retrieval for a strange case 3.9 see NullCls in pyperf.bpf.c
  • support for python 3.12
  • support for python 2.7
  • CI step to verify if the PR has correctly regenerated the files
  • e2e tests (maybe using cmd/playground)

@petethepig petethepig changed the base branch from next to main August 29, 2023 19:38
ebpf/python/pyperf.go Outdated Show resolved Hide resolved
ebpf/python/pyperf.go Outdated Show resolved Hide resolved
if len(s.events) == 0 {
return nil
}
eventsCopy := make([]*PerfPyEvent, len(s.events))
Copy link
Contributor

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

Copy link
Collaborator Author

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 {
Copy link
Contributor

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) { ?

ebpf/python/pyperf.go Outdated Show resolved Hide resolved
func (s *session) updateSampleRate(sampleRate int) error {
if s.options.SampleRate == sampleRate {
return nil
func (s *session) readEvents(events *perf.Reader,
Copy link
Contributor

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@cyriltovena cyriltovena left a 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.

@korniltsev korniltsev merged commit f1b82c5 into main Oct 25, 2023
16 checks passed
@korniltsev korniltsev deleted the pyperf2 branch October 25, 2023 03:05
cyriltovena added a commit that referenced this pull request Oct 25, 2023
* 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>
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.

None yet

2 participants