-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix connector validation based on usage in pipelines #8004
Fix connector validation based on usage in pipelines #8004
Conversation
Validation of connectors was too aggressive in that it failed if a connector was used in any combination of unsupported roles. Instead, it should pass validation as long as each use of the connector has a supported corresponding use. For example, the forward connector may forward traces and metrics at the same time. Previously, validation would fail because it detected that traces->metrics and metrics->traces were possible connections. Now it will pass as long as there is a supported connection type for each pipeline in which the connector is used.
Validation of connectors is currently split between
The validation added in this PR could arguably be added to either package, as it seems to fall somewhere in between, but having tried adding it to |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #8004 +/- ##
==========================================
- Coverage 90.90% 90.80% -0.10%
==========================================
Files 300 300
Lines 15074 15146 +72
==========================================
+ Hits 13703 13754 +51
- Misses 1097 1110 +13
- Partials 274 282 +8
☔ View full report in Codecov by Sentry. |
357d112
to
f735a61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one last thought that we don't need to address here:
Validation of connectors is currently split between otelcol and service/internal/graph packages
I am wondering if everything should be on service/internal/graph
; if we are to eventually support #4970/#6888 it seems like the best solution is to expose the graph package, in which case all validation should happen there.
We don't need to solve this on this PR necessarily, since moving the validation around is not a breaking change.
@mx-psi, thanks for the reviews. w/r/t where validation is located, I think this PR effectively consolidates it into the |
alright, sounds like it's something we should solve independently then :) |
Alternate to #8003
Fixes ##7892
Validation of connectors was too aggressive such that a connector that was used in any combination of unsupported roles would fail. Instead, validation should pass as long as each use of the connector has a supported corresponding use.
For example, the forward connector may forward traces and metrics at the same time. Previously, validation would fail because it detected that traces->metrics and metrics->traces were possible connections. Now it will pass as long as there is a supported connection type for each pipeline in which the connector is used.