shift process management from Probe to Manager #2029
Conversation
damemi
force-pushed
the
decouple-process-from-probe
branch
2 times, most recently
from
8d8aa32 to
6b65138
Compare
There was a problem hiding this comment.
I like the idea of decoupling the probe from process management, but it seems like this is just shuffling existing patterns out of a probe without refactoring the details deep in the functionality. I think we need to drill deeper into what the process interactions are for a probe and likely redesign around that.
| // GetLogger returns the *slog.Logger associated with the Probe. | ||
| GetLogger() *slog.Logger |
There was a problem hiding this comment.
Why would we ever need to get the logger from a probe? Doesn't the Manager have its own logger it can use for its own operations?
There was a problem hiding this comment.
This sill needs to be addressed. The Manager needs to use it's own logger. This should be removed.
| // GetLogger returns the *slog.Logger associated with the Probe. | |
| GetLogger() *slog.Logger |
| // GetUprobes returns a list of *Uprobes for the Probe. | ||
| GetUprobes() []*Uprobe | ||
|
|
||
| // GetConsts returns a list of Consts for the Probe. | ||
| GetConsts() []Const |
There was a problem hiding this comment.
These are duplicates of the Manifest method which contains these values.
There was a problem hiding this comment.
True, and it wasn't clear to me because we change the name of them in the Manifest: Uprobes->FunctionSymbol and Consts->StructFieldIDs
The latter I think is because the Manifest doesn't actually store all the Consts in the Probe, just the consts for fields. If we're loading the uprobes in the manager now and injecting the consts there, we'll need all of the consts.
Do you think we should refactor the Manifest struct a bit to align better with the Probe definition?
There was a problem hiding this comment.
We should definitely unify these types respectively.
Given they are static to a probe, keeping their definitions in the Manifest seems appropriate.
damemi
force-pushed
the
decouple-process-from-probe
branch
from
4cd3c36 to
39d7f0b
Compare
|
@MrAlias I did some more refactoring based on the last sig call. Basically, the Manager will now track a reference to the "initialized" probe, ie the probe+its collection+its closers. Then all collection/closer interaction can be done in the manager. It's just a step, but let me know what you think |
|
@MrAlias and now that I think about it, the collection/closer stuff could be set in the Manifest, but is the manifest intended to be mutable like that (ie, updated once the Probe is initialized) or just a static representation of the Probe's defintion? |
My intent was for it to be static. |
damemi
changed the title
damemi
force-pushed
the
decouple-process-from-probe
branch
2 times, most recently
from
613d429 to
4987d3a
Compare
damemi
force-pushed
the
decouple-process-from-probe
branch
3 times, most recently
from
a923943 to
e0d26fa
Compare
damemi
force-pushed
the
decouple-process-from-probe
branch
from
e0d26fa to
71a7496
Compare
| // GetLogger returns the *slog.Logger associated with the Probe. | ||
| GetLogger() *slog.Logger |
There was a problem hiding this comment.
This sill needs to be addressed. The Manager needs to use it's own logger. This should be removed.
| // GetLogger returns the *slog.Logger associated with the Probe. | |
| GetLogger() *slog.Logger |
| // GetUprobes returns a list of *Uprobes for the Probe. | ||
| GetUprobes() []*Uprobe | ||
|
|
||
| // GetConsts returns a list of Consts for the Probe. | ||
| GetConsts() []Const |
There was a problem hiding this comment.
We should definitely unify these types respectively.
Given they are static to a probe, keeping their definitions in the Manifest seems appropriate.
| return nil | ||
| } | ||
|
|
||
| func (m *Manager) loadUprobe(u *probe.Uprobe, c *ebpf.Collection) error { |
There was a problem hiding this comment.
It looks like this is no longer used and a can be removed.
| return inject.Constants(spec, opts...) | ||
| } | ||
|
|
||
| func (m *Manager) loadUprobesFromProbe(i ProbeReference) error { |
There was a problem hiding this comment.
Similar here, this can be removed.
| return errors.Join(err, m.cleanup()) | ||
| } | ||
|
|
||
| c, err := utils.InitializeEBPFCollection(spec, m.collectionOpts) |
There was a problem hiding this comment.
Should this function be moved to this package/file. This is the only place it is used.
| // ProbeReference is used by the Manager to track an initialized reference | ||
| // to a Probe and its related resources such as its ebpf.Collection and io.Closers. | ||
| type ProbeReference struct { |
There was a problem hiding this comment.
This can be unexported. It is only used internal to this package.
There was a problem hiding this comment.
It's also not clear why this is needed. The collection field is never used (it is ineffectively set, but never read), and the closers field could just be captured in a thin wrapper around a Probe implementation (given it is already an interface).
| @@ -54,6 +58,15 @@ type Manager struct { | |||
| probeMu sync.Mutex | |||
| state managerState | |||
| stateMu sync.RWMutex | |||
| collectionOpts *ebpf.CollectionOptions | |||
There was a problem hiding this comment.
This doesn't seem to have a need as a field. It is only ever used in a single call site and always set to the same value. It should just be a local var to https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/2029/files#diff-8008933194d32417cc135227b15916c4d1f575de0abda31bc0545b51d9783bebR435
| m.probes[id] = p | ||
| m.probes[id] = ProbeReference{ | ||
| probe: p, | ||
| closers: make([]io.Closer, 0), |
There was a problem hiding this comment.
This is not needed. It is only allocating memory prior to need. append handles appends to empty values without issue.
| closers: make([]io.Closer, 0), |
| if err != nil { | ||
| return errors.Join(err, m.cleanup()) | ||
| } | ||
| i.collection = c |
There was a problem hiding this comment.
This is an unused write to i.collection
There was a problem hiding this comment.
It looks like we are using it in the Close function,
the collection allows us to maybe make the closing mechanism simpler?
see
https://github.com/cilium/ebpf/blob/49a06b1fe26190a4c2a702932f611c6eff908d3a/collection.go#L904
There was a problem hiding this comment.
Oh, I gotcha, just this is writing to a value, not a reference. When this function returns the passed argument remains unchanged. This assignment is to a copied value that is not used.
| @@ -398,11 +419,189 @@ func (m *Manager) loadProbes() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (m *Manager) LoadProbe(i ProbeReference, name probe.ID, cfg Config) error { | |||
There was a problem hiding this comment.
LoadProbe is only ever called internal to the package. This should be unexported.
Part of setting up for exposing the Probe API -- the main goal of this PR is to shift usage of the internal
processpackage from the base Probe implementation to the Manager.#1943 moved process analysis to the manager, so this follows a similar pattern of consolidating process handling into the manager.
Functionally, that means moving a lot of the Probe initialization to the manager. So now, instead of thinking "the Probe sets itself up" the flow is more "the Manager sets up the Probe"
This is a WIP right now showing the basic idea, the tougher part will be decoupling Process info from Consts