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

Recursive unmarshaling of components #8940

Open
djaglowski opened this issue Nov 16, 2023 · 2 comments
Open

Recursive unmarshaling of components #8940

djaglowski opened this issue Nov 16, 2023 · 2 comments

Comments

@djaglowski
Copy link
Member

djaglowski commented Nov 16, 2023

Technically, we could do steps 1-3 again for only these components but it looks like a substantial overhaul to separate this functionality from the unmarshaler.

That was more or less the approach I had in mind. I agree it would probably require a substantial overhaul, but I thought about it some more after reading your summary and I still think it is possibly worth the effort. I've attempted to draft a solution that would solve all our issues with minimal downsides, but let me know if there's some fatal flaw that I've missed.

I think the problems with implementing templating within our current unmarshaling facilities are that they expect a one-to-one mapping between component configs and factories, and that all component unmarshaling is done within a section of the config instead of in the scope of the whole config.

We would probably still want to take advantage of configunmarshaler.Configs to unmarshal individual components, but would want to process the whole config after that to do template expansion. I would see a "config processor" being something similar to a converter that implements the following interface. A list of these would be registered with otelcol.ConfigProvider and would be used during config unmarshaling.

type ConfigProcessor interface {
	// Prefix for configs like "template/"
	Prefix() string

	// Config type for component configs that need
	// processing like `template/my_receiver`
	ComponentConfig() any

	// Key and type for the processor's config (e.g. "templates" and `templatesConfig`)
	ProcessorConfig() (string, any)

	// Handles configs starting with the processor's prefix
	// given the available factories and its config.
	// All instances of the ComponentConfig type should be
	// replaced by component.Config when this function returns.
	Process(pc *PartialConfig, factories Factories, config any) error
}

We could then split config unmarshaling into stages:

First, unmarshal the config as much as possible, considering during unmarshaling that there may be some keys that need further processing. So we could unmarshal components, but any config keys prepended with template/ would need to be handled by a processor. I think this first step would produce a struct like:

type PartialConfig struct {
	Receivers: map[string]any // Factory or types returned by ConfigProcessor.ComponentConfig()
	[...]
	Service: PartialService // Some type that enables accessing/creating pipelines for the case of templating 
}

We would then run the Process method of each ConfigProcessor on the PartialConfig, with the expectation that after all processors have run, we're left with map[string]component.Config for each component class. I think the template converter would run in a similar fashion to how it runs now with this: it would remove the component template from the config, then add all components and add them to the pipelines.

I didn't specify a mechanism here, but at least for templates we would also want to provide a common function to take a confmap.Conf and return a component.Config. Component config unmarshaling would still happen at two different steps, but would at least be done consistently.

I think this design would also provide a straightforward path for open-telemetry/opentelemetry-collector-contrib#18509, which would probably work similar to templates but be slightly simpler.

I see a few downsides to this route:

  1. We currently only have two use-cases in mind, which I'm not sure is enough to say that this design will serve all our needs going forward or will be used enough to justify the additional complexity.
  2. Overall I think this will be cleaner and for templating will enable things like checking whether the forward connector is available. However, at least for our current use-cases, this doesn't enable anything that we can't already do with a Converter.
  3. We would need to meddle with service.Config (or produce it from another struct), for which the whole point is to be directly unmarshaled from a confmap.Conf. We'd likely have to refactor something here.

Originally posted by @evan-bradley in #8507 (comment)

@djaglowski
Copy link
Member Author

This is one possible approach for enabling the notion of templated components, as described in #8372.

The general idea here is that templated components may contain multiple "subcomponents". We can handle this in two ways:

  1. Run these subcomponents as a service which is internal to a receiver (as proposed in New component: Template Receiver opentelemetry-collector-contrib#26312).
  2. "Expand" the top-level component graph by substituting the subcomponents in place of the templated component. (Again, see Template provider #8372)

The first approach has a number of problems, described in detail here.

The second approach is currently possible, but the implementation (see #8507) is somewhat messy because we are forced to reason about the structure of otelcol.Config _but must actually perform all work on a raw confmap.Conf.

This issue represents a potential path forward based on the goal of obtaining a proper otelcol.Config before attempting to substitute subcomponents into the component graph.

@evan-bradley
Copy link
Contributor

I have a working prototype for this, but there are still some rough edges. I hope to have a PR that we can use as a basis for further discussion sometime soon.

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

No branches or pull requests

2 participants