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

config: Parse model from YAML #4373

Open
pellared opened this issue Oct 2, 2023 · 9 comments · May be fixed by #4826
Open

config: Parse model from YAML #4373

pellared opened this issue Oct 2, 2023 · 9 comments · May be fixed by #4826
Labels
area: config Related to config functionality

Comments

@pellared
Copy link
Member

pellared commented Oct 2, 2023

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

@pellared pellared added the area: config Related to config functionality label Oct 2, 2023
@pellared pellared changed the title configyaml: Parse model from YAML config: Parse model from YAML Oct 10, 2023
@pellared
Copy link
Member Author

PTAL @codeboten

@pellared pellared changed the title config: Parse model from YAML config: Add parse OpenTelemetryConfiguration from JSON example Jan 16, 2024
@pellared pellared changed the title config: Add parse OpenTelemetryConfiguration from JSON example config: Add parse OpenTelemetryConfiguration from YAML example Jan 16, 2024
@pellared pellared changed the title config: Add parse OpenTelemetryConfiguration from YAML example config: Parse model from YAML Jan 16, 2024
@carrbs
Copy link
Contributor

carrbs commented Jan 16, 2024

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

// TODO: implement parsing functionality:
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4373
// - https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4412

@pellared
Copy link
Member Author

pellared commented Jan 16, 2024

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

@codeboten
Copy link
Contributor

i think thats a sensible way forward

@carrbs carrbs linked a pull request Jan 18, 2024 that will close this issue
@carrbs
Copy link
Contributor

carrbs commented Jan 19, 2024

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
Copy link
Contributor

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
Copy link
Member Author

pellared commented Mar 1, 2024

Regarding env var substitution: open-telemetry/opentelemetry-specification#3914 (comment)

@codeboten
Copy link
Contributor

codeboten commented Apr 23, 2024

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 #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
Copy link
Contributor

@MadVikingGod @carrbs an alternative is to generate the yaml struct tags and avoid the mapstructure middle step, see #5433 as an example

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

Successfully merging a pull request may close this issue.

4 participants