-
Notifications
You must be signed in to change notification settings - Fork 487
[Kernel scoping 4/5] Add TraceProgram object #1687
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
Conversation
83b3541 to
f14cff1
Compare
Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
f14cff1 to
daf54c5
Compare
Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
| literal_bpf_trace_min = | ||
| std::regex_replace(literal_bpf_trace_min, std::regex(R"(\\\n)"), R"(\\\\n)"); | ||
| literal_bpf_trace_min = std::regex_replace(literal_bpf_trace_min, std::regex("\n"), "\\n"); | ||
| literal_bpf_trace_min = std::regex_replace(literal_bpf_trace_min, std::regex("\""), "\\\""); |
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.
Do we have to use regular expressions? It seems the other tests use absl::Substitute and imo it's much more readable.
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.
Thanks for pointing that out - I've removed a few calls to regex_replace which I believe are unneeded.
I think absl::Substitute is used for parameterized string substitution rather than pattern-based substitution which we're doing here. The existing test case for bpftrace strings on lines 635-649 also makes use of regex_replace, so I simply copied those over. They were added in this commit to escape newlines.
Another option would be to use absl::StrReplaceAll, though I ran into the error: no member named 'StrReplaceAll' in namespace 'absl'
ddelnano
left a comment
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.
Just one comment about the use of regular expressions, but otherwise lgtm!
Signed-off-by: Benjamin Kilimnik <bkilimnik@pixielabs.ai>
Summary: Adds new
TraceProgramobject topxtracewhich accepts a BPFtrace program and selectors to specify deployment restrictions as key value arguments. Also modifies theprobe_fnargument topxtrace.UpsertTracepointto accept a singleTraceProgramobject or a list of them, extracting selectors and populatingTracepointDeploymentas required, so that the query broker can register tracepoints with agents that match those selectors.Example usage
Related issues: #1582. Closes #1659.
Type of change: /kind feature
Test Plan: Tested all existing targets, added compiler tests to
src/carnot/planner/logical_planner_test.ccandsrc/carnot/planner/probes/probes_test.cc, and skaffolded changes to a cluster, verifying expected behavior and interoperability with selectors introduced in PR #1686.Known issues
TracepointDeployment, stirling throws an errorstirling.cc:511error::Internal("Only one Tracepoint is currently supported.");. This is an existing limitation, which is a little more exposed as a result of the selector feature introduced in this PR. To mitigate this, we could add documentation, improve the error message, and/or have just one of the bpftrace programs in theTracepointDeploymentrun.pxtrace.UpsertTracepointcall differ, only one of the schemas will be used to the create the output table (currently the first one that is processed). When the program whose schema differs tries to push a record to the table, we get the errorsource_connector.cc:64] Failed to push data. Message = Table_id ## doesn't exist.The user has to manually create another table/make another call toUpsertTracepointif they want to deploy bpftrace programs with different schemas.