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

Refactor pipelines builder, fix some issues #5512

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Jun 10, 2022

  • Unconfigured receivers are not identified, this was not a real problem in final binaries since the validation of the config catch this.
  • Allow configurations to contain "unused" receivers. Receivers that are configured but not used in any pipeline, this was possible already for exporters and processors.
  • Remove the enforcement/check that Receiver factories create the same instance for the same config.

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

@bogdandrutu bogdandrutu requested review from a team and codeboten June 10, 2022 23:27
@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #5512 (a88d448) into main (a88d448) will not change coverage.
The diff coverage is n/a.

❗ Current head a88d448 differs from pull request most recent head 886c68a. Consider uploading reports for the commit 886c68a to get more accurate results

@@           Coverage Diff           @@
##             main    #5512   +/-   ##
=======================================
  Coverage   90.94%   90.94%           
=======================================
  Files         191      191           
  Lines       11381    11381           
=======================================
  Hits        10350    10350           
  Misses        808      808           
  Partials      223      223           

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 a88d448...886c68a. Read the comment docs.

@bogdandrutu bogdandrutu marked this pull request as draft June 13, 2022 01:07
@bogdandrutu
Copy link
Member Author

@mx-psi converting for the moment to draft, since this PR is too large, will send smaller PRs tomorrow

@mx-psi
Copy link
Member

mx-psi commented Jun 13, 2022

👍 Ping me when the PRs are ready to review

@bogdandrutu bogdandrutu force-pushed the refactorbuilder branch 2 times, most recently from 560ca13 to 886c68a Compare June 14, 2022 17:03
@bogdandrutu bogdandrutu force-pushed the refactorbuilder branch 2 times, most recently from 94d0951 to 787b1bb Compare June 15, 2022 14:28
@bogdandrutu bogdandrutu changed the title Refactor pipelines builder Refactor pipelines builder, fix some issues Jun 20, 2022
@bogdandrutu bogdandrutu force-pushed the refactorbuilder branch 2 times, most recently from 949fa14 to 5a5cfc2 Compare June 20, 2022 14:58
@bogdandrutu bogdandrutu marked this pull request as ready for review June 20, 2022 14:58
@bogdandrutu
Copy link
Member Author

@mx-psi please review, now this PR is well documented and ready for the final review.

@bogdandrutu bogdandrutu force-pushed the refactorbuilder branch 2 times, most recently from e6b783c to f708e0e Compare June 20, 2022 15:08
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only a few nits for consistency

CHANGELOG.md Outdated Show resolved Hide resolved
service/internal/pipelines/pipelines.go Outdated Show resolved Hide resolved
service/internal/pipelines/pipelines.go Outdated Show resolved Hide resolved
service/internal/pipelines/pipelines.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu force-pushed the refactorbuilder branch 2 times, most recently from 2d87748 to 8c64dae Compare June 21, 2022 10:35
* Unconfigured receivers are not identified, this was not a real problem in final binaries since the validation of the config catch this.
* Allow configurations to contain "unused" receivers. Receivers that are configured but not used in any pipeline, this was possible already for exporters and processors.
* Remove the enforcement/check that Receiver factories create the same instance for the same config.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu bogdandrutu merged commit b05c0f3 into open-telemetry:main Jun 21, 2022
@bogdandrutu bogdandrutu deleted the refactorbuilder branch June 21, 2022 11:15
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.

3 participants