[target allocator] Make scrape interval configurable #1923
Conversation
145e4cc to
5128c0d
Compare
cmd/otel-allocator/config/config.go
Outdated
| @@ -96,6 +99,12 @@ func unmarshal(cfg *Config, configFile string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func createDefaultConfig() Config { | |||
| return Config{ | |||
| CRScrapeInterval: DefaultCRScrapeInterval, | |||
There was a problem hiding this comment.
Are there any other defaults that would be good to have here?
There was a problem hiding this comment.
i.e. would it be better for us to just embed something like CommonPrometheusFields in the config?
There was a problem hiding this comment.
Having had a quick look through that (https://github.com/prometheus-operator/prometheus-operator/blob/main/Documentation/api.md#commonprometheusfields), there's way too many that wouldn't apply to TA, or which would apply, but we'd rather set them once at the Collector level and inherit. So I think we'll need to cherry pick the ones we want, unfortunately.
There was a problem hiding this comment.
Ah, you mean just for TA config, not for the spec in the CR? I guess we could do that. It'd still be a bit messy to have settable values that don't do anything, but if they're just exposed in the TA config and not the CR, it's less of a problem.
There was a problem hiding this comment.
holy crap there are a LOT of fields in there. I think it's okay to piece meal it on second thought.
There was a problem hiding this comment.
I can open an issue to decide what we want from there, exactly. Should that block this change, though?
There was a problem hiding this comment.
An issue would be great... and nope, definitely not blocking!
71e9141 to
cd0fe81
Compare
cmd/otel-allocator/config/config.go
Outdated
|
|
||
| type Config struct { | ||
| LabelSelector map[string]string `yaml:"label_selector,omitempty"` | ||
| Config *promconfig.Config `yaml:"config"` | ||
| AllocationStrategy *string `yaml:"allocation_strategy,omitempty"` | ||
| FilterStrategy *string `yaml:"filter_strategy,omitempty"` | ||
| CRScrapeInterval model.Duration `yaml:"cr_scrape_interval,omitempty"` |
There was a problem hiding this comment.
I thought some more about this over the weekend, and I think this may be better if we make an embedded CR config resource and that's where we'll be able to then move the incorrect Pod and Service monitor selectors to. Does that seem reasonable to you? (this issue)
There was a problem hiding this comment.
Sure, what should we name it? I'm ok with PrometheusCR for consistency, but no strong opinions. Also, should we allow enabling the functionality via the config file (currently it can only be done via a CLI flag)?
Also, do you want to move the current selector attributes, as opposed to creating new ones with the right semantics?
There was a problem hiding this comment.
PrometheusCR makes sense. And I'm not sure I have an opinion on where the enabling should happen... i trust your judgement. And let's keep the current selector attributes where they are now, and then in a follow up make the new ones in the PrometheusCR, deprecating the current ones.
There was a problem hiding this comment.
Done. I left the CLIConf alone for now, it's not entirely clear what the logic should be there, and I'd rather do it in a separate PR without these other changes in the way.
cd0fe81 to
61fa119
Compare
|
FYI, the E2E upgrade test passes on my machine ™️ . |
…1923) * [target allocator] Make scrape interval configurable * Put PrometheusCR config for TA in a separate structure
Resolves #1925.
I don't entirely like the fact that this only affects Prometheus CRs, but this is part of an ongoing problem where some values can logically be set both on the Collector CR and in prometheus receiver configuration.
I've tested this end-to-end and it does work.