Fix static converters #2270
Fix static converters #2270
Conversation
| RefreshInterval: time.Duration(sdConfig.RefreshInterval), | ||
| ResourceGroup: sdConfig.ResourceGroup, | ||
| ProxyConfig: common.ToProxyConfig(sdConfig.HTTPClientConfig.ProxyConfig), | ||
| FollowRedirects: sdConfig.HTTPClientConfig.FollowRedirects, | ||
| EnableHTTP2: sdConfig.HTTPClientConfig.EnableHTTP2, | ||
| TLSConfig: *common.ToTLSConfig(&sdConfig.HTTPClientConfig.TLSConfig), | ||
| } | ||
|
|
||
| // Only one auth method is allowed. |
There was a problem hiding this comment.
If the user supplied a config with both auth methods, isn't that technically a problem with the config and not with the converters? I'm ok for this check to remain, but not sure if it's overkill to check for invalid configuration in the converter.
There was a problem hiding this comment.
I don't think that setting two auth methods is possible in static mode because you must use the authentication_method field to specify your method (default OAuth). The problem here is that no matter which auth method the user would set, the converter would always create the two auth blocks, resulting in an invalid config.
| ForwardTo: forwardTo, | ||
| AddMetricSuffixes: defaultArgs.AddMetricSuffixes, | ||
| ResourceToTelemetryConversion: defaultArgs.ResourceToTelemetryConversion, | ||
| } | ||
|
|
||
| // Override default only if > 0 because GCFrequency of 0 is not allowed |
There was a problem hiding this comment.
Same here, do we really need to check if the incoming config is invalid? Also, if we set a different GC frequency than what the user specified, we're technically not converting their exact config anymore. Maybe it'd make more sense to just refuse to convert the config, or to log a warning.
But IMO we could just convert it as it is and let Alloy fail when it loads it.
There was a problem hiding this comment.
I dug deeper into this one because I could not understand why it would be set to 0 by default and I found out that when we moved to Alloy we transformed some static components into empty shells: https://github.com/grafana/alloy/pull/55/files#diff-1b127540731f7403cbe5211de129eddfc35579d88d6bcbc371520642a9aa5486. The problem with this component is that the default values were set in the newRemoteWriteExporter() function. This StaleTime default value is actually supposed to be 15 minutes. I got rid of the check in the converter and set the correct value in the yaml component. This way, if you convert a config that uses the default yaml value, it will be correctly converted to 15m in Alloy.
| }, | ||
| EnableCommunityComps: true, | ||
| }) | ||
| err = f.LoadSource(cfg, nil, "") | ||
|
|
||
| // This is a bit of an ugly workaround but the spanmetrics connector is starting a go routine in its start function. |
There was a problem hiding this comment.
I guess this is referring to the start function of OTel Collector components? Do we call this when we call Alloy'sNew() function for a component? I think those New() functions aren't meant to have side effects such as opening files and starting goroutines. They should only do this once Run is called.
There was a problem hiding this comment.
yeah we often have the pattern that the New() function is calling the Update function and this creates some side-effects but only for the tests. Currently we start the Otel components in the Run() function and that's where the race problems appeared. This change: #2262 is making the otel components start in the New() function, which is solving the problem at runtime but it can be a bit more annoying for testing
There was a problem hiding this comment.
Thank you for pointing this out... I added a comment to the other PR. New is not supposed to have such side effects.
PR Description
#2262 I discovered a few bugs with the static converters:
As explained in the comment in testing.go, we need to run the controller to cleanup the component that are starting go routines on Start. But we can't do it for all components because some have config errors and others panic. Because these are only static converters tests, I decided to add the workaround that the controller will run only when the config contains a spanmetrics connector for now. LMK if you're ok with it or if you want to explore alternatives.
PR Checklist