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

feat!: data field decode types #4353

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

NDStrahilevitz
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz commented Oct 16, 2024

1. Explain what the PR does

9e4e121 chore(integration): align to data decode types
14aa920 feat!(events): normalise data presentation types
d915bdd chore(bufferdecoder): readability refactors
d4892fa feat(events): streamline event data decoding types

14aa920 feat!(events): normalise data presentation types

(Almost) all event data type field names (post-decode) should now
conform to go type names. This will ensure a more consistent and simpler
format for event data.

d915bdd chore(bufferdecoder): readability refactors

Move some functions around, rename and improve documentation.

d4892fa feat(events): streamline event data decoding types

1. Reduce the type variance in data fields via hardcoding possible types
   decodable from the kernel.
2. Move data field decode types to the types/trace package
3. Make corresponding changes in eBPF and test code

2. Explain how to test it

  1. Nothing should functionally change
  2. Events will be emitted with new and simplified field types (BREAKING CHANGE)

3. Other comments


switch argType {
case u8T:
case trace.U8_T:
Copy link
Member

@geyslan geyslan Oct 17, 2024

Choose a reason for hiding this comment

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

How about reducing the size of readArgFromBuff() by replacing the switch/if branches with an already initialized array containing pointer functions for each type (ebpfMsgDecoder.Decode*)?

Copy link
Collaborator Author

@NDStrahilevitz NDStrahilevitz Oct 17, 2024

Choose a reason for hiding this comment

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

This is basically the planned next step. I plan to move the decoding mapping to a hash map similar to the processor and derive functions. In this way we, and plugin authors, can easily introduce new decoding strategies.
A useful example already exists in the code: we have two different submission formats for string arrays. Therefore we have a single "presentation type" with two "decoding strategies".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@geyslan I've opted not to make this step in this PR but rather in a followup so that we merge smaller chunks at a time as this is a large scale change.

@NDStrahilevitz NDStrahilevitz force-pushed the hard_type_args branch 2 times, most recently from 74d839d to 9df64bd Compare November 8, 2024 11:28
@NDStrahilevitz NDStrahilevitz changed the title [DRAFT] chore!: concrete type arguments [DRAFT] feat!: concrete type arguments Nov 8, 2024
@NDStrahilevitz NDStrahilevitz changed the title [DRAFT] feat!: concrete type arguments feat: concrete type arguments Nov 8, 2024
@NDStrahilevitz NDStrahilevitz marked this pull request as ready for review November 8, 2024 11:28
@NDStrahilevitz NDStrahilevitz marked this pull request as draft November 8, 2024 11:36
@NDStrahilevitz NDStrahilevitz changed the title feat: concrete type arguments [WIP] feat: concrete type arguments Nov 8, 2024
Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

Not sure what we are trying to achieve in this PR

pkg/ebpf/c/common/buffer.h Show resolved Hide resolved
pkg/ebpf/c/common/buffer.h Outdated Show resolved Hide resolved
pkg/ebpf/tracee.go Outdated Show resolved Hide resolved
@NDStrahilevitz
Copy link
Collaborator Author

Not sure what we are trying to achieve in this PR

The PR has two goals:

  1. Reduce the type variance in data field declaration. We use concrete types instead, this is already achieved in the context of the below "decode types"
  2. Decouple the single data type into a 1. decode strategy type (encoding the protocol between kernel and decoder, this should be a registerable method in the future) 2. presentation data type which encodes what kind of data the user will receive when reading the value (a float, an int, a string, some array, a timestamp, so on).

Currently only the first goal is achieved.

@NDStrahilevitz NDStrahilevitz force-pushed the hard_type_args branch 2 times, most recently from f56e9bc to 8fc7341 Compare November 21, 2024 13:03
@NDStrahilevitz NDStrahilevitz force-pushed the hard_type_args branch 3 times, most recently from 5fe70f2 to c951d66 Compare December 12, 2024 11:23
1. Reduce the type variance in data fields via hardcoding possible types
   decodable from the kernel.
2. Move data field decode types to the types/trace package
3. Make corresponding changes in eBPF and test code
Move some functions around, rename and improve documentation.
@NDStrahilevitz NDStrahilevitz force-pushed the hard_type_args branch 3 times, most recently from 347f47f to ecc5fde Compare December 19, 2024 10:31
@NDStrahilevitz NDStrahilevitz changed the title [WIP] feat: concrete type arguments feat: concrete type arguments Dec 19, 2024
@NDStrahilevitz NDStrahilevitz marked this pull request as ready for review December 19, 2024 11:49
@NDStrahilevitz NDStrahilevitz changed the title feat: concrete type arguments feat: data field decode types Dec 19, 2024
@NDStrahilevitz NDStrahilevitz changed the title feat: data field decode types feat!: data field decode types Dec 19, 2024
(Almost) all event data type field names (post-decode) should now
conform to go type names. This will ensure a more consistent and simpler
format for event data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants