opentelemetry-go-instrumentation icon indicating copy to clipboard operation
opentelemetry-go-instrumentation copied to clipboard

[PoC] Support multiple process handling in otelsdk

Open MrAlias opened this issue 1 year ago • 7 comments

MrAlias avatar Mar 18 '25 21:03 MrAlias

I was thinking we could have something similar to what we have for WithScope and to have WithResource - that way a caller can have one base handler and generate sub-handlers for each process that would just add the process specific attributes.

  1. I didn't fully understand the comment about making this generic or not - I understand that otelsdk will manage resources differently than a collector handler but figured it would be useful for users to have a generic way to add resources to a handler regardless of the base handler.
  2. The multiplexer implementation seems to add pre-defined attributes for a PID - do you think we could allow the users pass their own attributes?

RonFed avatar Mar 21 '25 16:03 RonFed

How do you add attributes to a Handler if you don't know the type attributes are expressed in by the Handler? Same question for a Resource?

Also, why overload the Instrumentation with responsibility for resources? It is not designed to discover processes or provide information about the system it is running on. Why not encapsulate that responsibility where it makes sense (i.e. the Handler implementations)? What is the motivation to split this responsibility?

MrAlias avatar Mar 21 '25 16:03 MrAlias

I'm not suggesting to have it in the Instrumentation. I'm suggesting a way to provide users the ability to wrap any Handler with resource. Same as we have for scope - we assumed the scope is represented the way it is in pdata/pcommon (since this is the type in the Handler struct) - why we can make this assumption about the resource? My second point was - that if we allow this, it makes sense to allow the caller to provide custom resources as well - since they have more context about the process.

RonFed avatar Mar 21 '25 16:03 RonFed

Users don't call WithScope, Instrumentation does.

Why would a user call WithResource on a Handler if they can just wrap their implementation with the resource they want directly and avoid any conversion?

MrAlias avatar Mar 21 '25 22:03 MrAlias

Users don't call WithScope, Instrumentation does.

Then maybe the Instrumentation should call WithResource as well, since it is a per process object.

Why would a user call WithResource on a Handler if they can just wrap their implementation with the resource they want directly and avoid any conversion?

As a user, this feels not intuitive ant not necessary. I want to have a common handler which will handle the exporting and batching, and also have the ability to pass resource to each instrumentation/process/handler wrapper. I don't want to be aware of the internals of the Handler, and don't want to depend on those internals. Since today we support passing resources to an instrumentation (which is a process level object) - I think we should keep allowing this in a reasonable way.

RonFed avatar Mar 21 '25 22:03 RonFed

Then maybe the Instrumentation should call WithResource as well, since it is a per process object.

Right, so this is what I've been talking about in my comments here and #1859. I ask you please refer to them with this new understanding. To summarize, this is a bad idea as it separates the duty of maintaining a resource, it is not universal, and it imposes a resource/attribute representation burden on all Handlers.

MrAlias avatar Mar 22 '25 14:03 MrAlias

Why would a user call WithResource on a Handler if they can just wrap their implementation with the resource they want directly and avoid any conversion?

As a user, this feels not intuitive ant not necessary. I want to have a common handler which will handle the exporting and batching, and also have the ability to pass resource to each instrumentation/process/handler wrapper. I don't want to be aware of the internals of the Handler, and don't want to depend on those internals. Since today we support passing resources to an instrumentation (which is a process level object) - I think we should keep allowing this in a reasonable way.

I don't follow.

  1. How is this:

    handler, _ := otelsdk.NewHandler()
    handler = handler.WithResource(pcommon.New(/* pdata resource */))
    

    Simpler and more intuitive to a user then this?

    handler, _ := otelsdk.NewHandler(WithResourceAttributes(/* OTel Go resource attrs */))
    

    Especially if I need to import a pcommon representation of a resource when I want to use the OTel SDK?

  2. How do you plan to support resource data that cannot be expressed using a pcommon.Resource but a user users in their Handler implementation (e.g. []complex64, struct{/* ... */}, MyCustomType)?

  3. The configuration of a Resource becomes:

    • Accept user attributes on Handler implementation creation (i.e. otelsdk.NewHandler)
    • Compute a resource for the handler implementation and wrap it in a Handler
    • Send additional resource information to the Handler in the form of pcommon.Resource
    • Translate the pcommon.Resource data to the handler implementation format
    • Merge data into new resource, possibly dropping unsupported data
    • Return a copy of the Handler

    This is way more complex, and confusing, than just:

    • Accept user attributes on Handler implementation creation (i.e. otelsdk.NewHandler)
    • Compute a resource for the handler implementation and wrap it in a Handler

    Won't this be more confusing for users to use and Handler implementation authors to implement?

  4. What happens when users start asking for a resource to be computed in the hot-path of the handler? This seems like something we want to prevent by design, not allow.

  5. Why switch away from the information flow model we already have? The Controller is responsible for managing the resource. It accepts additional resource attributes from the users when it is created. It does not get resource information from Instrumentation. Why do we now want to separate this responsibility and have part of a resource come from Instrumentation?

  6. When are you going to use more than one Handler implementation? It seems like you are trying to achieve an overly generic solution for a situation that doesn't exist. Users will only use one Handler at a time. Even if they want to send data to multiple endpoints or use different formats that will all be encapsulated in a single Handler.

MrAlias avatar Mar 22 '25 15:03 MrAlias

@MrAlias Following the SIG meeting and reading through the multiplexer example, i think we can move forward with this approach. I have one question: from the current example here we pass a PID to the multiplexer and it adds pre-defined resource attributes. Can we add support for the multiplexer to receive custom attributes for a process?

RonFed avatar Mar 27 '25 12:03 RonFed

I have one question: from the current example here we pass a PID to the multiplexer and it adds pre-defined resource attributes. Can we add support for the multiplexer to receive custom attributes for a process?

Sure, that is something we can do if we have a use-case for it.

MrAlias avatar Mar 27 '25 15:03 MrAlias