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

Review component public API #6506

Closed
codeboten opened this issue Nov 8, 2022 · 9 comments
Closed

Review component public API #6506

codeboten opened this issue Nov 8, 2022 · 9 comments
Assignees
Labels
release:required-for-ga Must be resolved before GA release

Comments

@codeboten
Copy link
Contributor

Before considering component 1.0, we need to review the public API to ensure what is exposed makes sense https://pkg.go.dev/go.opentelemetry.io/collector/component

@codeboten
Copy link
Contributor Author

Some thoughts:

  • should BuildInfo be in this package? since its more directly tied to the service running, it might make more sense to move it to that package
  • TelemetrySettings should be moved into the telemetry package

@bogdandrutu
Copy link
Member

should BuildInfo be in this package? since its more directly tied to the service running, it might make more sense to move it to that package

I think cannot be moved to the "service" since you need to pass this to components, so will create a circular dependency.

TelemetrySettings should be moved into the telemetry package

Same issue about dependency.

@bogdandrutu
Copy link
Member

Before considering component 1.0, we need to review the public API to ensure what is exposed makes sense https://pkg.go.dev/go.opentelemetry.io/collector/component

@codeboten we also need to make sure that "consumer" package is 1.0 before :)

@atoulme atoulme added the release:required-for-ga Must be resolved before GA release label Dec 19, 2023
@mx-psi
Copy link
Member

mx-psi commented Dec 21, 2023

IMO BuildInfo does not make sense in service: the components should not know anything about the service other than through the component.Host interface; ideally we should be able to completely replace service with a new implementation without changing anything about the components.

For TelemetrySettings, I think it makes sense to move it to telemetry, but my proposal would be to make telemetry into a separate module outside of service. I want to work on this at some point, but haven't gotten around to it lately.

@mx-psi
Copy link
Member

mx-psi commented Feb 7, 2024

I am wondering if we should split component into further packages. For example, we could have componentstatus and componentfactory as separate packages. It currently feels like a mish-mash of important but unrelated things

codeboten pushed a commit that referenced this issue Aug 22, 2024
#### Description
Adds an RFC for component status reporting. The main goal is to define
what component status reporting is, our current, implementation, and how
such a system interacts with a 1.0 component. When merged, the following
issues will be unblocked:
- #9823
- #10058
- #9957
- #9324
- #6506

---------

Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@TylerHelmuth TylerHelmuth self-assigned this Aug 22, 2024
sfc-gh-sili pushed a commit to sfc-gh-sili/opentelemetry-collector that referenced this issue Aug 23, 2024
Adds an RFC for component status reporting. The main goal is to define
what component status reporting is, our current, implementation, and how
such a system interacts with a 1.0 component. When merged, the following
issues will be unblocked:
- open-telemetry#9823
- open-telemetry#10058
- open-telemetry#9957
- open-telemetry#9324
- open-telemetry#6506

---------

Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@TylerHelmuth
Copy link
Member

TylerHelmuth commented Aug 28, 2024

Did a review of Component's public API and here are my thoughts:

  1. We could rename/refactor DataType (see Does DataType belong to component? Can we make it a type itself? #9429). Let's keep discussion on that specific field in that issue.

  2. We could move component.Telemetry to its own package as component.Telemetry is not used anywhere without component. But I struggle to find end-user value in such a change at this point.

  3. We could move component.BuildInfo to its own package as component.BuildInfo is not used anywhere without component. But I struggle to find end-user value in such a change at this point.

  4. The component.Factory has a comment that claims:

    // This interface cannot be directly implemented. Implementations must
    // use the factory helpers for the appropriate component type.
    

    but the interface itself doesn't include anything that prevents something from implementing the interface. Do we need to make it sealed? We'd have to do a bit of refactoring to seal the interface to make the implementations in receiver/exporter/processor work.

mx-psi pushed a commit that referenced this issue Aug 30, 2024
#### Description
Removes an incorrect comment from the `component.Factory interface`.

This comment was added via
#4338 when
the extension/receiver/processor/exporter factory types all still [lived
in](https://github.com/mx-psi/opentelemetry-collector/blob/b4be13e74923ebefb521f69a97288ce8d6f51ea9/component/exporter.go#L61)
`component`. Since the factories have been moved to other modules,
`component.Factory` needs to be implementable.

The specific extension/receiver/processor/exporter factory types are
unimplementable since they are defined in internal packages.

<!-- Issue number if applicable -->
#### Link to tracking issue
Related to
#6506
@mx-psi
Copy link
Member

mx-psi commented Sep 10, 2024

For (2) and (3) I feel like, unless not doing it would delay component 1.0, we should keep them where they are.

I said before

For TelemetrySettings, I think it makes sense to move it to telemetry

If memory serves this was because I though different telemetry implementations could have different telemetry settings, but I don't think that should be the case: the interfaces would stay the same.

@mx-psi
Copy link
Member

mx-psi commented Sep 17, 2024

@TylerHelmuth Should we close this as completed since we have a different issue for (1)?

@TylerHelmuth
Copy link
Member

I agree, there is not appetite for 2. and 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga Must be resolved before GA release
Projects
Archived in project
Development

No branches or pull requests

5 participants