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] Print path to error in ValidateConfig #12108

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

evan-bradley
Copy link
Contributor

Description

Follow up to #12102.

This prints the full path to where the error occurred in a config object using available reflection metadata. Currently some paths will be duplicated until we shift otelcol to call component.ValidateConfig on otelcol.Config and remove manual calls to Validate, but no information will be missing. If this is a concern we can do both steps at once.

Example error messages:

collector server run finished with error: invalid configuration: receivers::otlp: must specify at least one protocol when using the OTLP receiver
service::telemetry: collector telemetry metrics reader should exist when metric level is not None, level is Normal

I'll clean this PR up once #12102 is merged, but you can see the latest commit for the changes relevant to only this PR.

@mx-psi
Copy link
Member

mx-psi commented Jan 23, 2025

I think this can be rebased and marked as ready for review

@mx-psi mx-psi added the release:blocker The issue must be resolved before cutting the next release label Jan 23, 2025
@evan-bradley evan-bradley force-pushed the validateconfig-print-path branch from 9b84570 to 38d4974 Compare January 24, 2025 20:04
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 97.87234% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.31%. Comparing base (c119b2a) to head (0cecae7).

Files with missing lines Patch % Lines
component/config.go 97.87% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12108      +/-   ##
==========================================
+ Coverage   91.28%   91.31%   +0.02%     
==========================================
  Files         465      465              
  Lines       25616    25693      +77     
==========================================
+ Hits        23384    23461      +77     
  Misses       1819     1819              
  Partials      413      413              

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

@evan-bradley evan-bradley marked this pull request as ready for review January 24, 2025 20:54
@evan-bradley evan-bradley requested a review from a team as a code owner January 24, 2025 20:54
component/config.go Outdated Show resolved Hide resolved
@evan-bradley
Copy link
Contributor Author

As expected, some contrib tests are failing despite no outward API changes. I feel like this is more of an indication that the tests should be rewritten than that we've made a breaking change: most of the test failures seem to be happening because they assume a specific structure of the error object instead of working with the error interface.

component/config.go Outdated Show resolved Hide resolved
component/config.go Outdated Show resolved Hide resolved
component/config.go Outdated Show resolved Hide resolved
component/config.go Outdated Show resolved Hide resolved
component/config.go Outdated Show resolved Hide resolved
component/config.go Outdated Show resolved Hide resolved
@evan-bradley evan-bradley requested a review from mx-psi January 29, 2025 16:29
component/config.go Outdated Show resolved Hide resolved
component/config.go Outdated Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Jan 29, 2025

Can we fix (some or all of) the contrib tests before merging this?

@evan-bradley
Copy link
Contributor Author

It's definitely possible, let me see what I can do.

component/config.go Outdated Show resolved Hide resolved
@evan-bradley
Copy link
Contributor Author

I opened open-telemetry/opentelemetry-collector-contrib#37579 to address hopefully most of the failing contrib tests. Unfortunately it's proven bit challenging to run everything locally.

mx-psi pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request Jan 30, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Fixes some errors introduced by
open-telemetry/opentelemetry-collector#12108.

Overall this makes tests that check errors resulting from config
validation more resilient by only checking that the error message
contains the information relevant to the component. The tests changed in
this PR make more precise assumptions about the error message that no
longer hold up if `component.ValidateConfig` returns errors with
different internal structures or messages.

I've run this locally with the changes in
open-telemetry/opentelemetry-collector#12108,
but I encountered some issues that prevented running tests locally in
some modules, so not all errors may be fixed. I will play more with this
tomorrow, but I also think it's safe to merge this as-is and fix the
remaining errors during a `make update-otel` PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:blocker The issue must be resolved before cutting the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants