Split up the single Cortex binary into cortex-{distributor, ingester, querier, ruler, table-manager} #218
Conversation
tomwilkie
force-pushed
the
151-split-it-up
branch
4 times, most recently
from
c22f249 to
327cd5a
Compare
tomwilkie
changed the title
tomwilkie
force-pushed
the
151-split-it-up
branch
5 times, most recently
from
bfc04dd to
176bc38
Compare
tomwilkie
changed the title
tomwilkie
force-pushed
the
151-split-it-up
branch
from
c85b754 to
c8e6d2c
Compare
| var ( | ||
| serverConfig = server.Config{} | ||
| ingesterRegistrationConfig = ring.IngesterRegistrationConfig{ | ||
| ListenPort: &serverConfig.GRPCListenPort, // IngesterRegistrator need to know our gRPC listen port |
|
|
||
| func main() { | ||
| var ( | ||
| serverConfig = server.Config{} |
There was a problem hiding this comment.
The var serverConfig = server.Config{} style here and below is just a bit unusual. More usual would be either var serverConfig server.Config or serverConfig := server.Config{} (not a mix of var and explicit assignment of zero value).
There was a problem hiding this comment.
Fair point, I'll make it this:
var (
serverConfig server.Config
...
)
| "github.com/prometheus/common/model" | ||
| ) | ||
|
|
||
| // Registerer is a think that can RegisterFlags |
|
👍 |
|
Btw., do we need the |
I'm using it to generate the binary and image name, and image name definitely needs it. I can include that as a constant in the Makefile though. Let me give that a go. |
|
👍 |
- Push flag definitions onto the respective config objects - Separate binaries for each component - Add a new component: cortex_querier (currently a dressed up distributor) - New server package for common server logic - Makefile magic to decide what services to build - Nest the consul client in the ring, and the ring in the ingester registerer, as they share config. - Move prometheus.MustRegister into each component to reduce repetition - Remove ingester http interface - Move ring prometheus metrics to package-level, so we can instantiate the IngesterRegistration more than once.
tomwilkie
force-pushed
the
151-split-it-up
branch
from
ae0e822 to
82e7dfe
Compare
|
Great stuff. In person yesterday, you mentioned a pattern for constructors for components. Could you please jot that down in text somewhere in the repo? |
|
I'll take that as a no. |
| @@ -165,6 +174,7 @@ func New(cfg Config, chunkStore cortex_chunk.Store) (*Ingester, error) { | |||
| Help: "The total number of samples returned from queries.", | |||
| }), | |||
| } | |||
| prometheus.MustRegister(i) | |||
There was a problem hiding this comment.
@tomwilkie Registering Prometheus metrics from a constructor is problematic because it makes it impossible to instantiate multiple objects of the same type (the Prometheus client library will panic). I'm hitting this with ingester tests right now. While there's a non-panicking Register method, neither returning its error nor letting it silently fail seem like the right choice.
I would propose either moving this out of the constructor again or making the metrics global. Which one do you prefer?
There was a problem hiding this comment.
I'm hitting this with ingester tests right now
Have you written some new tests? Wondering why I didn't hit it.
I would propose either moving this out of the constructor again or making the metrics global. Which one do you prefer?
Global please. For consistency sake, I'd like everything to be global - I'm not concerned about the namespace pollution, and going global results in less code, which is one of the things I care most about.
There was a problem hiding this comment.
Have you written some new tests? Wondering why I didn't hit it.
Yeah, I hit this when writing a second ingester test for the series limiting.
Global please.
Ok, will get to it next week.
FWIW, with having metrics globally, the concern is less about global variable namespace pollution, but about anyone wanting to reuse a package getting globally registered metrics forced on them. As long as we don't care about reuse, that's fine.
Fixes #194, fixes #151