[confmap] Improve error messages on invalid YAML

[tsloughter]

Describe the bug

A duplicate key in the configuration like this duplicate use of http:

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: "0.0.0.0:4317"
      http:
        endpoint: "0.0.0.0:4318"
      http:
        cors:
          allowed_origins:
          - "http://localhost:8000"
        endpoint: "0.0.0.0:4318"

when run with main results in the error message:

$ bin/otelcorecol_linux_amd64 --config ../roll_dice_gleam/otel-collector-config.yaml
Error: failed to get config: cannot resolve the configuration: retrieved value (type=string) cannot be used as a Conf
2024/12/31 10:58:11 collector server run finished with error: failed to get config: cannot resolve the configuration: retrieved value (type=string) cannot be used as a Conf

This makes no mention of where the issue in the file was found or that it was a duplicate key.

Compare to what the error message was as of commit 334d95aebcffc88fa19ac4233dc166dd51aaa55f:

Error: failed to resolve config: cannot resolve the configuration: cannot retrieve the configuration: yaml: unmarshal errors:
  line 9: mapping key "http" already defined at line 7
2024/12/31 10:53:08 collector server run finished with error: failed to resolve config: cannot resolve the configuration: cannot retrieve the configuration: yaml: unmarshal errors:
  line 9: mapping key "http" already defined at line 7

The line number and the key http was given and that it was a duplicate issue.

Steps to reproduce

Run a recent collector with the configuration like the above.

What did you expect to see?

The previous error message as shown above.

What did you see instead?

An error message that does not give details on the actual issue.

What version did you use?

does't work: ffcef93
works: 334d95a

What config did you use?

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: "0.0.0.0:4317"
      http:
        endpoint: "0.0.0.0:4318"
      http:
        cors:
          allowed_origins:
          - "http://localhost:8000"
        endpoint: "0.0.0.0:4318"

exporters:
  debug:
    verbosity: detailed
    sampling_initial: 1
    sampling_thereafter: 1

  pipelines:
    traces:
      receivers: [otlp]
      processors: [h]
      exporters: [debug]
    metrics:
      receivers: [otlp]
      processors: []
      exporters: [debug]
    logs:
      receivers: [otlp]
      processors: [batch]
      exporters: [debug]

Environment

Fedora 41
Go 1.22.0

[mattsains]

I did a bisection and found that the regression was introduced by this PR: #10794

[mattsains]

the main issue as I understand it is that Providers tend to call confmap.NewRetrievedFromYAML when they've done a variable substitution. However, the substitution done by a provider may not be valid yaml (it might be a string as in #10759, which is what PR #10794 fixed, or it might be another variable substitution like [filelog,${env:ENV_VALUE}]).

This was fixed in the PR by reacting to all yaml errors by treating them as raw strings instead. However, this is a problem because it masks all yaml errors in the entire config by returning a string of invalid yaml, which isn't the same type as the otel config.

It's harder to fix this because of the Provider contract and also because the recursive evaluation is handled through a loop rather than the stack, and the loop can't tell why the expansion failed.

I'm still working on a solution here

[mattsains]

There are two conflicting requirements here:

1. You are able to include a file which will be interpreted as yaml if it can be, else it will be interpreted as a string
2. (This issue) if you make a yaml mistake in your config file, the collector should tell you what's wrong

Consider an otel config file like this:

config.yaml:

${file:b.yaml}

b.yaml:

invalidyaml:]

obviously it would be nice if the error happened after b.yaml was filled into config.yaml, pointing you to the ], but if we did that, then the following file which relies on requirement 1 would not work:

config.yaml:

receivers:
  my_rec:
    password: ${file:b.yaml}

b.yaml:

apassword]not}-yaml

In my opinion, requirement 1 should be removed and instead we should require users to encase the substitution in quotes, like this:

receivers:
  my_rec:
    password: "${file:b.yaml}"

but is it too late to change this? Perhaps @mx-psi could weigh in

[mx-psi]

Thanks for reporting! This is a known issue, I mention it on the PR that you linked (#10794) and point to a possible solution:

This has the downside that the error messages are less informative: it will tell you it expected something other than a string instead of the YAML parser error.
A future improvement could be to pass these errors down as extra metadata up until the unmarshaling stage.

I agree that the error message is confusing and that it would be good if we pointed to the specific issue.

---

In my opinion, requirement 1 should be removed and instead we should require users to encase the substitution in quotes, like this: [...]

This would be a breaking change in how configuration parsing works. We have already marked confmap as 1.0 so this is not a possibility.

It is not to late however to plumb the error through with a similar mechanism to expandedValue that we use for passing values with ambiguous types. (We explicitly say this is fine to change given the VERSIONING.md document under "String representation").

[mattsains]

not super sure I understand what you're suggesting. Are you saying that the Retrieved struct should be modified to be able to accept a wrapped type like this?

type invalidYamlStr struct {
	Err error
	Original string
}

It seems to me like Retrieved is explicitly designed for "normal" types, since it calls this method: checkRawConfType which only accepts build-ins like strings, maps, ints, etc

[mx-psi]

@mattsains Retrieved would have to store an error and then the resolver would have to have a invalidYAMLstr struct like you said and plumb it through. That would mean:

• A new yamlErr field on Retrieved
• A new method on retrieved to get the field
• Modifying NewRetrievedFromYAML to store this error
• Plumbing this through the resolver which includes creating the invalidYAMLstr and storing it in confmap.Conf in the proper place, as well as adding a bunch of cases (you can look at the ones for expandedValue here)
• Modifying the hooks that translate confmap.Conf into a struct so that they can handle invalidYAMLstr (see here the hook for expandedValue, again this would be similar)
• Sanitizing the map string representation to hide the invalidYAMLstr as an internal implementation detail

This is doable, the hard part is testing everything. The original implementation of expandedValue took quite a few attempts to get it right (as can be seen by the multiple issues referenced in the test suite).

The main reason I didn't do this at the beginning is because I was scared of breaking users once more. I think with enough tests this should be a non issue.

[mx-psi]

Thinking about this more, there is a simpler alternative, which is to store a map of strings to its YAML error in confmap.Conf, although you would not be able to tell where the error happened, this would still be an improvement over the current state and would be much simpler to implement

[mx-psi]

#12180 is a limited improvement here: it would surface the error in the case mentioned on this issue, but it would not surface the error in more complex setups where a file references other files. I need to write tests for it but it should work and improve the situation (help welcome :))

[mx-psi]

I merged #12180. I am going to keep this issue open so that we eventually implement the more complex error-passing mechanism and this works in all contexts