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: add yaml/json struct tags #5433

Merged

Conversation

codeboten
Copy link
Contributor

This will make parsing the configuration easier

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️
config/generated_config.go 53.3% <ø> (+53.3%) ⬆️

@MadVikingGod
Copy link
Contributor

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?

@pellared
Copy link
Member

pellared commented May 7, 2024

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

@codeboten
Copy link
Contributor Author

@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 :)

@carrbs
Copy link
Contributor

carrbs commented May 13, 2024

@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!

@MadVikingGod
Copy link
Contributor

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.

@MrAlias
Copy link
Contributor

MrAlias commented Jun 26, 2024

@MadVikingGod PTAL

Signed-off-by: Alex Boten <[email protected]>
@codeboten codeboten force-pushed the codeboten/additional-struct-tags branch from 772047e to 31f265c Compare October 2, 2024 20:32
@codeboten codeboten requested a review from a team as a code owner October 2, 2024 20:32
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
@codeboten
Copy link
Contributor Author

Added test to validate json/yaml parsing is working as expected, also pulled in part of #4826 for adding a ParseYAML api

@pellared pellared merged commit 722a536 into open-telemetry:main Oct 3, 2024
24 of 25 checks passed
@codeboten codeboten deleted the codeboten/additional-struct-tags branch October 3, 2024 15:19
MrAlias added a commit that referenced this pull request Oct 14, 2024
### Added

- The `Severitier` and `SeverityVar` types are added to
`go.opentelemetry.io/contrib/processors/minsev` allowing dynamic
configuration of the severity used by the `LogProcessor`. (#6116)
- Move examples from `go.opentelemetry.io/otel` to this repository under
`examples` directory. (#6158)
- Support yaml/json struct tags for generated code in
`go.opentelemetry.io/contrib/config`. (#5433)
- Add support for parsing YAML configuration via `ParseYAML` in
`go.opentelemetry.io/contrib/config`. (#5433)
- Add support for temporality preference configuration in
`go.opentelemetry.io/contrib/config`. (#5860)

### Changed

- The function signature of `NewLogProcessor` in
`go.opentelemetry.io/contrib/processors/minsev` has changed to accept
the added `Severitier` interface instead of a `log.Severity`. (#6116)
- Updated `go.opentelemetry.io/contrib/config` to use the
[v0.3.0](https://github.com/open-telemetry/opentelemetry-configuration/releases/tag/v0.3.0)
release of schema which includes backwards incompatible changes. (#6126)
- `NewSDK` in `go.opentelemetry.io/contrib/config` now returns a no-op
SDK if `disabled` is set to `true`. (#6185)
- The deprecated
`go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`
package has found a Code Owner. The package is no longer deprecated.
(#6207)

### Fixed

- Possible nil dereference panic in
`go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace`.
(#5965)
- `logrus.Level` transformed to appropriate `log.Severity` in
`go.opentelemetry.io/contrib/bridges/otellogrus`. (#6191)

### Removed

- The `Minimum` field of the `LogProcessor` in
`go.opentelemetry.io/contrib/processors/minsev` is removed.
  Use `NewLogProcessor` to configure this setting. (#6116)
- The deprecated
`go.opentelemetry.io/contrib/instrumentation/gopkg.in/macaron.v1/otelmacaron`
package is removed. (#6186)
- The deprecated `go.opentelemetry.io/contrib/samplers/aws/xray` package
is removed. (#6187)

---------

Co-authored-by: David Ashpole <[email protected]>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants