Add ability to marshal yaml-tagged structs

[djaglowski]

Possible solution for #10139 (comment)

More thorough explanation here: #10139 (comment)

Some open questions:

1. Is the "0 mapstructure tags and 1 or more yaml tags" rule accurate enough to be useful here? Is there a better rule?
2. Are there any other test cases I should add?
3. If all of the above are good, does this warrant a changelog?

[evan-bradley]

Thanks for digging into this. Overall I think this is a clean solution to the problem that will likely adequately address it.

I do have a few questions:

1. How do you think this compares to [confmap] Read YAML struct tags #10200? That one mostly keeps the current marshaling behavior, but also takes into account yaml tags for structural concerns (the issue that I was seeing in [otelcol] Obtain the Collector's effective configuration from otelcol.Config #10139 was related to invalid zero-valued structs not being left out when marshaling into a map). If we're marshaling to a non-YAML format, this could change the representation.
2. I think unmarshaling to a map[string]any with the YAML library may cause some type information to get lost. When debugging a few weeks back I think I saw that some types are kept during marshaling otelcol.Config to *confmap.Conf, will that cause any issues?

I can try to find answers for these as well, just wasn't sure what you may have found.

[djaglowski]

@evan-bradley, #10200 looks like a pretty good solution too. I am not very confident in my understanding how this all fits together but it seems 10200 tries to solve it at a higher level by normalizing the two styles, while this PR tries to redirect to what might be the more "native" way to do things.

the issue that I was seeing in #10139 was related to invalid zero-valued structs not being left out when marshaling into a map

Can you give me an example? I focused mostly on working towards some test cases so easily could have overlooked this. If an example of this fit into the test cases I can try to adapt this PR to handle it.

I think unmarshaling to a map[string]any with the YAML library may cause some type information to get lost. When debugging a few weeks back I think I saw that some types are kept during marshaling otelcol.Config to *confmap.Conf, will that cause any issues?

Off the top of my head I suppose more specific maps e.g. map[string]string would unnecessarily be converted to map[string]any. Is that what you mean? There might be a way to handle a reasonable set of map types.

[evan-bradley]

Can you give me an example? I focused mostly on working towards some test cases so easily could have overlooked this. If an example of this fit into the test cases I can try to adapt this PR to handle it.

The Prometheus TLSVersion type has a zero value that cannot be marshaled into YAML. It's included in the Prometheus receiver's HTTPClientConfig struct that is used for scrape configs and includes the TLS version as a field which has omitempty tags for yaml and json. Since our encoder doesn't read these tags, we include the struct when marshaling a *confmap.Conf object. Later when calling yaml.Marshal on the *confmap.Conf, it fails since MarhsalYAML on TLSVersion returns an error unless the user explicitly set the option.

Off the top of my head I suppose more specific maps e.g. map[string]string would unnecessarily be converted to map[string]any. Is that what you mean? There might be a way to handle a reasonable set of map types.

Specifically, here's a snippet of a map[string]any that is returned from a setup I have for #10139. I am iterating through the map, collecting the path to all keys, and printing out their types with the %T formatter.

exporters::otlphttp/own_metrics::compression: configcompression.Type
exporters::otlphttp/own_metrics::retry_on_failure::max_interval: time.Duration
exporters::otlphttp/own_metrics::retry_on_failure::enabled: bool
exporters::otlphttp/own_metrics::retry_on_failure::multiplier: float64
exporters::otlphttp/own_metrics::tls::server_name_override: string
exporters::otlphttp/own_metrics::tls::cert_pem: string
exporters::otlphttp/own_metrics::tls::cipher_suites: []interface {}
exporters::otlphttp/own_metrics::encoding: otlphttpexporter.EncodingType
receivers::prometheus/own_metrics::config::tracingconfig::clienttype: config.TracingClientType
receivers::prometheus/own_metrics::config::tracingconfig::timeout: model.Duration
receivers::prometheus/own_metrics::config::tracingconfig::tlsconfig::certfile: string
receivers::prometheus/own_metrics::config::tracingconfig::tlsconfig::key: config.Secret

Types like config.TracingClientType (a third-party type with only yaml tags) likely would be rendered down to string types with the approach in this PR. Maybe this doesn't matter, is actually a better outcome, or just means we need more String methods on our types to ensure this is handled by the config struct itself. It's just a difference I thought we would want to make sure we are deliberate about if changing this.

[djaglowski]

The Prometheus TLSVersion type has a zero value that cannot be marshaled into YAML. It's included in the Prometheus receiver's HTTPClientConfig struct that is used for scrape configs and includes the TLS version as a field which has omitempty tags for yaml and json. Since our encoder doesn't read these tags, we include the struct when marshaling a *confmap.Conf object. Later when calling yaml.Marshal on the *confmap.Conf, it fails since MarhsalYAML on TLSVersion returns an error unless the user explicitly set the option.

I'm probably missing something because I'm not very familiar with the internals of the confmap package but I think my PR should handle this correctly because it does read the yaml tags because it sees that the struct only has yaml tags and runs it through the yaml package, which respects the omitempty on the yaml tag. I think it's covered in the test cases because this field is omitted when empty despite only being defined with a yaml tag.

Specifically, here's a snippet of a map[string]any that is returned from a setup I have for #10139. I am iterating through the map, collecting the path to all keys, and printing out their types with the %T formatter.

What's the context of the map[string]any here? Are you saying that when you marshal a confmap.Conf, you get a map[string]any that contains all these types? Or are you saying that the map[string]any is an ad hoc description of the confmap.Conf, basically "keys paths" to types?

Types like config.TracingClientType (a third-party type with only yaml tags) likely would be rendered down to string types with the approach in this PR.

Again probably missing something but why would it render to a string? I would expect a map[string]any that is approximately what you'd get if marshaling with mapstructure directly, except that the effects of the yaml tags were taken into account appropriately (because the original struct was marshaled via yaml, then unmarshaled via yaml into map[string]any.

[evan-bradley]

@djaglowski and I had an offline discussion about this yesterday, here were our outcomes:

• The current functionality of (*confmap.Conf).Marshal that does not marshal some of the "leaf node" types (i.e. types that don't have fields under them like exporters::otlphttp/own_metrics::compression in my snippet above) shouldn't be considered a feature or functionality that should be relied upon. This is caused by the fact that these types don't implement certain interfaces which would cause them to be marshaled into a primitive value such as a string. I will issue a follow-up to determine which interfaces are required.
• There should be a follow-up effort to determine whether we want to pass a *confmap.Conf object to extensions that implement the ConfigWatcher interface.

Given that we're okay with the difference in behavior between this PR and #10200, I would say that I prefer this to #10200 since it is simpler and allows us to rely on a third-party library rather than have an implicit dependency on that library's struct tags. I will also follow up on the concrete differences in the final YAML representation between this approach and #10200.

[evan-bradley]

Overall this looks good, I'll run some tests to compare it to #10200.

This will need a changelog entry as well.

[evan-bradley]

Follow ups:

1. Regarding the resulting config from this PR vs. [confmap] Read YAML struct tags #10200, they are equivalent except for a small difference in Prometheus' config enabled by the use of the yaml.Marshaler interface in this PR. In particular, Prometheus mutates the config struct during marshaling inside the MarshalYAML method on the config, which this PR will call. I think this is a good change.
2. Regarding why configopaque.String is marshaled to a string while configcompression.Type is left unmarshaled, it comes down to our encoder's support for the encoding.TextMarshaler interface, which configopaque.String implements. This is unlikely to pose any issues since structs that have yaml tags should implement marshaling through MarshalYAML.

[codecov]

Codecov Report

Attention: Patch coverage is 76.47059% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (2c0ed85) to head (6e24246).
Report is 5 commits behind head on main.

❗ Current head 6e24246 differs from pull request most recent head 142fff1

Please upload reports for the commit 142fff1 to get more accurate results.

Files	Patch %	Lines
confmap/internal/mapstructure/encoder.go	75.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10282      +/-   ##
==========================================
+ Coverage   92.53%   92.56%   +0.03%     
==========================================
  Files         388      387       -1     
  Lines       18317    18269      -48     
==========================================
- Hits        16949    16911      -38     
+ Misses       1022     1011      -11     
- Partials      346      347       +1     

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

[evan-bradley]

Looks good to me, I tested this with a local build and everything looked like how I would expect. Thanks for taking this.