Refactor Probe API #1307
Refactor Probe API #1307
Conversation
There was a problem hiding this comment.
I'm generally not in favor of this design.
- It has added interface complexity for the theoretical possibility of future additions.
- Deeply nesting these interface types will make this code harder to read and maintain. Instead we should strive for a flatter structure and use composition to achieve the desired functionality. Ideally our interfaces will be small and focused. For example, we could have a
Probeinterface:And then have atype Probe interface { // Base probe functionality... }RunCloserinterface:Though we need to motivate why atype RunCloser interface { Run() Close() }Probewould never need to have aRun/Closemethod. - Use-cases of probes we plan to implement would help motivate this change. It would also help us define our interfaces. I think it is ambitious to want and provide a general probe running platform, but at some point we will just be providing an alternate API for the Cilium project. We need to make sure our efforts are targeted at providing auto-instrumentation.
- Deeply nesting these interface types will make this code harder to read and maintain. Instead we should strive for a flatter structure and use composition to achieve the desired functionality. Ideally our interfaces will be small and focused. For example, we could have a
- This does not address the multi-process issue from Auto Instrumentation - Monitor multiple processes #197. If we plan to release this design to end-users we need to ensure this will be supported. Otherwise, we will ship an immediately outdated design as we try to progress.
- The load and targeting mechanisms are set by assignment to returned values.
- File names of the probe package a based on Go declarations not the functionality and types they contain. This will make it harder to find code and is not idiomatic with the rest of the project.
- The
utilspackage is separate from the probes, specificallyInitializeEBPFCollection. All probes will need to maintain similar BPF filesystem utilities, and much of this will be duplicative or conflicting if we cannot somehow incorporate it into the probe factories or types.
| ID() ID | ||
|
|
||
| // GetLogger returns an *slog.Logger for this Probe. | ||
| GetLogger() *slog.Logger |
There was a problem hiding this comment.
Isn't this the opposite of what we want in that we want the probes to use the instrumentation logger? When would we need to have the logger from the probe returned?
| } | ||
|
|
||
| // If Probe is used, pass target details to Probe | ||
| p.TargetConfig().TargetDetails = target |
There was a problem hiding this comment.
This is setting a value on a returned type from TargetConfig. This is not idiomatic of Go, parameters should be passed to function calls not set on returned values.
|
|
||
| // Load loads the eBPF programs and maps required by the Probe into memory. | ||
| // The specific types of programs and maps are implemented by the Probe. | ||
| Load() error |
There was a problem hiding this comment.
How do you specify what to load?
How will this handle multiple load targets when we support #197
| // BaseProbe is the most basic type of Probe. It must support configuration, | ||
| // loading and attaching eBPF programs, running, and closing. | ||
| // All Probes must implement this Base functionality. | ||
| type BaseProbe interface { |
There was a problem hiding this comment.
When do we expect this interface to be satisfied but not the others?
This seems like an over generalization for a project designed to produce auto-instrumentation. Shouldn't every probe produce telemetry? When they don't why would they use this project?
| BaseProbe | ||
|
|
||
| // Run starts the Probe. | ||
| Run() |
There was a problem hiding this comment.
How does this produce telemetry? There is no destination for that data being communicated to the probe.
| default: | ||
| return nil, fmt.Errorf("unknown probe type") |
There was a problem hiding this comment.
It seems like this only supports GoLibraryTelemetryProbe. Why not just have that probe defined and add new ones when we support those?
| BaseProbe | ||
|
|
||
| // Run starts the Probe. | ||
| Run() |
There was a problem hiding this comment.
Do we want to pass context here?
Do we want to commit to the current approach that Run is blocking until canceled?
|
|
||
| // Attach attaches the eBPF programs to trigger points in the process. | ||
| // The specific attachment points are implemented by the Probe. | ||
| Attach() error |
There was a problem hiding this comment.
I think we need to support a use case of Loading the eBPF program once and attaching it to multiple locations in runtime.
For that reason, I think the Attach function should probably have an argument specifying where to attach to.
|
|
||
| // Manifest returns the Probe's instrumentation Manifest. This includes all | ||
| // the information about any packages the Probe instruments. | ||
| Manifest() Manifest |
There was a problem hiding this comment.
I was thinking about adding version support to the Manfiest (#1105 (comment))
Do you think that it will fit here or should the Manfiest be a part of the more basic interfaces?
| } | ||
|
|
||
| // TargetExecutableProbe is a provided implementation of GoLibraryTelemetryProbe. | ||
| type TargetExecutableProbe[BPFObj any] struct { |
There was a problem hiding this comment.
I wonder if we need the generic type here - as it seems we don't make use of it?
|
I agree with most of @MrAlias points, except the multi-process one which I addressed in #1219 (comment) |
Ref #1105
To support custom (modular) Probes, we want a Probe API that is:
All in all, this came out less heavy than I expected. But there is still a lot of nuance here, especially around making sure the interface design truly is flexible enough for us to expand in the future. As always, I'm open to any feedback on this.
Summary
In this design, I am proposing modifying our Probe API to rely on a layered composition of interfaces to define Probes. Today, this gives the appearance of simple inheritance. However, as more diverse functionality support is added in the future, I believe this will show more truly as a composition of base features.
To enable this composition, I am proposing adding extension points into our existing framework, gated by interface checks. Again, this is not very compelling with our single use case today but sets us up for adding new extension points in the future.
Current Design Problems
Our current, single
Probeinterface is designed with the assumption that all Probes will be Loaded, Run, and produce a Go Library Manifest.Note that this does not include any indication that our current Probes will produce any telemetry. It also means that any new Probes that aren’t based on a Go program, or a specific target, or that perhaps don’t need to be triggered to Run continuously must implement superfluous functions.
Instead, we have achieved polymorphism through structs:
Base,SpanProducer, andTraceProducer. While this has worked well for our different needs specific to Go library trace-producing Probes, these structs are still bound to a single interface definition which limits our ability to add new functionality and Probe types as time goes on.Proposal
Interfaces
To start, I broke down the functionality of our current Probes into layers. Each Probe we have today:
From these points I am proposing the initial interfaces:
BaseProbe- Must provide ID, Logging, Loading/Attaching, and Config functions. This are the base requirements for all Probes.RunnableProbe- Implements Run and Close functions.TracingProbe- Accepts Tracing config, such as Sampling.GoLibraryTelemetryProbe- Provides a Go Manifest and Target Executable config. This is what our current Probes are, but note that this only embeds aRunnableProbeand not aTracingProbeto allow us to implement future telemetry interfaces (such asMetricsProbe).My goal with this is to provide different “branching-off” points for new types of Probes in the future. For example, we may have a
RunnableProbethat relies on kprobes or network events.The list of interfaces we define will be baked into the framework and outline the functionality that is available in the framework. End-user developers will not be able to define their own interfaces, as that wouldn’t make sense (because they can’t customize the framework itself). However, users will be able to create their own implementations.
Default implementations
For each of these new Probe interfaces, I’m providing a default implementation:
BasicProbe- Simple (partial) implementation ofBaseProbe, but doesn’t have Load or Attach functionality.TargetExecutableProbe- Implementation ofGoLibraryTelemetryProbethat provides Load, Attach, Target Config, and Manifest functions.TargetEventProducingProbe- Generic struct that provides aread()helper for our 2 types of telemetry Probes.TargetSpanProducingProbeandTargetTraceProducingProbe- BothRunnableProbes that are the equivalent of our current Probe offerings.Refactors
The main refactor in this was the addition of several interface checks in the Manager: for example, we only call
Run()on aRunnableProbe, and we only check for the GoManifest()on aGoLibraryTelemetryProbe.As part of accepting the new interfaces, some helpers were consolidated (for example, now
FilterUnusedProbesisFilterUnusedProbesForTarget, with target info passed to the relevant Probes in order to simplify functions likeLoad()).On that note,
Load()has been simplified for the purpose of abstractingBasicProbe. The target executable information and Sampling config are now provided by the manager, if appropriate for the type of Probe.New features
As suggested in #1105 (comment),
Load()has been split intoLoad()andAttach(). This function also no longer relies on the Sampling config, allowing it to be used for non-tracing Probes in the future. (Instead, Sampling config should be provided by callingprobe.TracingConfig().SamplingConfigon aTracingProbe).All Probes are also now expected to implement a custom
ApplyConfig()function, which accepts a genericinterface{}as an argument. The Probe can then attempt to convert theinterface{}to its own unique config format for Probe-specific settings (see an example in thenet/httpprobe).