Skip to content

Static validation of improper connector usage in config files during dry-run #12681

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Static validation of improper connector usage in config files during dry-run #12681

wants to merge 5 commits into from

Conversation

sudiptob2
Copy link
Contributor

Description

As described in #8721, the current validate command cannot capture improper connector usage errors during a dry run.

One possible solution (as mentioned in this comment) is to surface configuration errors by instantiating the service during a dry run. However, reviewers indicated that statically verifying the errors is preferable to instantiating the service (see the PR
#12488 comments).

Currently, the validation logic for connectors is embedded in the service/internal/graph package. This PR extracts the relevant validation logic and uses it during dry-run to statically validate connector configurations.

Link to tracking issue

Fixes #8721

@codecov Codecov
Copy link

codecov bot commented Mar 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.55%. Comparing base (6e64a27) to head (415cbd4).
Report is 82 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #12681      +/-   ##
==========================================
- Coverage   91.55%   91.55%   -0.01%     
==========================================
  Files         481      481              
  Lines       26391    26420      +29     
==========================================
+ Hits        24162    24188      +26     
- Misses       1766     1768       +2     
- Partials      463      464       +1

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sudiptob2
Static validation of improper connector usage in config files during …
80e3883 
…dry runs

Signed-off-by: sudipto baral <[email protected]>
@sudiptob2
Add changelog.
a758c2a 
Signed-off-by: sudipto baral <[email protected]>
@sudiptob2
Add unit test
2057c75 
Signed-off-by: sudipto baral <[email protected]>
@sudiptob2
Add unit test for ValidateConnectors
bf9fe79 
Signed-off-by: sudipto baral <[email protected]>
@sudiptob2 sudiptob2 force-pushed the fix/connector-static-validation branch from 18bab32 to bf9fe79 Compare March 20, 2025 16:31
@sudiptob2 sudiptob2 marked this pull request as ready for review March 20, 2025 16:55
@sudiptob2 sudiptob2 requested a review from a team as a code owner March 20, 2025 16:55
@sudiptob2 sudiptob2 requested a review from songy23 March 20, 2025 16:55
@sudiptob2 sudiptob2 changed the title Static validation of improper connector usage in config files during … Static validation of improper connector usage in config files during dry-run Mar 20, 2025
songy23
songy23 approved these changes Mar 20, 2025
View reviewed changes
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

minor comments, otherwise LGTM

otelcol/collector_test.go
Factories: nopFactories,
ConfigProviderSettings: newDefaultConfigProviderSettings(t, []string{filepath.Join("testdata", "otelcol-invalid-connector-use.yaml")}),
},
expectedErr: `connector "nop/connector1" used as exporter in [logs/in2] pipeline but not used in any supported receiver pipeline`,
Copy link
Member

Choose a reason for hiding this comment

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

could you add another case on connector being used in receivers but not in exporters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added new tests

service/service_test.go Outdated

err := ValidateConnectors(connectorCfg, pipelinesCfg, connectorsFactories)
require.Error(t, err)
assert.Equal(t, "connector \"nop/connector1\" used as exporter in [logs/in2] pipeline but not used in any supported receiver pipeline", err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

same here

@songy23 songy23 requested review from djaglowski and mx-psi March 20, 2025 17:35
@sudiptob2
Add more testcases.
415cbd4 
Signed-off-by: sudipto baral <[email protected]>
djaglowski
djaglowski suggested changes Mar 22, 2025
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Can't we just instantiate the graph as usual and check for an error?

@djaglowski
Copy link
Member

djaglowski commented Mar 22, 2025 •
edited
Loading

In my opinion adding special code paths for validation unnecessarily complicates an already complex package. Instantiating the graph should be a low cost and repeatable operation which returns verbose errors if the graph is invalid.

@sudiptob2
Copy link
Contributor Author

@djaglowski

Can't we just instantiate the graph as usual and check for an error?

Yes, I agree, If we instantiate the graph we will also be able to surface cycle errors which is not possible to capture in this approach.
Please take a look at PR #12488 where I instantiated the service to surface these errors during dry run without touching the graph package.
However, reviewers indicated that statically verifying the errors is preferable to instantiating the service. Le me know which approach we should move forward with.

@sudiptob2 sudiptob2 mentioned this pull request Mar 22, 2025
Closed
@djaglowski
Copy link
Member

I am suggesting that we instantiate the graph only, not the service.

@sudiptob2 sudiptob2 mentioned this pull request Mar 24, 2025
Merged
@sudiptob2
Copy link
Contributor Author

Hi @djaglowski, could you please take a look at PR #12712, this PR uses the graph.Build() method to surface configuration errors during the dry run.

github-merge-queue bot pushed a commit that referenced this pull request Apr 9, 2025
@sudiptob2
Statically validate connector config during dry-run. (#12712)
6b9d125 
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
As described in
[#8721](#8721),
the current `validate` command cannot capture improper connector usage
errors during a dry run.

One proposed solution _(as mentioned in [this
comment](#8721 (comment)
is to surface configuration errors by instantiating the service during
the dry run. However, reviewers preferred statically verifying the
errors over instantiating the service (see the [PR #12488
comments](#12488)).

The validation logic for connectors is located in the
[`service/internal/graph`](https://github.com/open-telemetry/opentelemetry-collector/blob/1daa315455d02bac90185027878d858ba08a0f07/service/internal/graph)
package. In the discussion of [PR
#12681](#12681),
it was concluded that instantiating the graph is the better approach
(see [this
comment](#12681 (comment))).

Finally, this PR uses the `graph.Build()` method to surface
configuration errors during the dry run.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes #8721 #12535

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: sudipto baral <[email protected]>
@bogdandrutu
Copy link
Member

As @djaglowski suggested, let's use #12488 I think I was not sure about how much we start, but we only create the components, not start them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djaglowski djaglowski djaglowski requested changes

@songy23 songy23 songy23 approved these changes

@mx-psi mx-psi Awaiting requested review from mx-psi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate Connector Usage
4 participants
@sudiptob2 @djaglowski @bogdandrutu @songy23