[component] Refactor to use pipeline.ID and pipeline.Signal #11204
Conversation
TylerHelmuth
force-pushed
the
component-refactor-datatype
branch
5 times, most recently
from
fe90c10
to
1225ae3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11204 +/- ##
==========================================
- Coverage 91.80% 91.41% -0.39%
==========================================
Files 424 424
Lines 20094 20198 +104
==========================================
+ Hits 18447 18465 +18
- Misses 1268 1349 +81
- Partials 379 384 +5 ☔ View full report in Codecov by Sentry. |
TylerHelmuth
force-pushed
the
component-refactor-datatype
branch
from
1225ae3
to
3a776d6
Compare
#### Description To facilitate the work in #11204 as some breaking changes and some deprecations, this PR adds the new pipeline module separately so that future PRs can handle the breaking changes and deprecations. In order to make `Signal` uninstantiable outside of this repo, while still being extendable in places like `componentprofiles`, a new internal module is added to handle the `Signal` logic. To reduce the dependency sprawl that would happen if `signal` was an internal package in `go.opentelemetry.io/collector`, I made it a module, similar to `globalgates`. <!-- Issue number if applicable --> #### Link to tracking issue Related to #10947 <!--Describe what testing was performed and which tests were added.--> #### Testing Added unit tests <!--Describe the documentation added.--> #### Documentation Added godoc comments
TylerHelmuth
added
the
release:blocker
| @@ -153,5 +154,41 @@ func (cfg *Config) Validate() error { | |||
| return fmt.Errorf("service::pipelines::%s: references exporter %q which is not configured", pipelineID, ref) | |||
| } | |||
| } | |||
|
|
|||
| // Check that all pipelines reference only configured components. | |||
| for pipelineID, pipeline := range cfg.Service.PipelinesWithPipelineID { | |||
There was a problem hiding this comment.
do we need a second for loop here?
There was a problem hiding this comment.
This is a duplicate of the for pipelineID, pipeline := range cfg.Service.Pipelines { loop above it.
I didn't want to handle the merging of cfg.Service.Pipelines into cfg.Service.PipelinesWithPipelineID via the Validate function (it is instead handled in service.initGraph) so I instead opted to validate both cfg.Service.Pipelines and cfg.Service.PipelinesWithPipelineID.
The line above, if err := cfg.Service.Validate(); err != nil, will validate that only one of the 2 is set, so in reality only one of the two for loops is ever entered.
Description
Depends on #11209
This PR is a non-breaking implementation of #10947. It adds a new module,
pipeline, which houses apipeline.IDandpipeline.Signal.pipeline.IDis used to identify a pipeline within the service.pipeline.Signalis uses to identify the signal associated to a pipeline.I do this work begrudgingly. As the PR shows, this is a huge refactor when done in a non-breaking way, will require 3 full releases, and doesn't benefit our End Users or, in my opinion, our Component Developers or Collector Library Users. I view this refactor as a Nice-To-Have, not a requirement for Component 1.0.
Link to tracking issue
Works towards #9429