[otelcol] Allow passing confmap.Providers to otelcol.Collector #9228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9228 +/- ##
==========================================
+ Coverage 90.70% 90.74% +0.03%
==========================================
Files 347 347
Lines 18199 18212 +13
==========================================
+ Hits 16507 16526 +19
+ Misses 1369 1365 -4
+ Partials 323 321 -2 ☔ View full report in Codecov by Sentry. |
bogdandrutu
reviewed
Jan 5, 2024
evan-bradley
force-pushed
the
pass-confmap-providers
branch
9 times, most recently
from
8a1ed54 to
adae484
Compare
evan-bradley
force-pushed
the
pass-confmap-providers
branch
from
adae484 to
652dad4
Compare
|
This should be ready for review. cc @mx-psi |
mx-psi
reviewed
Jan 23, 2024
evan-bradley
force-pushed
the
pass-confmap-providers
branch
2 times, most recently
from
c783317 to
c63397c
Compare
mx-psi
approved these changes
Jan 25, 2024
mx-psi
approved these changes
Jan 26, 2024
|
@bogdandrutu Could you give this another look? |
This was referenced Feb 7, 2024
evan-bradley
force-pushed
the
pass-confmap-providers
branch
2 times, most recently
from
31fe3ff to
3c5146e
Compare
|
@bogdandrutu @mx-psi Any additional thoughts on this? I think this is good to go, and will pair well with #9516. |
|
@bogdandrutu will merge this tomorrow unless you raise any objections by then |
|
@evan-bradley can you fix the conflict? |
evan-bradley
force-pushed
the
pass-confmap-providers
branch
from
733c62d to
5494cfb
Compare
|
Thanks @mx-psi. Should be good now. |
evan-bradley
force-pushed
the
pass-confmap-providers
branch
from
dc39b02 to
082a2b8
Compare
djaglowski
approved these changes
Feb 16, 2024
evan-bradley
force-pushed
the
pass-confmap-providers
branch
from
082a2b8 to
32ddd03
Compare
kasia-kujawa
added a commit
to SumoLogic/sumologic-otel-collector
that referenced
this pull request
Mar 7, 2024
changes which impacts update: - httpforwarder: Remove extension named httpforwarder, use httpforwarderextension instead ref: open-telemetry/opentelemetry-collector-contrib#24171 - spanmetricsprocessor: Remove spanmetrics processor ref: open-telemetry/opentelemetry-collector-contrib#29567 - confighttp: Remove deprecated HTTPClientSettings, NewDefaultHTTPClientSettings, and CORSSettings. ref: open-telemetry/opentelemetry-collector#9625 - ConfigProvider is deprecated, use ConfigProviderSettings instead ref: open-telemetry/opentelemetry-collector#9228
kasia-kujawa
added a commit
to SumoLogic/sumologic-otel-collector
that referenced
this pull request
Mar 7, 2024
changes which impacts update: - httpforwarder: Remove extension named httpforwarder, use httpforwarderextension instead ref: open-telemetry/opentelemetry-collector-contrib#24171 - spanmetricsprocessor: Remove spanmetrics processor ref: open-telemetry/opentelemetry-collector-contrib#29567 - confighttp: Remove deprecated HTTPClientSettings, NewDefaultHTTPClientSettings, and CORSSettings. ref: open-telemetry/opentelemetry-collector#9625 - ConfigProvider is deprecated, use ConfigProviderSettings instead ref: open-telemetry/opentelemetry-collector#9228
mx-psi
pushed a commit
that referenced
this pull request
Apr 18, 2024
…ings (#9516) **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 --------- Co-authored-by: Evan Bradley <[email protected]>
Hey @evan-bradley, I just noticed this change today. We have a custom implementation of |
|
@splunkericl can you file an issue for this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
One way to work toward #4759. This implements the second approach that I've outlined here: #4759 (comment).
I think the main advantage of this approach is that it cleans up the API for the package.
otelcol.ConfigProvideris a fairly thin wrapper aroundconfmap.Resolver, so I think we could ultimately remove the interface, and any custom functionality for config merging or unmarshaling could be exposed to users through settings rather through a custom implementation.Link to tracking Issue:
Works toward #4759
Testing:
Unit tests
Documentation:
Added Godoc comments.