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

Make component.Factory inherit internalinterface.InternalInterface #4338

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Nov 2, 2021

Description:

Make component.Factory embed internalinterface.InternalInterface, so that the component factories must be implemented using the helpers.

This also includes extensions (which are not part of the focus of #3921, since extensions don't have per-signal methods), but I think for consistency it's better to add them (moving the internalinterface dependency to individual interfaces wouldn't be a breaking change if we find this is too restrictive).

This is a follow up to #4175.

Link to tracking Issue: fixes #3921

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #4338 (b4be13e) into main (a81963f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4338   +/-   ##
=======================================
  Coverage   87.24%   87.24%           
=======================================
  Files         177      177           
  Lines       10586    10586           
=======================================
  Hits         9236     9236           
  Misses       1108     1108           
  Partials      242      242           
Impacted Files Coverage Δ
component/componenttest/nop_exporter.go 100.00% <ø> (ø)
component/componenttest/nop_extension.go 100.00% <ø> (ø)
component/componenttest/nop_receiver.go 100.00% <ø> (ø)
exporter/exporterhelper/factory.go 100.00% <ø> (ø)
extension/extensionhelper/factory.go 81.25% <ø> (ø)
receiver/receiverhelper/factory.go 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a81963f...b4be13e. Read the comment docs.

@mx-psi mx-psi force-pushed the mx-psi/exporter-factory-force-helper branch from c62c75c to 76b6c95 Compare November 2, 2021 13:52
@mx-psi mx-psi changed the title Make component.ExporterFactory inherit internalinterface.InternalInterface Make component.Factory inherit internalinterface.InternalInterface Nov 2, 2021
@mx-psi
Copy link
Member Author

mx-psi commented Nov 2, 2021

Did this in a single PR since I am adding the dependency directly to component.Factory, but we can split into different PRs (one per component type + a final one for moving the internalinterface dependency to component.Factory)

@mx-psi mx-psi marked this pull request as ready for review November 2, 2021 14:01
@mx-psi mx-psi requested review from a team and jpkrohling November 2, 2021 14:01
@bogdandrutu
Copy link
Member

@mx-psi before merging this, I need an update of the contrib to latest core, so breaking changes are not accumulating.

@mx-psi
Copy link
Member Author

mx-psi commented Nov 2, 2021

@bogdandrutu 👍 I was doing it but it looks like Alex beat me with open-telemetry/opentelemetry-collector-contrib#6105, so let's use that one :)

@bogdandrutu bogdandrutu merged commit 9631cea into open-telemetry:main Nov 2, 2021
@mx-psi mx-psi deleted the mx-psi/exporter-factory-force-helper branch November 2, 2021 19:14
mx-psi pushed a commit that referenced this pull request 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
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.

Allow Factories to support new signals
2 participants