Instrument cortex tenant #35
Conversation
processor.go
Outdated
|
|
||
| var r result | ||
| r.code, r.body, r.err = p.send(clientIP, reqID, tenant, wrReq) | ||
| r.code, r.body, r.duration, r.err = p.send(clientIP, reqID, tenant, wrReq) |
There was a problem hiding this comment.
maybe we should switch to just return result struct here? too many fields returned already :)
processor.go
Outdated
| return | ||
| } | ||
|
|
||
| duration = float64(time.Since(start).Milliseconds()) |
There was a problem hiding this comment.
I guess since we anyway are using float64 then it's better to store seconds there, not milliseconds.
There was a problem hiding this comment.
This was made milliseconds because it's more likely for this value to be pretty small... I ripped this metric and buckets from the tool Istio
|
Thanks! I've left a few small comments and have a bigger one - maybe we should also add |
| }() | ||
| } | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
There's already cfg.ListenPprof -> maybe we can use that instead of another hardcoded HTTP port? Maybe we can rename cfg.ListenPprof to something more general, like ListenAux or whatever. Or add additional config variable ListenMetrics and use that (with same default value of :9090)
We can offer it, but may I suggest making it configurable? Some folks may have lots of tenants which would cause a lot of metric cardinality |
|
Yeah, probably makes sense, it's safer to either make it configurable or put some limit on the number of tenant metrics. |
|
@blind-oracle ready for another review |
config.go
Outdated
| Listen string `env:"CT_LISTEN"` | ||
| ListenPprof string `yaml:"listen_pprof" env:"CT_LISTEN_PPROF"` | ||
| ListenMetricsAddress string `yaml:"listen_metrics_address" env: "CT_LISTEN_METRICS_ADDRESS"` | ||
| ListenMetricsIncludeTenant bool `yaml:"listen_metrics_include_tenant" env: "CT_LISTEN_METRICS_INCLUDE_TENANT"` |
There was a problem hiding this comment.
Let's maybe rename it to smth like MetricsIncludeTenantLabel since it really has nothing to do with listening?
|
Super, let's merge it as is, if something comes up we can improve later. Thanks! |
|
@blind-oracle would you mind "releasing" (ie publishing the image) so I can use these changes? Thanks! |
|
Done, I need to setup some workflow for that... 🤔 |
2 participants
Adding RED metrics to cortex-tenant.