Add a NewConfigProvider that uses options for settings, and does input validation #4936

bogdandrutu · 2022-03-01T23:28:44Z

This is a fork of #4762

Also, deprecates service.MustNewConfigProvider and service.MustNewDefaultConfigProviderin favor of service.NewConfigProvider.

Signed-off-by: Bogdan Drutu [email protected]

codecov · 2022-03-01T23:32:00Z

Codecov Report

Merging #4936 (9601c47) into main (95b976f) will decrease coverage by 0.04%.
The diff coverage is 87.93%.

@@            Coverage Diff             @@
##             main    #4936      +/-   ##
==========================================
- Coverage   90.33%   90.29%   -0.05%     
==========================================
  Files         185      185              
  Lines       11000    11041      +41     
==========================================
+ Hits         9937     9969      +32     
- Misses        837      844       +7     
- Partials      226      228       +2

Impacted Files	Coverage Δ
service/config_provider.go	`90.22% <85.10%> (-4.12%)`	⬇️
service/command.go	`80.76% <100.00%> (-1.59%)`	⬇️
model/internal/generated_log.go	`96.71% <0.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95b976f...9601c47. Read the comment docs.

mx-psi

I like this NewConfigProvider more than the current one. I left some comments

service/collector.go

service/config_provider.go

service/config_provider_test.go

github-actions · 2022-03-18T03:16:26Z

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

CHANGELOG.md

github-actions · 2022-04-05T03:19:04Z

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

Folllowup: * When the deprecated funcs/types are removed the internal package can be moved to service/internal. * Part of open-telemetry#4936 do not offer ability to configure ConfigUnmarshaler. Motivation: This package is removed because with the latest addition of the `service.ConfigProvider` the usecase to change the unmarshaled config.Config can be achieved by wrapping/implementing that interface, so no clear use-case for this. In the future we can expose it again if we have good reasons. Updates open-telemetry#4605 Signed-off-by: Bogdan Drutu <[email protected]>

Folllowup: * When the deprecated funcs/types are removed the internal package can be moved to service/internal. * Part of open-telemetry#4936 do not offer ability to configure ConfigUnmarshaler. Motivation: * This package is removed because with the latest addition of the `service.ConfigProvider` the usecase to change the unmarshaled config.Config can be achieved by wrapping/implementing that interface, so no clear use-case for this. In the future we can expose it again if we have good reasons. * During the review of another PR, this was mentioned as something some approvers/maintainers were concerned about what to do with this package. See open-telemetry#4608 (review) Updates open-telemetry#4605 Signed-off-by: Bogdan Drutu <[email protected]>

codeboten

Thanks for addressing my suggestions. Just one comment in the discussion w/ @Aneurysm9 & @mx-psi. It would be good to agree on the way to move forward.

service/config_provider.go

jpkrohling

LGTM, the only contention point seems to be about calling WithConfigMapConverters multiple times. Once that is sorted out, either by returning an error on the second call or by using a settings object instead of options, this would be ready to be merged.

service/config_provider.go

jpkrohling · 2022-04-08T16:43:45Z

service/collector_test.go

-			),
-		}))
+	cfgSet := NewDefaultConfigProviderSettings()
+	cfgSet.Locations = []string{


I didn't know this before, but I prefer this so much more than the previous iteration.

codeboten

Overall I like this a lot. Just a question and a couple of nits

service/config_provider.go

codeboten · 2022-04-08T18:36:22Z

service/config_provider.go

+	Unmarshaler configunmarshaler.ConfigUnmarshaler
+}
+
+func newDefaultConfigProviderSettings() ConfigProviderSettings {


I'm probably missing this somewhere, but as a user is there a way for me to obtain the defaults from outside this package? I'm thinking of the case where someone may only want to override the locations but not the default providers/converters

I think, if we go with the same model as the Factories for components, we should not have any "defaults" every distribution will pick their "defaults" that they commit to support. All the components that we use in the "default" config are also public, so a distro can emulate the same if this is what they want.

…ation Signed-off-by: Bogdan Drutu <[email protected]>

Signed-off-by: Bogdan Drutu <[email protected]>

mx-psi

Using a struct resolves the ambiguity of repeating an option, I only have two nits but otherwise LGTM

service/config_provider.go

mx-psi · 2022-04-11T14:44:50Z

service/command.go

+				var err error
+				cfgSet := newDefaultConfigProviderSettings()
+				cfgSet.Locations = getConfigFlag()
+				// Append the "overwrite properties converter" as the first converter.
+				cfgSet.MapConverters = append(
+					[]config.MapConverterFunc{overwritepropertiesmapconverter.New(getSetFlag())},
+					cfgSet.MapConverters...)
+				set.ConfigProvider, err = NewConfigProvider(cfgSet)
+				if err != nil {
+					return err
+				}


This and the collector_windows code are the same, should we extract it to a function?

Will do in a separate PR.

Signed-off-by: Bogdan Drutu <[email protected]>

Folllowup: * When the deprecated funcs/types are removed the internal package can be moved to service/internal. * Part of open-telemetry#4936 do not offer ability to configure ConfigUnmarshaler. Motivation: * This package is removed because with the latest addition of the `service.ConfigProvider` the usecase to change the unmarshaled config.Config can be achieved by wrapping/implementing that interface, so no clear use-case for this. In the future we can expose it again if we have good reasons. * During the review of another PR, this was mentioned as something some approvers/maintainers were concerned about what to do with this package. See open-telemetry#4608 (review) Updates open-telemetry#4605 Signed-off-by: Bogdan Drutu <[email protected]>

Signed-off-by: Bogdan Drutu <[email protected]>

Folllowup: * When the deprecated funcs/types are removed the internal package can be moved to service/internal. * Part of open-telemetry#4936 do not offer ability to configure ConfigUnmarshaler. Motivation: * This package is removed because with the latest addition of the `service.ConfigProvider` the usecase to change the unmarshaled config.Config can be achieved by wrapping/implementing that interface, so no clear use-case for this. In the future we can expose it again if we have good reasons. * During the review of another PR, this was mentioned as something some approvers/maintainers were concerned about what to do with this package. See open-telemetry#4608 (review) Updates open-telemetry#4605 Signed-off-by: Bogdan Drutu <[email protected]>

Folllowup: * When the deprecated funcs/types are removed the internal package can be moved to service/internal. * Part of #4936 do not offer ability to configure ConfigUnmarshaler. Motivation: * This package is removed because with the latest addition of the `service.ConfigProvider` the usecase to change the unmarshaled config.Config can be achieved by wrapping/implementing that interface, so no clear use-case for this. In the future we can expose it again if we have good reasons. * During the review of another PR, this was mentioned as something some approvers/maintainers were concerned about what to do with this package. See #4608 (review) Updates #4605 Signed-off-by: Bogdan Drutu <[email protected]>

…t validation (open-telemetry#4936) * Change deprecated NewConfigProvider to use options and do input validation Signed-off-by: Bogdan Drutu <[email protected]> * Revert deprecation of WithConfigUnmarshaler Signed-off-by: Bogdan Drutu <[email protected]> * Change to use settings for the ConfigProvider Signed-off-by: Bogdan Drutu <[email protected]> * Update service/config_provider.go Co-authored-by: Alex Boten <[email protected]>

…telemetry#5185) Signed-off-by: Bogdan Drutu <[email protected]>

…try#5151) Folllowup: * When the deprecated funcs/types are removed the internal package can be moved to service/internal. * Part of open-telemetry#4936 do not offer ability to configure ConfigUnmarshaler. Motivation: * This package is removed because with the latest addition of the `service.ConfigProvider` the usecase to change the unmarshaled config.Config can be achieved by wrapping/implementing that interface, so no clear use-case for this. In the future we can expose it again if we have good reasons. * During the review of another PR, this was mentioned as something some approvers/maintainers were concerned about what to do with this package. See open-telemetry#4608 (review) Updates open-telemetry#4605 Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu requested review from a team and mx-psi March 1, 2022 23:28

bogdandrutu force-pushed the GH-4730-0.45 branch from f983845 to e1fca07 Compare March 1, 2022 23:31

bogdandrutu force-pushed the GH-4730-0.45 branch from e1fca07 to bda2750 Compare March 1, 2022 23:47

bogdandrutu mentioned this pull request Mar 1, 2022

Remove deprecated NewConfigProvider funcs #4937

Merged

mx-psi mentioned this pull request Mar 2, 2022

Implement Stringer for service.State #4938

Merged

mx-psi reviewed Mar 2, 2022

View reviewed changes

service/collector.go Outdated Show resolved Hide resolved

service/config_provider.go Outdated Show resolved Hide resolved

service/config_provider.go Outdated Show resolved Hide resolved

service/config_provider_test.go Outdated Show resolved Hide resolved

github-actions bot added the Stale label Mar 18, 2022

bogdandrutu force-pushed the GH-4730-0.45 branch 3 times, most recently from d5ff8df to 9761e5a Compare March 21, 2022 09:11

bogdandrutu removed the Stale label Mar 21, 2022

mx-psi reviewed Mar 21, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

bogdandrutu force-pushed the GH-4730-0.45 branch from 9761e5a to 9d89a66 Compare March 21, 2022 11:13

neelayu mentioned this pull request Mar 31, 2022

Make collector flag functions public to allow collector builders to use them #5130

Closed

github-actions bot added the Stale label Apr 5, 2022

bogdandrutu removed the Stale label Apr 5, 2022

bogdandrutu mentioned this pull request Apr 5, 2022

Deprecate configunmarshaler package, move it to internal #5151

Merged

bogdandrutu force-pushed the GH-4730-0.45 branch from 9d89a66 to e79931b Compare April 5, 2022 18:08

bogdandrutu force-pushed the GH-4730-0.45 branch from e79931b to 1d88e19 Compare April 5, 2022 18:26

codeboten reviewed Apr 6, 2022

View reviewed changes

service/config_provider.go Outdated Show resolved Hide resolved

bogdandrutu force-pushed the GH-4730-0.45 branch from a3c5d6b to c177990 Compare April 7, 2022 20:06

jpkrohling reviewed Apr 8, 2022

View reviewed changes

service/config_provider.go Outdated Show resolved Hide resolved

service/config_provider.go Show resolved Hide resolved

bogdandrutu force-pushed the GH-4730-0.45 branch 3 times, most recently from 6b71755 to ea7c2b4 Compare April 8, 2022 16:49

jpkrohling approved these changes Apr 8, 2022

View reviewed changes

codeboten reviewed Apr 8, 2022

View reviewed changes

codeboten approved these changes Apr 8, 2022

View reviewed changes

bogdandrutu added 2 commits April 8, 2022 15:57

Change deprecated NewConfigProvider to use options and do input valid…

a7d0f00

…ation Signed-off-by: Bogdan Drutu <[email protected]>

Revert deprecation of WithConfigUnmarshaler

c4730cd

Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu force-pushed the GH-4730-0.45 branch 2 times, most recently from 422a3d2 to 46ce4bf Compare April 8, 2022 23:20

Change to use settings for the ConfigProvider

a08acfa

Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu force-pushed the GH-4730-0.45 branch from 46ce4bf to a08acfa Compare April 8, 2022 23:24

mx-psi approved these changes Apr 11, 2022

View reviewed changes

Update service/config_provider.go

9601c47

bogdandrutu merged commit cd5df21 into open-telemetry:main Apr 11, 2022

bogdandrutu deleted the GH-4730-0.45 branch April 11, 2022 16:05

bogdandrutu added a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Apr 11, 2022

Followup after open-telemetry#4936, remove some duplicate code

9efd371

Signed-off-by: Bogdan Drutu <[email protected]>

bogdandrutu added a commit that referenced this pull request Apr 11, 2022

Followup after #4936, remove some duplicate code (#5185)

7d6984b

Signed-off-by: Bogdan Drutu <[email protected]>

dmitryax mentioned this pull request Apr 12, 2022

Update core dependency and adopt to its API changes open-telemetry/opentelemetry-collector-contrib#9231

Merged

vasireddy99 mentioned this pull request May 2, 2022

Lambda Layer update : Update Collector 0.51 open-telemetry/opentelemetry-lambda#231

Merged

Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022

Followup after open-telemetry#4936, remove some duplicate code (open-…

2a13fa1

…telemetry#5185) Signed-off-by: Bogdan Drutu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a NewConfigProvider that uses options for settings, and does input validation #4936

Add a NewConfigProvider that uses options for settings, and does input validation #4936

bogdandrutu commented Mar 1, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading

mx-psi left a comment

github-actions bot commented Mar 18, 2022

github-actions bot commented Apr 5, 2022

codeboten left a comment

jpkrohling left a comment

jpkrohling Apr 8, 2022

codeboten left a comment

codeboten Apr 8, 2022

bogdandrutu Apr 8, 2022

mx-psi left a comment

mx-psi Apr 11, 2022

bogdandrutu Apr 11, 2022

Add a NewConfigProvider that uses options for settings, and does input validation #4936

Add a NewConfigProvider that uses options for settings, and does input validation #4936

Conversation

bogdandrutu commented Mar 1, 2022 • edited Loading

codecov bot commented Mar 1, 2022 • edited Loading

Codecov Report

mx-psi left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2022

github-actions bot commented Apr 5, 2022

codeboten left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling Apr 8, 2022

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

codeboten Apr 8, 2022

Choose a reason for hiding this comment

bogdandrutu Apr 8, 2022

Choose a reason for hiding this comment

mx-psi left a comment

Choose a reason for hiding this comment

mx-psi Apr 11, 2022

Choose a reason for hiding this comment

bogdandrutu Apr 11, 2022

Choose a reason for hiding this comment

bogdandrutu commented Mar 1, 2022 •

edited

Loading

codecov bot commented Mar 1, 2022 •

edited

Loading