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

[confmap] Add converter and provider settings to confmap.ResolverSettings #9516

Merged
merged 7 commits into from Apr 18, 2024

Conversation

evan-bradley
Copy link
Contributor

Description:

Follows #9443, relates to #9513.

This builds on #9228 to demonstrate the concept.

This shows one way of extending the otelcol APIs to allow passing converters and providers from the builder with the new settings structs for each type.

I think this approach has a few advantages:

  1. This follows our pattern of passing in "factory" functions instead of instances to the object that actually uses the instances.
  2. Makes the API more declarative: the settings specify which modules to instantiate and which settings to instantiate them with, but don't require the caller to actually do this.
  3. Compared to the current state, this allows us to update the config at different layers. A distribution's main.go file can specify the providers/converters it wants and leave the settings to be created by otelcol.Collector.

The primary drawbacks I see here are:

  1. This is a little more opinionated since you don't have access to the converter/provider instances or control how they are instantiated. I think this is acceptable and provides good encapsulation.
  2. The scheme->provider map can now only be specified by the providers' schemes, which is how it is currently done by default. I would want to hear what use cases we see for more complex control here that necessitates using schemes not specified by the providers.

cc @mx-psi

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Feb 28, 2024
@mx-psi
Copy link
Member

mx-psi commented Mar 1, 2024

@evan-bradley can you fix the merge conflict? I think that would make it ready for review :)

@evan-bradley evan-bradley force-pushed the pass-confmap-settings branch 3 times, most recently from 8e07bb3 to 77dab0c Compare March 4, 2024 03:46
@evan-bradley evan-bradley marked this pull request as ready for review March 4, 2024 03:46
@evan-bradley evan-bradley requested a review from a team as a code owner March 4, 2024 03:46
@evan-bradley
Copy link
Contributor Author

@mx-psi Should be good-to-go now, please take a look.

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.77%. Comparing base (e9b432d) to head (bd5eaa3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9516      +/-   ##
==========================================
+ Coverage   91.76%   91.77%   +0.01%     
==========================================
  Files         358      359       +1     
  Lines       16604    16631      +27     
==========================================
+ Hits        15236    15263      +27     
  Misses       1041     1041              
  Partials      327      327              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot removed the Stale label Mar 6, 2024
confmap/resolver.go Outdated Show resolved Hide resolved
confmap/resolver.go Outdated Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Mar 6, 2024

@evan-bradley Can you address the linter issue?

Error: otelcoltest/config.go:48:6: func `makeMapProvidersMap` is unused (unused)

@evan-bradley evan-bradley force-pushed the pass-confmap-settings branch 2 times, most recently from 860d0d4 to c4bdf49 Compare March 7, 2024 04:02
@evan-bradley
Copy link
Contributor Author

@mx-psi Thanks for the heads-up, I think everything should be taken care of now.

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Mar 22, 2024
@mx-psi mx-psi removed the Stale label Mar 25, 2024
@evan-bradley
Copy link
Contributor Author

@mx-psi This is ready for another look whenever you are able.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM! I agree that the factories feel a bit 'heavy', but I think this works

cc @open-telemetry/collector-approvers, will merge this tomorrow unless anyone blocks before then

confmap/converter/expandconverter/expand.go Outdated Show resolved Hide resolved
confmap/resolver.go Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Apr 18, 2024

@evan-bradley can you address the linter on here?

Error: loader.go:248:13: SA1019: fileprovider.NewWithSettings is deprecated: [v0.99.0] Use NewFactory instead. (staticcheck)
	cp, err := fileprovider.NewWithSettings(confmaptest.NewNopProviderSettings()).Retrieve(context.Background(), "file:"+filePath, nil)
	           ^

@evan-bradley
Copy link
Contributor Author

@mx-psi The build is green! 🎉

@mx-psi mx-psi merged commit 2108ae8 into open-telemetry:main Apr 18, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 18, 2024
codeboten pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request May 2, 2024
**Description:**

Follows
open-telemetry/opentelemetry-collector#9516.
This doesn't introduce any functional changes, just uses the new API.

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
evan-bradley added a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request May 3, 2024
**Description:**

Follows
open-telemetry/opentelemetry-collector#9516.
This doesn't introduce any functional changes, just uses the new API.

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Antoine Toulme <antoine@lunar-ocean.com>
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
…try#32743)

**Description:**

Follows
open-telemetry/opentelemetry-collector#9516.
This doesn't introduce any functional changes, just uses the new API.

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
**Description:**

Follows
open-telemetry/opentelemetry-collector#9516.
This doesn't introduce any functional changes, just uses the new API.

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Antoine Toulme <antoine@lunar-ocean.com>
jlg-io pushed a commit to jlg-io/opentelemetry-collector-contrib that referenced this pull request May 14, 2024
…try#32743)

**Description:**

Follows
open-telemetry/opentelemetry-collector#9516.
This doesn't introduce any functional changes, just uses the new API.

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Curtis Robert <crobert@splunk.com>
jlg-io pushed a commit to jlg-io/opentelemetry-collector-contrib that referenced this pull request May 14, 2024
**Description:**

Follows
open-telemetry/opentelemetry-collector#9516.
This doesn't introduce any functional changes, just uses the new API.

---------

Co-authored-by: Evan Bradley <evan-bradley@users.noreply.github.com>
Co-authored-by: Antoine Toulme <antoine@lunar-ocean.com>
codeboten pushed a commit that referenced this pull request May 21, 2024
dmitryax added a commit that referenced this pull request May 22, 2024
After
#9516, the
`validate` sum-command works differently from the root command.

If a downstream distribution provides `URIs` as part of the new
`ConfigProviderSettings` without `--config` flag, the `validate` command
fails with
```
2024/05/21 15:53:25 main.go:85: application run finished with error: at least one config flag must be provided
```
andrzej-stencel added a commit to andrzej-stencel/elastic-agent that referenced this pull request May 23, 2024
The `Providers` and `Converters` fields of `ResolverSettings` struct
were deprecated in [v0.99.0](https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.99.0) (open-telemetry/opentelemetry-collector#9516)
and removed in [v0.101.0](https://github.com/open-telemetry/opentelemetry-collector/releases/tag/v0.101.0).
This refactoring is required before updating to OTel v0.101.0.
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.

None yet

3 participants