continuous-test as module #7747
Conversation
pkg/continuoustest/config.go
Outdated
| ServerMetricsPort int `yaml:"server_metrics_port"` | ||
| LogLevel log.Level `yaml:"log_level"` |
There was a problem hiding this comment.
These 2 settings should be removed, because we should use the Mimir ones.
There was a problem hiding this comment.
Leaving in for the backwards compatibility of cmd/mimir-continuous-test/main.go for right now and will eventually remove. Unused in module now.
…nd not the module
| @@ -135,6 +136,7 @@ type Config struct { | |||
| MemberlistKV memberlist.KVConfig `yaml:"memberlist"` | |||
| QueryScheduler scheduler.Config `yaml:"query_scheduler"` | |||
| UsageStats usagestats.Config `yaml:"usage_stats"` | |||
| ContinuousTest continuoustest.Config `yaml:"-"` | |||
There was a problem hiding this comment.
Seems like we should add a YAML section if this is going to be a standard part of Mimir.
There was a problem hiding this comment.
I disagree as it doesn't run as a default startup piece of Mimir with target=all. It's currently usable as a standalone docker image with flags passed and doesn't take a config file and I think keeping it as close to current usage as possible is the best approach. I was including config for it originally and decided that passing config via file isn't something currently anyone has asked for so to hold off
There was a problem hiding this comment.
Other components of Mimir that have CLI flags have a corresponding YAML configuration. That makes this particular feature inconsistent.
There was a problem hiding this comment.
I agree re consistency. Adding YAML support will also expose all the continuous-test flags in reference documentation, which would be quite ugly, but now that continuous-test is part of Mimir, I think we should do it.
pkg/mimir/modules.go
Outdated
| @@ -1034,6 +1037,23 @@ func (t *Mimir) initUsageStats() (services.Service, error) { | |||
| return t.UsageStatsReporter, nil | |||
| } | |||
|
|
|||
| func (t *Mimir) initContinuousTest() (services.Service, error) { | |||
| start := func(ctx context.Context) error { | |||
There was a problem hiding this comment.
Doesn't seem necessary to create a Better idea belowstart method here for the service. Should be fine to just execute this as regular code here and pass nil for the start method
pkg/mimir/modules.go
Outdated
| return nil | ||
| } | ||
|
|
||
| return services.NewBasicService(start, func(ctx context.Context) error { |
There was a problem hiding this comment.
Usually the way you'd use the service stuff is to have continoustest.Manager implement the service.Service interface and return an instance of it from this method. Then the module system handles calling start and run methods.
There was a problem hiding this comment.
True! That was the original design. I don't know how I got here from there, but happy to go back to it as that's how the other modules behave.
There was a problem hiding this comment.
Nope! Nevermind. I just converted it and got the errors I was getting beforehand when using the Manager as the service. Basically yes, that is the pattern we use in many other modules but they are meant to be continuously running with different behaviors for stopping etc. whereas if you look at ActivityTracker as an example for NewIdleService, it's worth it to use that dskit infrastructure instead of writing a new AwaitRunning func etc for it to behave properly. All I want is to init, and then be able to call Run and wrapping in BasicService does that here cleanly I think. I do agree with your previous comment for cleanly running it all as init, and then just pass it the Run func.
…rvice" This reverts commit 578d527.
What this PR does
Puts continuous-test into a mimir module so we can run
mimir -target=continuous-testChecklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features. n/a