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

[component] Try adding PipelineID #10947

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Aug 22, 2024

Another investigation into #9429 based on this strategy.

If we like this approach I can add in the Must* functions to enforce pipeline name patterns. It also doesn't really matter where PipelineID or Signal live, but componentstatus depends on PipelineID so it can't live in service.

Breaking changes in:

  • componentstatus: component.ID -> component.PipelineID
  • componentprofiles: DataTypeProfiles -> SignalProfiles
  • exporter.exporterqueue.Settings.DataType: component.DataType -> component.Signal
  • service.pipelines.Config: component.ID -> component.PipelineID
  • exporter.exportertest.CheckConsumeContractParams: component.DataType -> component.Signal
  • connector.LogsRouterAndConsumer.Consumer: component.ID -> component.PipelineID
  • connector.LogsRouterAndConsumer.PipelineIDs: component.ID -> component.PipelineID
  • connector.NewLogsRouter: component.ID -> component.PipelineID
  • connector.MetricsRouterAndConsumer.Consumer: component.ID -> component.PipelineID
  • connector.MetricsRouterAndConsumer.PipelineIDs: component.ID -> component.PipelineID
  • connector.NewMetricsRouter: component.ID -> component.PipelineID
  • connector.TracesRouterAndConsumer.Consumer: component.ID -> component.PipelineID
  • connector.TracesRouterAndConsumer.PipelineIDs: component.ID -> component.PipelineID
  • connector.NewTracesRouter: component.ID -> component.PipelineID

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 74.81203% with 67 lines in your changes missing coverage. Please review.

Project coverage is 91.96%. Comparing base (6928951) to head (0d37132).
Report is 89 commits behind head on main.

Files with missing lines Patch % Lines
component/identifiable.go 0.00% 35 Missing ⚠️
component/config.go 0.00% 20 Missing ⚠️
connector/internal/factory.go 58.82% 7 Missing ⚠️
receiver/receivertest/nop_receiver.go 0.00% 4 Missing ⚠️
service/internal/graph/nodes.go 97.91% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10947      +/-   ##
==========================================
- Coverage   92.21%   91.96%   -0.26%     
==========================================
  Files         414      414              
  Lines       19802    19857      +55     
==========================================
  Hits        18261    18261              
- Misses       1168     1223      +55     
  Partials      373      373              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mx-psi pushed a commit that referenced this pull request Sep 20, 2024
#### 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
codeboten pushed a commit that referenced this pull request Sep 23, 2024
#### Description
Depends on
#11209

This PR is a non-breaking implementation of
#10947. It
adds a new module, `pipeline`, which houses a `pipeline.ID` and
`pipeline.Signal`. `pipeline.ID` is used to identify a pipeline within
the service. `pipeline.Signal` is 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](https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#target-audiences).
I view this refactor as a Nice-To-Have, not a requirement for Component
1.0.

<!-- Issue number if applicable -->
#### Link to tracking issue
Works towards
#9429
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this pull request Sep 24, 2024
…emetry#11204)

#### Description
Depends on
open-telemetry#11209

This PR is a non-breaking implementation of
open-telemetry#10947. It
adds a new module, `pipeline`, which houses a `pipeline.ID` and
`pipeline.Signal`. `pipeline.ID` is used to identify a pipeline within
the service. `pipeline.Signal` is 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](https://github.com/open-telemetry/opentelemetry-collector/blob/main/CONTRIBUTING.md#target-audiences).
I view this refactor as a Nice-To-Have, not a requirement for Component
1.0.

<!-- Issue number if applicable -->
#### Link to tracking issue
Works towards
open-telemetry#9429
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.

1 participant