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

Manager API does not match singleton design #1219

Open
MrAlias opened this issue Oct 25, 2024 · 2 comments
Open

Manager API does not match singleton design #1219

MrAlias opened this issue Oct 25, 2024 · 2 comments

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Oct 25, 2024

The Manager is designed in such a way that it expects Run is only ever called once. However, it is a method on the Manager and can be called multiple times (i.e. multi-process instrumentation).

There seems a strong need to refactor this type. Ideally, the refactored code will have independent Run functionality or unify so there can only ever be one "Run".

@MrAlias MrAlias added the bug Something isn't working label Oct 25, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 25, 2024

Additionally, from #1218:

It would be best to adhere to the idiom: "the writer should close the chan". This would mean that a probe's Run func needs to return the chan it writes to and only closes it when it returns. This would also mean the Manager will need to dynamically multiplex chan selection.

@RonFed
Copy link
Contributor

RonFed commented Oct 26, 2024

We have this check when Instrumentation.Run is called:

if i.stop != nil {
return parent, errors.New("instrumentation already running")
}

So at least for the current implementation, it prevents from Run being called more than once (so maybe it should not be classified as a bug but as a feature request)

Regarding future multi-process support: do auto-instrumentations for other languages contain this kind of support or is it out of scope for them? (i.e detecting all the relevant processes for the language, filtering the processes the user wants, and managing the state for each process) - I guess I'm wondering whether it should be in the scope of this repo.

If the answer is yes, then maybe the Instrumentation object should exist per process - which means the manager will still be per process, and a higher level object will need to be created to manage all the Instrumentations and it will be responsible to handle stuff in the process level. The motivation for this is the manager already does quite a lot, and will probably have more jobs once we have support for more configuration options for each probe. Adding the multi-process handling to it can make it more complicated.

If the answer is no, we can keep the current structure and a higher-level implementation can make use of multiple Instrumentation objects to manage multi-process instrumentation (this is what Odigos does today).

@RonFed RonFed removed the bug Something isn't working label Oct 30, 2024
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

No branches or pull requests

2 participants