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] Pass ConverterSettings and ProviderSettings to converters and providers #9443

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jan 31, 2024

Description:

For both #5615 and #9162 we need to be able to log during the confmap resolution.

My proposed solution is to pass a *zap.Logger to converters and providers during initialization. These components can then use this to log any warnings they need. This PR does the first step: being able to pass anything to converters and providers during initialization.

The obvious alternative to this is to change the interface of confmap.Provider and confmap.Converter to pass any warnings in an explicit struct. I think the *zap.Logger alternative is more natural for developers of providers and converters: you just use a logger like everywhere else in the Collector.

One problem for the Collector usage of confmap is: How does one pass a *zap.Logger before knowing how a *zap.Logger should be configured? I think we can work around this by:

  1. Passing a special 'deferred' Logger that just stores the warnings without actually logging them (we can use something like zaptest/observer for this)
  2. Resolving configuration
  3. Building a *zap.Logger with said configuration
  4. Logging the entries stored in (1) with the logger from (3) (using zaptest/observer we can do that by taking the zapcore.Core out of the logger and manually writing)

We don't actually need ProviderSettings today, just ConverterSettings, but I think it can still be useful.

Link to tracking Issue: Relates to #5615 and #9162

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f37e376) 90.24% compared to head (5f87038) 90.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9443      +/-   ##
==========================================
+ Coverage   90.24%   90.27%   +0.02%     
==========================================
  Files         344      344              
  Lines       17932    17955      +23     
==========================================
+ Hits        16182    16208      +26     
+ Misses       1421     1419       -2     
+ Partials      329      328       -1     

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

@mx-psi mx-psi requested a review from bogdandrutu January 31, 2024 15:28
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Looks good to me. I like this approach versus the alternative of adding parameters/return values to the interface method signatures. If we still want to use a type we make ourselves instead of zap.Logger, we could pass that in as a field in the Settings struct.

There may be some considerations we will need to take into account with the logger (how do we work around constructing a zap.Logger from the config if the config fails to resolve?) but those can be addressed later.

confmap/provider.go Show resolved Hide resolved
@bogdandrutu
Copy link
Member

We don't actually need ProviderSettings today, just ConverterSettings, but I think it can still be useful.

At one point I was very close to remove EnvConvertor (for some personal reasons I dropped that effort to not have to deal with more issues). Is that an alternative to both issues?

  • We have expanding available (and recursive) in the confmap resolver which supports almost all cases of the EnvConvertor.

@bogdandrutu bogdandrutu merged commit 11d8d52 into open-telemetry:main Feb 1, 2024
32 checks passed
dmitryax pushed a commit that referenced this pull request Mar 7, 2024
**Description:**
Follow up to #9443 - deleting the deprecated `New` methods on providers.
mx-psi added a commit that referenced this pull request Mar 26, 2024
**Description:** As requested by @mx-psi , added a no-op log for when
variables using the deprecated $VAR style are used. The logger should be
replaced once it is clear how to pass it down (see #9443). Also, from my
testing, the function passed to os.Expand is in fact only run when we
have $VAR and not for ${env:VAR}, so I did not add additional checking.

**Link to tracking Issue:** #9162 

**Testing:** I am not sure how to go about testing it, since we are not
passing a logger in yet, there is no easy way to know what is being
logged or what the map looks like. Some ideas on this would be
appreciated

---------

Co-authored-by: Pablo Baeyens <[email protected]>
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]>
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.

3 participants