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

Extract service.pipelines interface #6764

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

djaglowski
Copy link
Member

@djaglowski djaglowski commented Dec 12, 2022

Subset of #6700

@djaglowski djaglowski force-pushed the component-nodes branch 2 times, most recently from dbc3603 to 7d2a1da Compare December 12, 2022 21:52
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Base: 90.67% // Head: 90.60% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (707364e) compared to base (51c9139).
Patch coverage: 8.33% of modified lines in pull request are covered.

❗ Current head 707364e differs from pull request most recent head ad38f1f. Consider uploading reports for the commit ad38f1f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6764      +/-   ##
==========================================
- Coverage   90.67%   90.60%   -0.07%     
==========================================
  Files         241      242       +1     
  Lines       14525    14536      +11     
==========================================
  Hits        13171    13171              
- Misses       1087     1098      +11     
  Partials      267      267              
Impacted Files Coverage Δ
service/graph.go 0.00% <0.00%> (ø)
service/host.go 89.47% <ø> (ø)
service/pipelines.go 93.93% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@djaglowski djaglowski marked this pull request as ready for review December 13, 2022 02:13
@djaglowski djaglowski requested review from a team and Aneurysm9 December 13, 2022 02:13
@djaglowski
Copy link
Member Author

cc @bogdandrutu

@djaglowski
Copy link
Member Author

This PR establishes the "fork" between the current way of managing pipelines and the way of doing it via a graph that includes connectors. At the appropriate time, we can use a feature gate to toggle between the two implementations.

.chloggen/component-nodes.yaml Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
service/graph.go Outdated Show resolved Hide resolved
service/graph.go Outdated Show resolved Hide resolved
service/graph.go Outdated Show resolved Hide resolved
service/pipelines_test.go Outdated Show resolved Hide resolved
service/pipelines_test.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

Need rebase.

@djaglowski
Copy link
Member Author

I've rebased, but this basically required adding connector.Builder, which I've now spun off into #6867. Also added tests for other component builders in #6862.

@gbbr
Copy link
Member

gbbr commented Jan 3, 2023

Hi Daniel & Bogdan! It would be very nice if we could add descriptions to these PRs

@djaglowski djaglowski force-pushed the component-nodes branch 2 times, most recently from 08ebaec to 98d41dd Compare January 4, 2023 16:35
@djaglowski
Copy link
Member Author

This is rebased onto #6867 and ready to go otherwise.

@djaglowski djaglowski force-pushed the component-nodes branch 2 times, most recently from 4ea69db to 17c560f Compare January 9, 2023 22:13
@djaglowski djaglowski added Skip Changelog PRs that do not require a CHANGELOG.md entry area:connector labels Jan 9, 2023
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Looks promising, but what's up with the tests? It would be nice to have a few tests demonstrating the graph. For instance, in the look from the last to first after sorting the graph, I assume the exporters are all located towards the end of the slice, with processors next, and receivers first. This is something that could be exemplified (and proven) in the tests.

@djaglowski
Copy link
Member Author

Yeah, test coverage is poor on this PR. I think things are covered quite thoroughly on #6700, but some lower level tests would be better here. For now, what do you think about me removing the implementations of Start and Shutdown in this PR and then adding tests alongside the implementations later?

@djaglowski djaglowski force-pushed the component-nodes branch 2 times, most recently from 4274247 to 8c4fd69 Compare January 17, 2023 18:11
@djaglowski
Copy link
Member Author

I've removed the implementations entirely, so now this is just adding the interface and a stub for the graph-based implementation. @open-telemetry/collector-approvers, please take a look.

service/graph.go Outdated Show resolved Hide resolved
@djaglowski djaglowski force-pushed the component-nodes branch 3 times, most recently from 0dd840e to 8d24226 Compare January 23, 2023 15:15
@djaglowski
Copy link
Member Author

@open-telemetry/collector-maintainers, I believe this is ready to merge.

@dmitryax dmitryax merged commit dc3071c into open-telemetry:main Jan 23, 2023
@djaglowski djaglowski deleted the component-nodes branch January 23, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:connector Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants