Deprecate `config.Config` and `config.Service`, use `service.Config*` #4608

bogdandrutu · 2021-12-21T22:42:37Z

Followup PRs:

Move service/internal/configunmarshaler as private struct into service/ to avoid circular dependency (no public API change)
Change service/internal/builders to private structs in service/ or don't pass config, instead only pass the components and pipeline from the config.

While doing this, I realize that the "service" section in our config is a bit useless, we can think of promoting everything under "service::" one level up.

codecov · 2021-12-21T22:45:34Z

Codecov Report

Merging #4608 (3bbcd72) into main (85c26ea) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4608   +/-   ##
=======================================
  Coverage   90.63%   90.63%           
=======================================
  Files         190      190           
  Lines       11435    11435           
=======================================
  Hits        10364    10364           
  Misses        851      851           
  Partials      220      220

Impacted Files	Coverage Δ
config/common.go	`100.00% <ø> (ø)`
config/moved_config.go	`100.00% <ø> (ø)`
service/service.go	`41.79% <ø> (ø)`
service/config_provider.go	`90.32% <100.00%> (ø)`
service/featuregate/gates.go	`100.00% <0.00%> (ø)`

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 85c26ea...3bbcd72. Read the comment docs.

service/serviceconfig/service.go

github-actions · 2022-02-02T03:15:44Z

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

bogdandrutu · 2022-02-14T16:52:35Z

ping @open-telemetry/collector-approvers

tigrannajaryan

LGTM.

Given that there were questions and arguments in favour and against this change in the issue I would like other approvers to also have a look at this.

Aneurysm9 · 2022-02-14T23:42:28Z

I don't think my questions and concerns from #4605 have been addressed. I still don't know what the plan is here.

codeboten

After reviewing the code in the PR I think the only question i have is the same as @Aneurysm9 mentioned before around what the plan to decouple configunmarshaler is

bogdandrutu · 2022-02-16T16:44:32Z

@codeboten I keep thinking about the need to expose ConfigUnmarshaler in the first place (maybe the answer is not needed). But if we are keeping it I think we will have "util" funcs around the config.Receiver to unmarshal that UnmarshalReceiverConfig(default config.Receiver). The reason is that in the "config" package after this change, there is no concept of "Service" so that packages offers utils for what it provides. The rest of the ConfigUnmarshaler will leave somewhere in the service package.

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]>

github-actions · 2022-04-09T03:17:35Z

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. * 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]>

github-actions · 2022-04-29T03:37:07Z

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

bogdandrutu · 2022-05-12T23:42:10Z

Right, in that case any reason not to move it as part of this change? This way we at least avoid introducing a dependency between on serviceconfig from a config package.

After some time, I got to the state you asked for. We don't have any circular dependency with config since the unmarshaler is now internal in the service. There is still a followup as documented in the description, but that is easy and non breaking to solve.

Fixes open-telemetry#4605 Followup PRs: * Move service/internal/configunmarshaler as private struct into service/ to avoid circular dependency (no public API change) * Change service/internal/builders to private structs in service/ or don't pass config, instead only pass the components and pipeline from the config. While doing this, I realize that the "service" section in our config is a bit useless, we can think of promoting everything under "service::" one level up. Signed-off-by: Bogdan Drutu <[email protected]>

codeboten

Just one comment, otherwise the change looks good.

config/moved_service.go

…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 force-pushed the mvconfig branch 4 times, most recently from edb2a4f to b185d13 Compare December 22, 2021 18:01

bogdandrutu marked this pull request as ready for review December 22, 2021 18:02

bogdandrutu requested review from a team and codeboten December 22, 2021 18:02

Aneurysm9 mentioned this pull request Jan 5, 2022

Decouple Service from components #4605

Closed

Aneurysm9 reviewed Jan 5, 2022

View reviewed changes

service/serviceconfig/service.go Outdated Show resolved Hide resolved

bogdandrutu force-pushed the mvconfig branch 2 times, most recently from 6e6b389 to 7db60cb Compare January 18, 2022 21:09

github-actions bot added the Stale label Feb 2, 2022

bogdandrutu force-pushed the mvconfig branch from 80942db to 5d57fca Compare February 2, 2022 18:49

bogdandrutu removed the Stale label Feb 2, 2022

tigrannajaryan approved these changes Feb 14, 2022

View reviewed changes

codeboten reviewed Feb 15, 2022

View reviewed changes

bogdandrutu force-pushed the mvconfig branch from e2aed2a to 1e86f44 Compare February 16, 2022 20:25

This comment was marked as resolved.

Sign in to view

bogdandrutu marked this pull request as draft February 16, 2022 20:29

bogdandrutu force-pushed the mvconfig branch from 1e86f44 to 5c99c54 Compare February 19, 2022 18:07

bogdandrutu marked this pull request as ready for review February 19, 2022 18:08

bogdandrutu force-pushed the mvconfig branch 2 times, most recently from df52ad2 to 0c20627 Compare February 19, 2022 18:46

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

dmitryax removed the Stale label Mar 25, 2022

bogdandrutu mentioned this pull request Apr 5, 2022

Deprecate configunmarshaler package, move it to internal #5151

Merged

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

bogdandrutu removed the Stale label Apr 14, 2022

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

bogdandrutu removed the Stale label Apr 29, 2022

bogdandrutu force-pushed the mvconfig branch 2 times, most recently from 898740c to 11d708f Compare May 12, 2022 23:38

bogdandrutu changed the title ~~Move config.Config and config.Service to serviceconfig~~ Deprecate config.Config and config.Service, use service.Config* May 12, 2022

bogdandrutu force-pushed the mvconfig branch from 11d708f to ea2e23b Compare May 12, 2022 23:39

bogdandrutu force-pushed the mvconfig branch from ea2e23b to ff6fe6b Compare May 12, 2022 23:49

bogdandrutu force-pushed the mvconfig branch from ff6fe6b to 168ef6f Compare May 13, 2022 14:21

codeboten approved these changes May 13, 2022

View reviewed changes

config/moved_service.go Outdated Show resolved Hide resolved

Update moved_service.go

3bbcd72

bogdandrutu merged commit 3a97fe1 into open-telemetry:main May 13, 2022

bogdandrutu deleted the mvconfig branch May 13, 2022 22:27

fatsheep9146 mentioned this pull request Oct 31, 2022

Consider moving configauth to extension package #4996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `config.Config` and `config.Service`, use `service.Config*` #4608

Deprecate `config.Config` and `config.Service`, use `service.Config*` #4608

bogdandrutu commented Dec 21, 2021 •

edited

Loading

codecov bot commented Dec 21, 2021 •

edited

Loading

github-actions bot commented Feb 2, 2022

bogdandrutu commented Feb 14, 2022

tigrannajaryan left a comment

Aneurysm9 commented Feb 14, 2022

codeboten left a comment

bogdandrutu commented Feb 16, 2022

This comment was marked as resolved.

github-actions bot commented Apr 9, 2022

github-actions bot commented Apr 29, 2022

bogdandrutu commented May 12, 2022

codeboten left a comment

Deprecate config.Config and config.Service, use service.Config* #4608

Deprecate config.Config and config.Service, use service.Config* #4608

Conversation

bogdandrutu commented Dec 21, 2021 • edited Loading

codecov bot commented Dec 21, 2021 • edited Loading

Codecov Report

github-actions bot commented Feb 2, 2022

bogdandrutu commented Feb 14, 2022

tigrannajaryan left a comment

Choose a reason for hiding this comment

Aneurysm9 commented Feb 14, 2022

codeboten left a comment

Choose a reason for hiding this comment

bogdandrutu commented Feb 16, 2022

This comment was marked as resolved.

github-actions bot commented Apr 9, 2022

github-actions bot commented Apr 29, 2022

bogdandrutu commented May 12, 2022

codeboten left a comment

Choose a reason for hiding this comment

Deprecate `config.Config` and `config.Service`, use `service.Config*` #4608

Deprecate `config.Config` and `config.Service`, use `service.Config*` #4608

bogdandrutu commented Dec 21, 2021 •

edited

Loading

codecov bot commented Dec 21, 2021 •

edited

Loading