shift process management from Probe to Manager #2029

damemi · 2025-03-20T18:38:39Z

Part of setting up for exposing the Probe API -- the main goal of this PR is to shift usage of the internal process package 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

MrAlias

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.

internal/pkg/instrumentation/probe/probe.go

MrAlias · 2025-03-25T16:17:19Z

internal/pkg/instrumentation/probe/probe.go

+	// GetUprobes returns a list of *Uprobes for the Probe.
+	GetUprobes() []*Uprobe
+
+	// GetConsts returns a list of Consts for the Probe.
+	GetConsts() []Const


These are duplicates of the Manifest method which contains these values.

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?

We should definitely unify these types respectively.

Given they are static to a probe, keeping their definitions in the Manifest seems appropriate.

@MrAlias looking more at the Manifest, I think this change in particular makes it redundant and everything it provides can be in the Probe itself. Given that we are defining the Probe as a more static representation now, I pushed an update to remove the Manifest type entirely and just use the references from it directly in the Probe.

Let me know if there's some other reason for Manifest I'm missing

internal/pkg/instrumentation/probe/probe.go

damemi · 2025-04-03T18:48:26Z

@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

damemi · 2025-04-03T18:53:35Z

@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?

MrAlias · 2025-04-08T16:06:29Z

just a static representation of the Probe's defintion?

My intent was for it to be static.

MrAlias

This doesn't looks to be a working example. I'm not sure how far I can review because of this.

internal/pkg/instrumentation/probe/probe.go

MrAlias · 2025-04-17T20:01:39Z

internal/pkg/instrumentation/probe/probe.go

+	// GetUprobes returns a list of *Uprobes for the Probe.
+	GetUprobes() []*Uprobe
+
+	// GetConsts returns a list of Consts for the Probe.
+	GetConsts() []Const


We should definitely unify these types respectively.

Given they are static to a probe, keeping their definitions in the Manifest seems appropriate.

internal/pkg/instrumentation/manager.go

MrAlias · 2025-04-17T20:05:43Z

internal/pkg/instrumentation/manager.go

+// 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 {


This can be unexported. It is only used internal to this package.

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).

replied above that this should have been set on a pointer, sorry about that confusion. But I'm curious what you mean by the closers field being a wrapper around a Probe. If I understand you correctly, that sounds like what I'm basically trying to do here, but could you explain what you have in mind? If it simplifies this then awesome

internal/pkg/instrumentation/manager.go

MrAlias · 2025-04-17T20:12:10Z

internal/pkg/instrumentation/manager.go

+	if err != nil {
+		return errors.Join(err, m.cleanup())
+	}
+	i.collection = c


This is an unused write to i.collection

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

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.

this was a mistake on my part, it should be a pointer reference so we can access it when closing as Ron pointed out

We should avoid programming side effects. Instead, we should use standard Go programming practices and have any values we want to use from a function be returned.

Can you please elaborate what you mean?

internal/pkg/instrumentation/manager.go

damemi · 2025-05-02T19:54:39Z

Hi all, thanks for the feedback so far and for your patience on my end getting back to you.

I pushed some more updates, notably:

Unexported locally used types and methods in the manager
Made sure probeReference fields are set by reference (the new Go testing framework actually helped catch this, thanks @MrAlias for that work)
Collapsed the Manifest field into Probe. It seems like Manifest is mostly redundant at this point when we can get what that provided from the Probe itself (ID, uprobes, and struct fields). Downside is this adds methods to the Probe type, but I think they make sense (GetUprobes, GetConsts, GetID).

For the consts, I made a type that has a method to just get the StructField consts to preserve the current behavior of Manifest. This looks like it's only used in a test though, so if we don't need it then I'm find dropping it.

This now passes the tests, but I'm sure there's more we can do so please take another look. I squashed the commits down to give us a clean slate at this point. Thanks!

MrAlias · 2025-05-02T22:41:43Z

.gitignore

@@ -47,3 +47,5 @@ examples/kafka-go/producer/producer
 examples/kafka-go/consumer/consumer

 go.work*
+
+opentelemetry-helm-charts


This is not needed.

Suggested change

opentelemetry-helm-charts

This folder got created somehow, I think while I was running one of the generators. It shouldn't be committed, and it should be in our gitignore. I can open a separate PR for this change if you'd prefer

MrAlias · 2025-05-02T23:03:33Z

internal/pkg/instrumentation/probe/probe.go

 // Probe is the instrument used by instrumentation for a Go package to measure
 // and report on the state of that packages operation.
 type Probe interface {
-	// Manifest returns the Probe's instrumentation Manifest. This includes all
-	// the information about the package the Probe instruments.
-	Manifest() Manifest


Replacing this with individual mentions does not seem advantageous.

Methods cannot be added to an interface in a backwards compatible manner. Fields and methods can be added to a strict in a backwards compatible manner.

There is also no way for default behavior for an interface other than compile time failures. A struct can be designed to apply sane defaults are expressive errors.

I feel we should continue to hold static definitions about the probe in a manifest type of some sort instead of this flattening. Methods for the probe should be added for actions the probe makes and other components will can to initiate those actions.

damemi · 2025-05-03T12:20:00Z

It seems unnecessary to have a separate struct nested in the Probe definition whose entire purpose is just to be a carbon copy of existing fields in the Probe. We know that every Probe is going to need an ID, Uprobes, and Consts. So it’s reasonable to add those as basic interface getters on the API. The Base Probe implementation can provide the default behavior like it does here. I don’t see the point in a duplicate Manifest type that has to be instantiated on each Probe anyway.

…

On Fri, May 2, 2025 at 7:03 PM Tyler Yahn ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In internal/pkg/instrumentation/probe/probe.go <#2029 (comment)> : > // Probe is the instrument used by instrumentation for a Go package to measure // and report on the state of that packages operation. type Probe interface { - // Manifest returns the Probe's instrumentation Manifest. This includes all - // the information about the package the Probe instruments. - Manifest() Manifest Replacing this with individual mentions does not seem advantageous. Methods cannot be added to an interface in a backwards compatible manner. Fields and methods can be added to a strict in a backwards compatible manner. There is also no way for default behavior for an interface other than compile time failures. A struct can be designed to apply sane defaults are expressive errors. I feel we should continue to hold static definitions about the probe in a manifest type of some sort instead of this flattening. Methods for the probe should be added for actions the probe makes and other components will can to initiate those actions. — Reply to this email directly, view it on GitHub <#2029 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOA77NZ4HTNDIFEZOCIJ2324P2VVAVCNFSM6AAAAABZOGHD72VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMJTGAZDIMZVGY> . You are receiving this because you authored the thread.Message ID: <open-telemetry/opentelemetry-go-instrumentation/pull/2029/review/2813024356 @github.com>

damemi force-pushed the decouple-process-from-probe branch 2 times, most recently from 8d8aa32 to 6b65138 Compare March 20, 2025 18:48

MrAlias reviewed Mar 25, 2025

View reviewed changes

damemi force-pushed the decouple-process-from-probe branch from 4cd3c36 to 39d7f0b Compare April 3, 2025 18:44

damemi changed the title ~~(wip) shift process management from Probe to Manager~~ shift process management from Probe to Manager Apr 8, 2025

damemi marked this pull request as ready for review April 8, 2025 19:51

damemi requested a review from a team as a code owner April 8, 2025 19:51

damemi force-pushed the decouple-process-from-probe branch 6 times, most recently from e0d26fa to 71a7496 Compare April 10, 2025 20:56

MrAlias reviewed Apr 17, 2025

View reviewed changes

damemi force-pushed the decouple-process-from-probe branch 5 times, most recently from 3b5bcce to 2c0dcaa Compare May 2, 2025 19:30

Move process management from Probe to Manager, remove Manifest

d1d8c62

damemi force-pushed the decouple-process-from-probe branch from 2c0dcaa to d1d8c62 Compare May 2, 2025 19:43

MrAlias reviewed May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shift process management from Probe to Manager #2029

shift process management from Probe to Manager #2029

damemi commented Mar 20, 2025

MrAlias left a comment

MrAlias Mar 25, 2025

damemi Apr 3, 2025

MrAlias Apr 17, 2025

damemi May 2, 2025

damemi commented Apr 3, 2025

damemi commented Apr 3, 2025

MrAlias commented Apr 8, 2025

MrAlias left a comment

MrAlias Apr 17, 2025

MrAlias Apr 17, 2025

MrAlias Apr 17, 2025

damemi May 2, 2025

MrAlias Apr 17, 2025

RonFed Apr 19, 2025

MrAlias Apr 19, 2025

damemi May 2, 2025

MrAlias May 2, 2025

damemi May 5, 2025

damemi commented May 2, 2025

MrAlias May 2, 2025

damemi May 5, 2025

MrAlias May 2, 2025

damemi commented May 3, 2025 via email

shift process management from Probe to Manager #2029

Are you sure you want to change the base?

shift process management from Probe to Manager #2029

Conversation

damemi commented Mar 20, 2025

MrAlias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented Apr 3, 2025

damemi commented Apr 3, 2025

MrAlias commented Apr 8, 2025

MrAlias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented May 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damemi commented May 3, 2025 via email