search

open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.13k stars 539 forks source link

config: Parse model from YAML #4373

Open pellared opened 11 months ago

pellared commented 11 months ago

Implement https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#parse for YAML.

pellared commented 11 months ago

PTAL @codeboten

carrbs commented 8 months ago

@codeboten , @pellared should I implement this, (and remove the comment about JSON?), since we are likely just implementing for YAML atm?

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/96790b3addf68bdceb23a44d8f080085bfb82936/config/config.go#L116-L118

pellared commented 8 months ago

Works for me but I am leaving the decision to @codeboten

codeboten commented 8 months ago

i think thats a sensible way forward

carrbs commented 7 months ago

Spent some more time looking into this and I have questions on what the scope of this issue is. I'm new to otel, so hopefully you can give me some grace if there are obvious oversights or overly simple questions.

I can see that this repo is leveraging the "official" opentelemetry specification schema to render the generated_config.go file.

  1. Should the Parse(config_file) function utilize this generated config for validation?
  2. Is the purpose to ensure that all known properties from the generated config map from the input YAML to the expected structs in the generated config file, or just to focus on the environment variable handling?
  3. If the YAML input file contains mapping unknown to the schema, how is that handled?
    • The examples describing the nuance around environment variables are not part of the schema, and I also saw the "additionalProperties": true on the schema ☝️.

Any thoughts on this would be awesome, I'd be happy to help on this but if it's not high priority we can table it :)

MadVikingGod commented 6 months ago

The current generated config uses the mapstructure tags. This means that parsing yaml (and json) is done by marshaling raw yaml into a map[string]interface{} and then using the mapstructure package to Decode() the map into the actual config.

I think it might be best if we have an example of this.

pellared commented 6 months ago

Regarding env var substitution: https://github.com/open-telemetry/opentelemetry-specification/issues/3914#issuecomment-1972753424

codeboten commented 4 months ago

Thanks all for the patience, I'm finally getting back to the work on the config package.

  1. Is the purpose to ensure that all known properties from the generated config map from the input YAML to the expected structs in the generated config file, or just to focus on the environment variable handling?

Ideally yes. It's okay to implement these as we go, as in I wouldn't worry about supporting everything right away, we can always add new issues for features that are not supported, this is how the development has been done so far :)

  1. If the YAML input file contains mapping unknown to the schema, how is that handled?

    • The examples describing the nuance around environment variables are not part of the schema, and I also saw the "additionalProperties": true on the schema ☝️.

@carrbs Support for additional properties should be addressed by https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4832. I submitted a prototype upstream to the go-jsonschema library and it's since been implemented in the library.

The current generated config uses the mapstructure tags. This means that parsing yaml (and json) is done by marshaling raw yaml into a map[string]interface{} and then using the mapstructure package to Decode() the map into the actual config.

@MadVikingGod Correct, this is what the Collector does today with this configuration. I don't know if we have a strong preference taking this in a different direction, if we wanted to support additional struct tags. I'm open to ideas from the maintainers/approvers in this community.

codeboten commented 4 months ago

@MadVikingGod @carrbs an alternative is to generate the yaml struct tags and avoid the mapstructure middle step, see https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5433 as an example

carrbs commented 3 months ago

@MadVikingGod @codeboten It's not immediately apparent to me if/how #5433 resolves this issue. If it does, can we have a bit more of an explanation here? Otherwise, I'd be happy to refresh the PR I started, and get this working.

More generally, I still feel like the documentation is confusing around what file/file formats are valid (as mentioned in #4412), and also mentioned by @MadVikingGod in #5433.