-
Notifications
You must be signed in to change notification settings - Fork 565
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
Comments
PTAL @codeboten |
@codeboten , @pellared should I implement this, (and remove the comment about JSON?), since we are likely just implementing for YAML atm? opentelemetry-go-contrib/config/config.go Lines 116 to 118 in 96790b3
|
Works for me but I am leaving the decision to @codeboten |
i think thats a sensible way forward |
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.
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 :) |
The current generated config uses the I think it might be best if we have an example of this. |
Regarding env var substitution: open-telemetry/opentelemetry-specification#3914 (comment) |
Thanks all for the patience, I'm finally getting back to the work on the config package.
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 :)
@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.
@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. |
@MadVikingGod @carrbs an alternative is to generate the yaml struct tags and avoid the mapstructure middle step, see #5433 as an example |
@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. |
Implement https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#parse for YAML.
The text was updated successfully, but these errors were encountered: