Untangle the section id, factories and modules #421
Merged
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
atenart
force-pushed
the
at/section-id-untangle
branch
from
2d000f2
to
46be6ba
Compare
|
Again we have a failure due to benchmarking being so difficult to maintain. I'm not going to fix this here as we have #412; so let's merge it first. |
amorenoz
reviewed
Sep 25, 2024
|
Mainly I have comments on the last two patches. For the code refactorization, besides nits, it looks great to me and I think it goes in the right direction of getting rid of the module abstraction. |
atenart
force-pushed
the
at/section-id-untangle
branch
from
46be6ba
to
8cc0c84
Compare
|
Rebased on top of #411 (as it seems to be ready) to avoid conflicts and removed the common type commits from this. I'll open a dedicated PR and we can have the discussion about those there. |
|
Same CI error as the others, this week our running CI can't work. |
atenart
force-pushed
the
at/section-id-untangle
branch
from
8cc0c84
to
3d89873
Compare
|
Removed the same commit as in #411. |
|
The benchmarking CI is actually failing for a good reason; will have a look. |
atenart
force-pushed
the
at/section-id-untangle
branch
from
3d89873
to
daf3558
Compare
|
All good now. |
amorenoz
reviewed
Sep 30, 2024
amorenoz
previously approved these changes
Sep 30, 2024
atenart
force-pushed
the
at/section-id-untangle
branch
from
daf3558
to
cbdf8ff
Compare
|
Renamed |
Since commit 2373796 ("events: split ebpf and json factories") raw factories are only parsing and converting raw events, aka BPF ones. Thus we no longer have to differentiate the user of such factories and can make raw factory implementations cleaner. Signed-off-by: Antoine Tenart <[email protected]>
This makes modules not required to produce events, as they can provide value by doing other scoped work (eg. installing probes). This shows again the limits of our module abstraction and it might be removed at some point to clean things up a bit. Signed-off-by: Antoine Tenart <[email protected]>
SectionId member can be directly casted as u8, there's no need to maintain an helper doing so. Signed-off-by: Antoine Tenart <[email protected]>
We no longer have a need to split both definitions, EventSection is never used outside of event_section. Signed-off-by: Antoine Tenart <[email protected]>
Those two were loosely tightened, when adding sub-sections to an event. Even more, the section names where both defined in the event sections and in the section ids. As there should be a 1:1 relationship between event sections and ids, make the id part of the event section. This has another advantage as a side effect: raw section factories do not need a 1:1 relationship with an event section and can generate any event section. Signed-off-by: Antoine Tenart <[email protected]>
Signed-off-by: Antoine Tenart <[email protected]>
Stop using SectionId for indexing event section factories; those are tied to a BPF counterpart, not to a section id. This helps modelizing that a factory can produce different sections and that a section does not necessarily have a BPF counterpart, such as the Tracking one. To do this use a dedicated FactoryId enum instead of reusing SectionId and use a similar logic as what we do for event sections (eg. embed the id into the factory definition). While doing this we can remove the TrackingInfoEventFactory. Signed-off-by: Antoine Tenart <[email protected]>
Stop using SectionId for indexing modules (aka. collectors nowadays); there is no 1:1 relationship between the two. Eg. core events are not tied to a module. To to this introduce a ModuleId enum. Hopefully this will go away once we remove the module abstraction. Signed-off-by: Antoine Tenart <[email protected]>
Cosmetic only. Signed-off-by: Antoine Tenart <[email protected]>
atenart
force-pushed
the
at/section-id-untangle
branch
from
cbdf8ff
to
717bfee
Compare
|
Rebased on top of main to fix a conflict. No other change. |
amorenoz
approved these changes
Oct 7, 2024
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.
Semi-RFC material; since code is ready let's not flag it but feel free to discuss the approach :)
The main goal of this series is $title, and some preliminary work for a possible future removal of the module concept. In any way this solve issues linked to how the section ids were used which did not map to the reality:
This also now allows to have a factory producing more than a single type of event section; this is no side effect and is intended.
Overall this was something I wanted to tackle for some time now, but I was keeping it for later. But adding support for
enum sk_rst_reasonshowed w/o this preliminary rework things would have gotten even worse between event sections, factories and modules. This is why I chose to keep those patches in that PR; but I can split into two PRs if needed.One question I have is with the last commit: should be put all those "common types" into the same event section or add one for each. Having a dedicated section helps for possible future improvement (more fields when applicable), but makes event section count to potentially grow quite a lot. Also I should find a better name instead of "common types".