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

Add config marshaler. #5566

Merged
merged 1 commit into from
Sep 29, 2022
Merged

Conversation

jefchien
Copy link
Contributor

@jefchien jefchien commented Jun 21, 2022

Description:
Add Marshal function to confmap. Update types implementing encoding.TextUnmarshaler to implement encoding.TextMarshaler as well. Add a mapstructure encoder that encodes the config provided using the existing mapstructure tags.

Link to tracking Issue: #5418

Testing: Added unit test to validate the unmarshal/marshal cycle.

@jefchien jefchien requested review from a team and bogdandrutu June 21, 2022 15:50
@codecov
Copy link

codecov bot commented Jun 22, 2022

Codecov Report

Base: 91.77% // Head: 91.87% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (deeacb6) compared to base (8060037).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5566      +/-   ##
==========================================
+ Coverage   91.77%   91.87%   +0.10%     
==========================================
  Files         218      219       +1     
  Lines       13364    13533     +169     
==========================================
+ Hits        12265    12434     +169     
  Misses        870      870              
  Partials      229      229              
Impacted Files Coverage Δ
config/configtelemetry/configtelemetry.go 100.00% <100.00%> (ø)
config/identifiable.go 100.00% <100.00%> (ø)
confmap/confmap.go 93.19% <100.00%> (+2.20%) ⬆️
confmap/internal/mapstructure/encoder.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to press submit, sorry.

config/configcompression/compressionType.go Outdated Show resolved Hide resolved
config/identifiable.go Outdated Show resolved Hide resolved
config/configtelemetry/configtelemetry.go Show resolved Hide resolved
receiver/otlpreceiver/config.go Outdated Show resolved Hide resolved
config/common.go Outdated Show resolved Hide resolved
@jefchien jefchien force-pushed the 5418-marshaler branch 2 times, most recently from ae602a6 to 04defbf Compare June 28, 2022 18:07
@jefchien
Copy link
Contributor Author

@bogdandrutu PTAL when you get the chance. Refactored the marshaler to be more similar to the unmarshaler.

config/common.go Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@jefchien
Copy link
Contributor Author

jefchien commented Aug 3, 2022

Changed the Marshallable interface to take in a confmap.Conf. Added a function on the confmap.Conf that uses the encoder and uses koanf.Merge to "marshal" it into the confmap.Conf.

confmap/confmap.go Outdated Show resolved Hide resolved
service/configmarshaler/defaultmarshaler.go Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I am all focus on this PR now :)

service/configmarshaler/defaultmarshaler.go Outdated Show resolved Hide resolved
config/exporter.go Outdated Show resolved Hide resolved
config/common.go Outdated Show resolved Hide resolved
@jefchien jefchien requested a review from bogdandrutu September 1, 2022 20:24
@jefchien jefchien force-pushed the 5418-marshaler branch 2 times, most recently from 6403a98 to 92822b6 Compare September 7, 2022 15:17
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain why the Encoder is still needed.

config/identifiable.go Outdated Show resolved Hide resolved
confmap/marshaler.go Outdated Show resolved Hide resolved
@jefchien
Copy link
Contributor Author

jefchien commented Sep 8, 2022

The Encoder handles all of the reflection needed to respect the mapstructure tags and is used by confmap to marshal. Without it, the marshal/unmarshal cycle would break and the configs being generated would be invalid.

@bogdandrutu
Copy link
Member

The Encoder handles all of the reflection needed to respect the mapstructure tags and is used by confmap to marshal. Without it, the marshal/unmarshal cycle would break and the configs being generated would be invalid.

Then the dependency graph is totally unexpected. Move that to confmap/internal/mapstructure if needed.

@jefchien jefchien force-pushed the 5418-marshaler branch 2 times, most recently from 0313fc1 to 468aecd Compare September 8, 2022 15:28
@jefchien
Copy link
Contributor Author

jefchien commented Sep 8, 2022

The Encoder handles all of the reflection needed to respect the mapstructure tags and is used by confmap to marshal. Without it, the marshal/unmarshal cycle would break and the configs being generated would be invalid.

Then the dependency graph is totally unexpected. Move that to confmap/internal/mapstructure if needed.

Moved under confmap/internal/mapstructure. The old path was a holdover from the initial implementation where the confmap didn't handle the marshalling.

receiver/otlpreceiver/config.go Outdated Show resolved Hide resolved
service/telemetry/config.go Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

receiver/otlpreceiver/config.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit e9311f6 into open-telemetry:main Sep 29, 2022
@jefchien jefchien deleted the 5418-marshaler branch November 12, 2022 00:43
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.

2 participants