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

pkg/run: Don't rely on GADGET_TRACER as it is optional #2518

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

blanquicet
Copy link
Member

Don't rely on GADGET_TRACER as it is optional

GetTracerInfo gets the tracer information from what was defined by
the GADGET_TRACER macro. However, such macro is optional, so we
must not rely on it. Additionally, by the time
handleTracerMapDefinition() is called, the metadata was already
built (either from the metadata file or from the eBPF code), so we
can get the tracer information from there.

GetTracerInfo gets the tracer information from what was defined by
the GADGET_TRACER macro. However, such macro is optional, so we
must not rely on it. Additionally, by the time
handleTracerMapDefinition() is called, the metadata was already
built (either from the metadata file or from the eBPF code), so we
can get the tracer information from there.

Signed-off-by: Jose Blanquicet <josebl@microsoft.com>
Signed-off-by: Jose Blanquicet <josebl@microsoft.com>
This reverts commit 4ffe89c. It is not
necessary anymore to export neither TracerInfo nor GetTracerInfo()
because we don't need to use it outside the gadgets/run/types pakage.

Signed-off-by: Jose Blanquicet <josebl@microsoft.com>
Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi.

From code inspection, this looks OK.
However, I would question the whole existence of this macro.
If it is optional, we should just drop it and not add another level of complexity.
Otherwise, we should document in which case it is mandatory to use it. I suspect this documentation to no more be up to date:

> The `tracers` and `structs` sections will only be generated if the eBPF program defined a tracer
> using the `GADGET_TRACER` macro.

Best regards.

@blanquicet
Copy link
Member Author

Hi.

Hola Francis 🙂

I would question the whole existence of this macro. If it is optional, we should just drop it and not add another level of complexity. Otherwise, we should document in which case it is mandatory to use it.

The macro GADGET_TRACER is only useful to allow IG autogenerating the tracers and structs section of the metadata file. However, if developer created the metadata manually, the macro is not necessary anymore. That's why it is optional. That's also why the documentation says "The tracers and structs sections will only be generated if the eBPF program defined a tracer using the GADGET_TRACER macro", so ...

I suspect this documentation to no more be up to date:

... no, it is up to date. Actually, I recently updated it in this PR the documentation to make this optionality clearer:

This information enables Inspektor Gadget to generate the metadata file automatically.
Refer to the [metadata file](#creating-a-metadata-file) section for detailed instructions.
```c
// [Optional] Define a tracer
GADGET_TRACER(open, events, event);
```

If you consider it is not yet clear enough, would you mind opening a PR to improve the documentation?

@eiffel-fl
Copy link
Member

Hi.

Hola Francis 🙂

I would question the whole existence of this macro. If it is optional, we should just drop it and not add another level of complexity. Otherwise, we should document in which case it is mandatory to use it.

The macro GADGET_TRACER is only useful to allow IG autogenerating the tracers and structs section of the metadata file. However, if developer created the metadata manually, the macro is not necessary anymore. That's why it is optional. That's also why the documentation says "The tracers and structs sections will only be generated if the eBPF program defined a tracer using the GADGET_TRACER macro", so ...

I suspect this documentation to no more be up to date:

... no, it is up to date. Actually, I recently updated it in this PR the documentation to make this optionality clearer:

This information enables Inspektor Gadget to generate the metadata file automatically.
Refer to the [metadata file](#creating-a-metadata-file) section for detailed instructions.
```c
// [Optional] Define a tracer
GADGET_TRACER(open, events, event);
```

If you consider it is not yet clear enough, would you mind opening a PR to improve the documentation?

If this metadata file is optional, then why do we need it?
We should go straight to the point and not bothering about optional stuff, if this is optional, we just drop it and move forward.

@mauriciovasquezbernal
Copy link
Member

If this metadata file is optional, then why do we need it?

The metadata file allows to define extra attributes for the gadget as the format used to print the fields, and in the future documentation and all other stuff we can't put directly in the eBPF code. It's optional because the gadget can work without it, we don't want to force gadget developers creating this file if they don't want to define any of these extra attributes.

@blanquicet blanquicet merged commit 2340dd4 into main Feb 21, 2024
59 checks passed
@blanquicet blanquicet deleted the jose/use-metadata branch February 21, 2024 12:48
@blanquicet
Copy link
Member Author

If this metadata file is optional, then why do we need it?

The metadata file allows to define extra attributes for the gadget as the format used to print the fields, and in the future documentation and all other stuff we can't put directly in the eBPF code. It's optional because the gadget can work without it, we don't want to force gadget developers creating this file if they don't want to define any of these extra attributes.

I agree the metadata file should be optional, but I'm not fully convinced about the GADGET_TRACER. Do you really expect users to create the metadata file manually? I'm not that convinced. In addition, even if they define it by hand, they won't be able to keep it automatically updated using the --update-metadata flag without defining the GADGET_TRACER.

Let's discuss it in IG Developer Sync meeting today.

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.

None yet

3 participants