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

[otelcol] update to use confmap unmarshal validation instead of invoking top-level Validate function manually. #12058

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

TylerHelmuth
Copy link
Member

Description

Mockup of what it could look like to use confmap validation in otelcol.

Link to tracking issue

Related to #11524

Testing

Would need actual unit test updates

@evan-bradley
Copy link
Contributor

Just giving this a quick pass, I like that we hoisted the calls to validate the config out of the (otelcol.Config).Validate function. Overall this seems like an improvement, even if only because it removes responsibility from otelcol.

Given the PoC, what do you think about this change?

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Jan 9, 2025

Given the PoC, what do you think about this change?

I like utilizing Validate on configSettings more than trying to unmarshal directly into otelcol.Config, but while I was implementing this I realized that it would be a lot safer/wholistic if validation was enabled by default and there was an option to disable it.

I ran into an issue in the unmarshalHookFunction where I could not specify all custom unmarshal implementations to use confmap.WithInvokeValidate. The result was that the configSettings fields like Recievers that have a custom unmarshal implementation needed to be manually updated to use confmap.WithInvokeValidate.

This update allows the top-level unmarshalling to validate correctly, which means that component unmarshal implementations don't need to use confmap.WithInvokeValidate (otlpreceiver custom unmarshaller calls the generic Unmarshal without the validate option and it triggers the Validate function), but it feels risky. It feels like there are other opportunities for holes.

The other thing I don't care for is we can no longer differentiate between errors retrieving the configuration and validating the retrieved config. When this was separate steps before we knew which step was erroring - now in order to differentiate we'd need special error types.

@evan-bradley
Copy link
Contributor

It sounds like most of the downsides result from merging the unmarshaling and validation steps. What if instead of doing validation in confmap.Unmarshal we made it a separate step and created a new confmap.Validate function?

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Jan 13, 2025

What if instead of doing validation in confmap.Unmarshal we made it a separate step and created a new confmap.Validate function?

We could move the ValidateConfig function to confmap, but it would still get invoked in otelcol. I don't view see that as any better than invoking the existing component.ValidateConfig. I'm still trending towards leaving things as they are.

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.

2 participants