new(userspace): plugin api to dump async events #2152
+230
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
poiana
added
release-note
kind/feature
|
/milestone 0.19.0 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
FedeDP
commented
Nov 7, 2024
| @@ -159,7 +159,7 @@ bool sinsp::is_initialstate_event(scap_evt* pevent) const { | |||
| return pevent->type == PPME_CONTAINER_E || pevent->type == PPME_CONTAINER_JSON_E || | |||
| pevent->type == PPME_CONTAINER_JSON_2_E || pevent->type == PPME_USER_ADDED_E || | |||
| pevent->type == PPME_USER_DELETED_E || pevent->type == PPME_GROUP_ADDED_E || | |||
| pevent->type == PPME_GROUP_DELETED_E; | |||
| pevent->type == PPME_GROUP_DELETED_E || pevent->type == PPME_ASYNCEVENT_E; | |||
There was a problem hiding this comment.
Initial state events parsing enablement for PPME_ASYNCEVENT_E.
FedeDP
commented
Nov 7, 2024
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
|
/hold |
FedeDP
force-pushed
the
new/plugin_api_dump
branch
from
56f40b5
to
6615ce7
Compare
|
/unhold |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2152 +/- ##
==========================================
+ Coverage 74.82% 74.84% +0.02%
==========================================
Files 254 255 +1
Lines 33519 33620 +101
Branches 5749 5779 +30
==========================================
+ Hits 25079 25162 +83
- Misses 8440 8458 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
FedeDP
force-pushed
the
new/plugin_api_dump
branch
from
a58b66a
to
ef8f0cc
Compare
userspace/plugin/plugin_api.h
Outdated
| // pointer is not retained by the handler after it returns. | ||
| // | ||
| // Return value: A ss_plugin_rc with values SS_PLUGIN_SUCCESS or SS_PLUGIN_FAILURE. | ||
| ss_plugin_rc (*dump)(ss_plugin_t* s, |
There was a problem hiding this comment.
Suggested change
| ss_plugin_rc (*dump)(ss_plugin_t* s, | |
| ss_plugin_rc (*dump_state)(ss_plugin_t* s, |
Not sure what's the right name for this... but I think that giving a more scoped name (I'll trust you with that) would save us some headaches maybe in the future. @gnosek since you envisioned this to happen too some time ago, you have any naming suggestions?
There was a problem hiding this comment.
Agree with the naming, gonna change it asap!
| @@ -1053,6 +1053,25 @@ typedef struct { | |||
| ss_plugin_rc (*set_async_event_handler)(ss_plugin_t* s, | |||
| ss_plugin_owner_t* owner, | |||
| const ss_plugin_async_event_handler_t handler); | |||
|
|
|||
There was a problem hiding this comment.
I don't fully enjoy the fact that this is tied to the async events capability. For instances, certain plugins may collect some state through parse_event and make it available through the table API too, but maybe will never emit async events.
However, I see the point. It also makes sense to "recycle" get_async_event_sources and get_async_events, since they are indeed useful for this use case. Given that this symbol is optional, my interpretation is that defining set_async_event_handler will be a requirement even if not used. If so, we'll have the opportunity later on to change our minds and loosen the requirements without incurring in a breaking change. So I think we can sleep peacefully with this for now 👍
| return false; | ||
| } | ||
|
|
||
| m_async_dump_handler = [&dumper](auto e) { dumper.dump(e.get()); }; |
There was a problem hiding this comment.
I assume we don't have thread safety issue here compared to the async event handler, right?
Also, ASYNCEVENT_E will not be correctly pre-parsed at init time while reading from captures. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
We now expect `PPME_ASYNCEVENT_E` whose `name` matches one of the plugin supported ones (get_async_events() API). The new API is not required for async capability. Added also a test. Signed-off-by: Federico Di Pierro <[email protected]>
Other tests using no_driver engine and async plugin are also disabled on it. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
…_t` callback. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]> Co-authored-by: Jason Dellaluce <[email protected]>
FedeDP
force-pushed
the
new/plugin_api_dump
branch
from
5dbd02e
to
7fcd4dc
Compare
|
Pushed with the proposed changes! |
|
Moving to next milestone. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
This PR adds a new
dump()API under theasync eventsplugin capability.It is used to dump plugin state (as list of
PPME_ASYNCEVENT_Eevents) when a scap file dump is requested.Note:
PPME_ASYNCEVENT_Ecan be dumpedPPME_ASYNCEVENT_Edumped must have a name supported by the plugin (seeget_async_events()API)Moreover, enable initialstate consuming of
PPME_ASYNCEVENT_Eevents while opening the inspector.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: