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

Add USDT support. #171

Merged
merged 10 commits into from
Mar 29, 2021
Merged

Add USDT support. #171

merged 10 commits into from
Mar 29, 2021

Conversation

tobz
Copy link
Contributor

@tobz tobz commented Mar 22, 2021

This PR adds USDT support, based on a transliteration of the Python implementation in iovisor/bcc.

There's a lot of "icing" here as well: cleaning up styling, formatting, condensing code, etc.

Closes #167

@tobz tobz marked this pull request as draft March 22, 2021 20:29
src/core/mod.rs Outdated Show resolved Hide resolved
src/core/mod.rs Outdated Show resolved Hide resolved
src/uprobe/mod.rs Outdated Show resolved Hide resolved
src/usdt/mod.rs Outdated Show resolved Hide resolved
src/usdt/mod.rs Outdated Show resolved Hide resolved
src/usdt/mod.rs Outdated Show resolved Hide resolved
src/uprobe/mod.rs Outdated Show resolved Hide resolved
@brayniac
Copy link
Collaborator

Thanks for working on this. Overall it looks pretty good.

src/core/mod.rs Outdated Show resolved Hide resolved
@brayniac
Copy link
Collaborator

There's a few other things that clippy is pointing out in this PR. Would be good to address the ones in the new code (src/usdt/mod.rs) - I think I commented on the only new one in core, there's some existing code causing some clippy warnings which can be ignored for the sake of this PR.

src/usdt/mod.rs Outdated Show resolved Hide resolved
src/usdt/mod.rs Outdated Show resolved Hide resolved
@javierhonduco
Copy link
Contributor

Very interested in this! Happy to take a look once it's ready for review! :)

@tobz
Copy link
Contributor Author

tobz commented Mar 26, 2021

Went through and did a bunch of cleanup and various bits, namely....

  • There's now a usdt feature flag, added only by the versions that support the C API functions we required.
  • Per the discussion about the error message around converting to CString, I've cleaned up all instances where a CString was being made, and added a contextual error message properly describing the error.
  • Moved the usdt module into core so it could follow the file-per-major-API-changes approach.
  • Tried my best to collapse redundant code in BPFBuilder.
  • Fixed import groupings where applicable.

I realize that the CString stuff might be too noisy when trying to properly review the core aspect of this PR -- USDT support -- so I'm happy to revert those changes if need be.

@tobz tobz marked this pull request as ready for review March 26, 2021 19:04
src/error.rs Outdated Show resolved Hide resolved
@tobz tobz changed the title [WIP] Add USDT support. Add USDT support. Mar 26, 2021
Cargo.toml Outdated Show resolved Hide resolved
@brayniac
Copy link
Collaborator

Thanks @tobz - this is looking pretty solid now. I'll do one more pass of review once CI is green. Appreciate the contribution.

Copy link
Collaborator

@brayniac brayniac left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for the contribution. I'll proceed to merge and generate a new release.

@brayniac brayniac merged commit 1be868c into rust-bpf:master Mar 29, 2021
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.

USDT probe support
3 participants