Skip to content

Support multiple process handling in otelsdk #2016

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

Merged
merged 6 commits into from
Apr 20, 2025

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 18, 2025

Resolve #2143

@MrAlias MrAlias mentioned this pull request Mar 18, 2025
@MrAlias MrAlias force-pushed the otelsdk-multiproc branch from a8cb945 to 723546c Compare March 18, 2025 21:35
@RonFed
Copy link
Contributor

RonFed commented Mar 21, 2025

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?

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 21, 2025

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?

@RonFed
Copy link
Contributor

RonFed commented Mar 21, 2025

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.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 21, 2025

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?

@RonFed
Copy link
Contributor

RonFed commented Mar 21, 2025

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.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 22, 2025

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
Copy link
Contributor Author

MrAlias commented Mar 22, 2025

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.

@RonFed
Copy link
Contributor

RonFed commented Mar 27, 2025

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

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 27, 2025

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 MrAlias force-pushed the otelsdk-multiproc branch 6 times, most recently from 80384ef to 21096f9 Compare April 16, 2025 20:46
@MrAlias MrAlias changed the title [PoC] Support multiple process handling in otelsdk Support multiple process handling in otelsdk Apr 16, 2025
@MrAlias MrAlias force-pushed the otelsdk-multiproc branch 2 times, most recently from 68628b5 to fafdd27 Compare April 16, 2025 20:52
@MrAlias MrAlias force-pushed the otelsdk-multiproc branch from fafdd27 to b59ed53 Compare April 16, 2025 20:54
@MrAlias MrAlias marked this pull request as ready for review April 16, 2025 20:54
@MrAlias MrAlias requested a review from a team as a code owner April 16, 2025 20:54
@MrAlias MrAlias added this to the v0.22.0 milestone Apr 17, 2025
@MrAlias MrAlias merged commit 336677d into open-telemetry:main Apr 20, 2025
21 checks passed
@MrAlias MrAlias deleted the otelsdk-multiproc branch April 20, 2025 14:30
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

Successfully merging this pull request may close these issues.

Support multiple process handling in otelsdk
2 participants