Notify extensions of the Collector's effective configuration

[evan-bradley]

Description:

This adds an optional ConfigWatcher interface that extensions can implement if they want to be notified of the effective configuration that is used by the Collector.

I don't feel very strongly about any of the decisions I made in this PR, so I am open to input if we would like to take a different approach anywhere. I will leave some comments to explain the decisions I made.

Link to tracking Issue:
Closes #6596

Testing:

I've made minimal unit test changes, but I expect to write more tests once there is consensus on the direction for implementing this functionality. I have done some manual testing to show that an extension can get a YAML representation of the effective config using two YAML input files.

[evan-bradley]

cc @tigrannajaryan @portertech

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.17 ⚠️

Comparison is base (88e260c) 90.74% compared to head (c0e16b1) 90.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6833      +/-   ##
==========================================
- Coverage   90.74%   90.57%   -0.17%     
==========================================
  Files         300      301       +1     
  Lines       15164    15347     +183     
==========================================
+ Hits        13760    13900     +140     
- Misses       1123     1161      +38     
- Partials      281      286       +5     

Impacted Files	Coverage Δ
extension/extension.go	97.61% <ø> (ø)
otelcol/collector.go	80.26% <100.00%> (+1.53%)	⬆️
otelcol/configprovider.go	87.75% <100.00%> (+1.70%)	⬆️
service/extensions/extensions.go	79.78% <100.00%> (+1.88%)	⬆️
service/service.go	76.78% <100.00%> (+0.71%)	⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[github-actions]

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

[evan-bradley]

@bogdandrutu This should be ready for another look.

[bogdandrutu]

Not ok, this is not parsed from the config, you break the design here since everything else is parsed from the config.

[evan-bradley]

I agree, it's not ideal. I've pushed a commit that makes this field a method to separate it from the public fields on this struct. Let me know what you think.

If we don't want the source confmap.Conf struct available from Config, I see two possibilities:

• We could change the ConfigProvider.Get method to also return *confmap.Conf. To me, this muddies the method signature a little bit, but would provide a clear separation between the two types.
• We could add a new method to ConfigProvider to only resolve and return a confmap.Conf struct. I opted against this since we are already resolving the config in Get and I think another method would be unnecessary on the ConfigProvider interface, but this would also allow clearly separating confmap.Conf and Config.

Let me know if there's another option I've missed.

[evan-bradley]

@bogdandrutu I'm sorry, I think I misunderstood what you meant.

If we are looking to have both Config and ConfigProvider be agnostic of the underlying configuration structure, then I see a few options:

• Provide an optional interface for config providers to implement that resolves the configuration and translates it (if necessary) to a confmap.Conf object. This seems like it would probably be the most straightforward solution.
• Provide another type that can be marshaled into YAML from either the Config or ConfigProvider types that is used in extension.ConfigWatcher, in place of confmap.Conf.
• Provide a way to translate Config instances back into confmap.Conf.

What do you think?

[evan-bradley]

@bogdandrutu I've pushed a commit including a rough draft for an optional interface to provide confmap.Conf objects from a Config Provider. Let me know what you think.

[github-actions]

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

[tigrannajaryan]

@bogdandrutu PTAL.

[evan-bradley]

@dmitryax Your review would be greatly appreciated to get this closer to the finish line.

I will try to summarize the discussion so far, I'd welcome your input on these points.

Getting the config

The extension needs a serialized version of the Collector's effective configuration, which we determined is best provided as a confmap.Conf object. Since there's currently no way to go from an unmarshaled config (otelcol.Config) to a marshaled version, I've created otelcol.ConfmapProvider, an optional interface for otelcol.ConfigProvider structs to provide the unmarshaled config.

Communicating the config

We want to provide an optional interface to facilitate communicating the effective config from the Host to Extensions. The communication flow will look slightly differently depending on what interface we provide.

We can provide an interface to be implemented by the Host, e.g. with a GetConfig method, and have Extensions "pull" the configuration. I see the following benefits to this approach:

1. The extension can call this in its Start method, and obtaining the config is synchronous. This isn't particularly helpful in the context of OpAMP, but may be useful to other extensions.
2. Extensions can query the Host for the interface to determine whether the Host supports providing config. It's unclear whether this would be useful.
3. Other component classes could use the Host interface. It's not clear to me this is desirable.

We can also provide an interface for Extensions, which the Host will query for and "push" the config to the Extensions. This is the approach currently implemented in this PR. I think this approach has the following benefits:

1. If a confmap.Provider that dynamically updates the config were used, it would be feasible for Extensions to persist across config reloads and for the Host to notify them of configuration updates. This would add some complexity and isn't part of this change, but would be possible later on or for custom distributions to implement.
2. Restricts config notifications to Extensions.
3. Follows a similar pattern to the PipelineWatcher interface.

Overall providing an interface for extensions opens the possibilities for additional functionality in the future while not being much more complex. However, I think both approaches would work fine and the differences between them aren't substantial right now.

Bogdan is out for a while. Removing the block since I believe his comments were addressed.

[djaglowski]

It seems the only place we use it is in this package. Is it intended to have other implementations?

[djaglowski]

I also noticed that we already have another interface called confmap.Provider, which seems odd.

[evan-bradley]

The goal of the interface was to not require structs implementing ConfigProvider to have any dependencies on confmap.Conf. As for the name, I agree that it is a bit unfortunate. I am open to suggestions here, my intent was to signify that otelcol.ConfigProvider provides Config structs while otelcol.ConfmapProvider provides confmap.Conf structs.

[tigrannajaryan]

The goal of the interface was to not require structs implementing ConfigProvider to have any dependencies on confmap.Conf.

I am probably missing something, but isn't configProvider struct in this same file the only implementation of ConfigProvider? And the configProvider struct in this same PR adds implementation of GetConfmap() func to configProvider struct , so it does take dependency confmap.Conf, right?

Are there are other implementations of ConfigProvider elsewhere which will not implement GetConfmap() func?

Which "structs implementing ConfigProvider" do you refer to?

[evan-bradley]

The current design is in response to this feedback: #6833 (comment). I realize the immediate comment is only referring to putting confmap.Conf as a field on Config, but the deeper implication to me seemed to be that ConfigProvider shouldn't depend on any underlying data structure. It's possible I've misinterpreted the feedback.

I'll try to answer your questions by explaining my understanding of the situation and how my approach fits in.

As far as I'm aware, configProvider is the only implementation of ConfigProvider in any upstream Collector repository. Furthermore, it is only currently used inside the package where it is defined. However, the existence of ConfigProvider as a public interface implies that anyone who wants to reimplement this part of the Collector can provide their own implementation. At present, configProvider implements ConfigProvider using confmap.Conf as its underlying format, but ConfigProvider doesn't specify any usage of confmap.Conf. Anyone who reimplements this part of the Collector could pass otelcol.NewCollector a ConfigProvider that knows nothing about confmap.Conf.

This works, but looking at it again now that I've spent a lot more time in this part of the code, I see a few things I don't like about this approach:

1. It's not clear why someone would want to avoid confmap.Conf when it is used across the rest of the Collector.
2. Interacting with confmap.Conf is required for the rest of the functionality in this PR, so realistically it's necessary regardless.
3. As pointed out here and here, the configuration represented by Config and confmap.Conf may not be in sync.

I would still argue that my original approach of providing Config.ToConfmap is probably the best approach. I think it makes the most sense to marshal data directly from the data itself, and the best way to do that here would be to store the marshaled representation on Config. confmap.Conf offers this with NewFromStringMap and Conf.ToStringMap, so there is also precedence for this design.

[tigrannajaryan]

It's not clear why someone would want to avoid confmap.Conf when it is used across the rest of the Collector.

+1. I don't understand what problem the avoidance solves.

[dmitryax]

I'll make another thorough review tomorrow

[dmitryax]

We can provide an interface to be implemented by the Host, e.g. with a GetConfig method, and have Extensions "pull" the configuration.

@evan-bradley have you considered an optional host interface with a function taking a callback that extensions can provide to be notified? With that, we should get the "push" model

[evan-bradley]

@evan-bradley have you considered an optional host interface with a function taking a callback that extensions can provide to be notified? With that, we should get the "push" model

I did think about this a little, I should have called this out as an option when summarizing possible approaches. For the most part this is functionally equivalent to an interface for extensions. Where I think it would get more complex is in the case where a Collector distro has a custom service that supports some variation of hot reloading, we would need to provide a mechanism for registering and unregistering callbacks. I've looked at #6550 and #6560 for ideas on how the two might compare. Overall, I think the interface for extensions ends up feeling conceptually simpler since it's just a method call and doesn't require any additional structures (callback lists, message buses, etc.).

[dmitryax]

Where I think it would get more complex is in the case where a Collector distro has a custom service that supports some variation of hot reloading, we would need to provide a mechanism for registering and unregistering callbacks

Why would we need unregistering of callbacks?

Overall, I think the interface for extensions ends up feeling conceptually simpler since it's just a method call and doesn't require any additional structures (callback lists, message buses, etc.).

With the callback, I think we solve the problem pointed out in #6833 (comment). We won't need ConfmapProvider and ConfigWatcher interfaces in that case, just another host interface that can look something like this:

type ConfmapNotifier interface {
	Subscribe(func(context.Context) (*confmap.Conf, error)) error
}

What other additional structures do you think we need? Maybe I miss something.

[evan-bradley]

I apologize in advance for the wall of text, but unfortunately these issues are really about the details.

Why would we need unregistering of callbacks?

If someone were to implement a service that allows reloading an extension only when its configuration changes, they would need to unregister callbacks for extensions that will be restarted. Thinking on it more though, the Host could handle this itself, so I think we are good here.

We won't need ConfmapProvider and ConfigWatcher interfaces in that case, just another host interface that can look something like this:

I would expect the interface to look something more like the following, with confmap.Conf as a parameter instead of a return value in the callback function. Is this correct, or am I missing how the interface in your comment would work?

type ConfmapNotifier interface {
	Subscribe(func(context.Context, *confmap.Conf) error) error
}

What other additional structures do you think we need?

It's not a big deal, but with an interface for extensions, we just loop through the service's set of extensions and query for the interface. With callbacks, we have to keep a list/map of callbacks and maintain it as extensions subscribe, and if a custom Service implements it, as they are hot reloaded.

We won't need ConfmapProvider and ConfigWatcher interfaces in that case, just another host interface that can look something like this:

I see how it would replace ConfigWatcher, but it's not clear to me that we could replace ConfmapProvider with a Host interface. Could you detail that a little more?

We need to pull the config out of ConfigProvider, then have the Collector pass it to the Service. ConfmapProvider is one way, but there are two other possible options I see to resolve that:

1. Just return it with ConfigProvider.Get. This feels a little strange to me, but would work.
2. Implement Config.ToConfmap. I had originally implemented it this way, but revised my approach to address this feedback: Notify extensions of the Collector's effective configuration #6833 (comment).

We could also use a callback here, but I feel like we shouldn't need to since ConfigProvider has a Watch method.

As for #6833 (comment), the core of the issue as I see it is passing two representations of the config across public API boundaries, I only see two solutions here:

1. Pass a single object from ConfigProvider to service.New so that the service gets all of its config from a single source. This would probably be otelcol.Config. I'm a little hesitant about this since it would confuse the function signature in my opinion, as fields from otelcol.Config are passed into two arguments in service.New.
2. Give the service its own ConfigProvider, which I played around with here: Notify extensions of the Collector's effective configuration #6833 (comment).

[github-actions]

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

[dmitryax]

I think this is a good solution. I played with it and didn't find any better way to expose this functionality.

[dmitryax]

@djaglowski, please take if it's good to go from your perspective