opentelemetry-go-contrib icon indicating copy to clipboard operation
opentelemetry-go-contrib copied to clipboard

config: add yaml/json struct tags

Open codeboten opened this issue 1 year ago • 7 comments

This will make parsing the configuration easier

codeboten avatar Apr 26 '24 22:04 codeboten

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.4%. Comparing base (793b970) to head (1aac30d). Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #5433     +/-   ##
=======================================
+ Coverage   66.8%   67.4%   +0.6%     
=======================================
  Files        206     206             
  Lines      13211   13217      +6     
=======================================
+ Hits        8833    8919     +86     
+ Misses      4111    3996    -115     
- Partials     267     302     +35     
Files with missing lines Coverage Δ
config/config.go 84.3% <100.0%> (+2.0%) :arrow_up:
config/generated_config.go 53.3% <ø> (+53.3%) :arrow_up:

codecov[bot] avatar Apr 26 '24 22:04 codecov[bot]

This needs a changelog.

I see this adds additional tags, but I'm not 100% sure how this is used. It doesn't necessarily need to be part of the PR, but could you share an example of how this was used before and how it works now?

MadVikingGod avatar May 07 '24 14:05 MadVikingGod

Can you add tests that verify that the YAML/JSON files are parsed (thanks to the tags) as expected?

pellared avatar May 07 '24 14:05 pellared

@MadVikingGod @pellared happy to add some functionality to test it, i didn't want to overstep on this PR https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4826

@carrbs if you're ok with it I can incorporate your work here, or you can pull this change in your PR, it's all the same to me :)

codeboten avatar May 08 '24 14:05 codeboten

@MadVikingGod @pellared happy to add some functionality to test it, i didn't want to overstep on this PR #4826

@carrbs if you're ok with it I can incorporate your work here, or you can pull this change in your PR, it's all the same to me :)

I'm good with incorporating here, thanks @codeboten!

carrbs avatar May 13 '24 15:05 carrbs

So, what I was hoping for to understand better why this change is necessary was something like: Before:

input := map[string]interface{}
err := json.Unmarshal(rawJson, &input)
config := config{}
err = mapstructure.Decode(input, &config)

But afterwards we have

Some happy path code

While this looks to add direct ways to decode directly into our Config structures, it also means we have to test three different decoders (Mapstructure, Json, and Yaml) and find all the edge cases of each.

MadVikingGod avatar May 23 '24 15:05 MadVikingGod

@MadVikingGod PTAL

MrAlias avatar Jun 26 '24 16:06 MrAlias

Added test to validate json/yaml parsing is working as expected, also pulled in part of https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4826 for adding a ParseYAML api

codeboten avatar Oct 02 '24 22:10 codeboten