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

[chore][confmap]: print a warning if invalid character is encountered in yaml #11292

Conversation

VihasMakwana
Copy link
Contributor

Description

If you use the following (invalid) configuration and start the collector:

receivers:
  receiver:
	  field: val

you'll see an error like this:

Error: failed to get config: cannot resolve the configuration: retrieved value (type=string) cannot be used as a Conf
2024/09/28 03:11:06 collector server run finished with error: failed to get config: cannot resolve the configuration: retrieved value (type=string) cannot be used as a Conf

However, the error message isn’t helpful. The issue arises from an invalid character (a horizontal tab - ASCII code 9) in the YAML, which may not be visible in text editors. It would be beneficial to print a more informative error message when such problems are detected.

This is a side-effect of #10794.

@VihasMakwana VihasMakwana requested a review from a team as a code owner September 27, 2024 21:46
@VihasMakwana
Copy link
Contributor Author

After this change, the error message would look like:

unmarshaling from yaml failed: yaml: line 3: found character that cannot start any token
Attempting to use it as a string verbatim. You can ignore this warning if the collector starts successfully.

Error: failed to get config: cannot resolve the configuration: retrieved value (type=string) cannot be used as a Conf
2024/09/28 03:18:00 collector server run finished with error: failed to get config: cannot resolve the configuration: retrieved value (type=string) cannot be used as a Conf

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.44%. Comparing base (c155b14) to head (383bfd9).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11292      +/-   ##
==========================================
- Coverage   91.57%   91.44%   -0.13%     
==========================================
  Files         425      422       -3     
  Lines       20205    20251      +46     
==========================================
+ Hits        18502    18518      +16     
- Misses       1319     1351      +32     
+ Partials      384      382       -2     

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

@@ -143,6 +143,7 @@ func withStringRepresentation(stringRepresentation string) RetrievedOption {
func NewRetrievedFromYAML(yamlBytes []byte, opts ...RetrievedOption) (*Retrieved, error) {
var rawConf any
if err := yaml.Unmarshal(yamlBytes, &rawConf); err != nil {
fmt.Printf("unmarshaling from yaml failed: %v\nAttempting to use it as a string verbatim. You can ignore this warning if the collector starts successfully.\n\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.

I shouldn't use fmt.Printf. We need to print everything with configured loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I was confused about the same.
I followed the same step as config.Validate(..).

It prints a message if service::telemetry::metrics validation fails.

Needs some additional work to involve loggers in this section but I get your point.

@dmitryax
Copy link
Member

The current message isn't ideal, but I don't think the additional helps significantly

@VihasMakwana
Copy link
Contributor Author

The current message isn't ideal, but I don't think the additional helps significantly

I agree to this one. I think in an ideal scenario, we should only have one error message out of two. Let me know what do you think.

@VihasMakwana
Copy link
Contributor Author

@dmitryax I think this whole thing boils down to using config as string.

If we encounter errors while decoding the otel yaml config itself, we should just throw an error straight away (dont use it verbatim as a string because there's no point of doing that)

If we successfully pass decoding step, then we should us it verbatim as a string while expanding.

What are your thoughts?

@mx-psi
Copy link
Member

mx-psi commented Oct 1, 2024

I think the right approach here is to have some sort of wrapper that adds additional error information to the Conf object and to show this error information if parsing fails for some reason.

@VihasMakwana
Copy link
Contributor Author

I'll open a new PR with another approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants