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] Allow calling component.ValidateConfig at the top level #12102

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evan-bradley
Copy link
Contributor

@evan-bradley evan-bradley commented Jan 15, 2025

Description

Updates component.ValidateConfig to recurse through the entire config object by checking for ConfigValidator implementations of structs that are under interfaces. Right now ValidateConfig stops at things like component.Config which themselves can't implement component.ValidateConfig because callValidateIfPossible can only see the interface and not the concrete type before this change.

Link to tracking issue

Replaces #12058.

Works toward #11524.

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.60%. Comparing base (6757cc7) to head (c53dbe7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12102   +/-   ##
=======================================
  Coverage   91.60%   91.60%           
=======================================
  Files         461      461           
  Lines       24678    24678           
=======================================
  Hits        22607    22607           
  Misses       1689     1689           
  Partials      382      382           

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

Comment on lines 27 to 33
if err := cfg.Pipelines.Validate(); err != nil {
return fmt.Errorf("service::pipelines config validation failed: %w", err)
}

if err := cfg.Telemetry.Validate(); err != nil {
fmt.Printf("service::telemetry config validation failed: %v\n", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we want to lose these error messages?

Copy link
Contributor Author

@evan-bradley evan-bradley Jan 15, 2025

Choose a reason for hiding this comment

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

In my opinion we don't, they're essential for users knowing where an error occurred. I think this is motivation for moving ConfigValidator to confmap: we can use the mapstructure tags (or struct field names/map keys/slice indexes/etc.) to recreate these paths automatically.

Copy link
Member

Choose a reason for hiding this comment

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

If this PR is merged and ConfigValidator is not moved what is the resulting error message when otelcol does the validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will just be the direct message from the Validate function. So for not configuring any protocols for the OTLP receiver, the Collector will print:

collector server run finished with error: invalid configuration: must specify at least one protocol when using the OTLP receiver

We could still make sure this message is printed inside component, but I think it's undesirable and adds to the reasons why we should move ConfigValidator:

  1. Simply use the struct field names to create paths and ignore any mapstructure tags. The paths would still be "correct" but may have names that don't match what's in the YAML file.
  2. Use mapstructure anyway either implicitly or explicitly to do this. This won't have any negative impacts for most users since mapstructure used by confmap and confmap is pretty much universal in the Collector. However I think it's not ideal.

Copy link
Contributor Author

@evan-bradley evan-bradley Jan 15, 2025

Choose a reason for hiding this comment

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

If we like this, I want to issue the following PRs:

  1. This PR with just the ValidateConfig changes. otelcol and service will still manually call Validate and keep the config paths in the error messages.
  2. Introduce printing the path to ValidateConfig.
  3. Remove direct calls to Validate in the codebase.
  4. Move ConfigValidator to confmap.

Copy link
Member

Choose a reason for hiding this comment

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

I support this plan. I think it is important to print the full path to configuration options (like we are doing today or with a new mechanism)

Copy link
Member

Choose a reason for hiding this comment

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

Ya that sounds good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mx-psi @TylerHelmuth this is ready to review.

Follow-up is here: #12108.

@evan-bradley
Copy link
Contributor Author

I'm skipping the contrib tests for now, I'll fix any errors with an update-otel PR in contrib.

@evan-bradley evan-bradley marked this pull request as ready for review January 16, 2025 20:29
@evan-bradley evan-bradley requested a review from a team as a code owner January 16, 2025 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants