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

Attempt to run without syscall tracepoints, if possible. #175

Merged
merged 9 commits into from
Nov 4, 2024

Conversation

umanwizard
Copy link
Contributor

This implements the proposal in
#173 . Please see there for full details.

@umanwizard umanwizard requested review from a team as code owners October 9, 2024 12:37
@umanwizard umanwizard force-pushed the remove-probe-tracepoint branch 2 times, most recently from 9b369d0 to b417df1 Compare October 9, 2024 13:05
Copy link
Member

@christos68k christos68k left a comment

Choose a reason for hiding this comment

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

There are a few linter failutes. Other than that, since we now allow running the agent without syscall tracepoints, we should probably also remove the eBPF integration test and its dependent hook.

CC: @florianl

Copy link
Contributor

@florianl florianl left a comment

Choose a reason for hiding this comment

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

[..] we should probably also remove the eBPF integration test and its dependent hook.

👍 on that

tracer/maccess.go Outdated Show resolved Hide resolved
@fabled
Copy link
Contributor

fabled commented Oct 28, 2024

@umanwizard Are you working on the feedback or do you need help to finalize this PR?

@umanwizard
Copy link
Contributor Author

Sorry, I dropped this. I'll address the feedback and send a new version soon.

@umanwizard
Copy link
Contributor Author

I'm not sure I understand why we should remove the integration test. Isn't it still useful to have this integration test run? We just need to ensure that it is always running in an environment that has syscall probes available (which is the vast majority of environments)

@florianl
Copy link
Contributor

I'm not sure I understand why we should remove the integration test.

With #205 a test was added that covers also the functionality of the named integration test for the tracepobe. And instead of a specific tracepoint that is written for the integration test, #205 tests the tracepoint that is actually used while running the eBPF profiler agent.

@umanwizard
Copy link
Contributor Author

Okay. In that case we can indeed remove it.

Copy link
Contributor

@florianl florianl left a comment

Choose a reason for hiding this comment

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

With the PR only the updated support/ebpf/tracer.ebpf.release.arm64 is provided. Please also add the updated support/ebpf/tracer.ebpf.release.amd64.

Copy link
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@christos68k christos68k merged commit 7c5686f into open-telemetry:main Nov 4, 2024
23 checks passed
dmathieu pushed a commit to dmathieu/opentelemetry-ebpf-profiler that referenced this pull request Nov 5, 2024
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.

4 participants