Generic KV Client Instrumentation #2648

jtlisi · 2020-05-28T18:32:48Z

What this PR does:

Add a name and backend type label to the prometheus metrics that wrap KV clients
Remove dedicated instrumentation for the consul client in favor of a generic instrumentation for all clients

Which issue(s) this PR fixes:
Fixes #2484

Checklist

Tests updated
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pstibrany

I'm fine with having generic metrics client only, although I've found ability to see individual Consul "CAS" calls useful in the past.

After some recent changes related to Cortex/Thanos metrics, I now think that passing name to individual components that is only used for ConstLabels is an anti-pattern:

It is giving component more responsibility than it needs
Name is useless is registrerer is nil
Name is not used by sub-components (eg. if Consul client itself wanted to register its own metrics, that would create conflict)

Better way of handling multiple components with same metrics is to wrap registrerer to add constant label to all metrics registered by component: prometheus.WrapRegistererWith(prometheus.Labels{"name": "component"}, reg) (if reg is not nil. extprom.WrapRegistererWith() from Thanos handles nil case as well), and pass such wrapped registrer to component. This solves all problems mentioned above.

/cc @pracucci to hear your opinion on this as well

pstibrany · 2020-05-29T11:54:39Z

pkg/ring/kv/metrics.go

Let's make sure to do that before merging please.

Agree. This TODO should be removed (and the default register never used).

pstibrany · 2020-05-29T12:02:48Z

pkg/ring/kv/client.go

I think passing name to components that is used just for metrics registration is an anti-pattern. Caller of this call can easily wrap reg into prometheus.WrapRegistererWith(prometheus.Labels{"name": "some name here"}, reg) if it wishes to do so. I would prefer to use this pattern instead. KV store client doesn't need a name for anything else, and it also doesn't need it if registrerer is nil.

pkg/ring/kv/client.go

pstibrany · 2020-05-29T12:05:27Z

pkg/ring/lifecycler.go

This is a great opportunity to pass registrerer to lifecycler and move lifecycler metrics away from global variables.

But maybe in another PR?

I will pass the Registerer but keep the lifecycler metrics global for now.

pkg/distributor/ha_tracker_test.go

pstibrany · 2020-05-29T12:09:34Z

pkg/ring/kv/consul/client.go

Previously we could see individual CAS calls to consul. Now we will only see "CAS loop".

pracucci · 2020-05-29T13:05:38Z

Better way of handling multiple components with same metrics is to wrap registrerer to add constant label to all metrics registered by component

Agree. It's more flexible.

pracucci

Good job @jtlisi! I left few comments as well.

pkg/distributor/distributor.go

pkg/distributor/ha_tracker.go

pracucci · 2020-05-29T13:10:41Z

pkg/ring/kv/metrics.go

Agree. This TODO should be removed (and the default register never used).

pracucci · 2020-05-29T13:14:37Z

pkg/querier/blocks_store_queryable.go

The generated name here looks a bit weird to me. Maybe querier-store-gateway may be more clear?

Sounds good, I wasn't sure how best to name some of these clients.

pracucci · 2020-05-29T13:20:58Z

pkg/ring/kv/consul/client.go

Now that I notice, I think previous version was even wrong. We did track "Put" twice for 1 Put call. Once here, and once in consul/metrics.go. Am I missing anything?

After investigating further, the consul metrics probably should remain as is. They provide lower level metrics for the client interacting with consul itself: https://github.com/hashicorp/consul/blob/master/api/kv.go

In our KV implementation CAS is retried multiple times and by using a generic metrics client we are missing out on instrumenting some of these calls. One option that may be used to solve this problem is moving retries to its own KV implementation and wrap clients with it.

I am going to re-add the separate consul instrumentation and will add an issue to add a generic retry implementation for the KV store interface. WDYT?

pracucci · 2020-05-29T13:22:19Z

pkg/ring/kv/metrics.go

Please mention this change in the CHANGELOG. You should also check if it has an impact on the cortex mixin's dashboards (depends on how we group series, I haven't checked).

pracucci · 2020-05-29T13:23:17Z

pkg/ring/lifecycler.go

But maybe in another PR?

pracucci · 2020-05-29T13:24:20Z

pkg/ring/kv/consul/metrics.go

Please remember to mention in the CHANGELOG that the metric cortex_consul_request_duration_seconds has been removed.

I re-added this metric since it fits a different use case.

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi · 2020-06-01T20:12:58Z

@pstibrany & @pracucci

I updated this PR per your comments. Changes include:

Keeping the cortex_consul_request_duration_seconds metric and consul instrumentation w/ note about a possible way of removing it in a future PR
Registerers are now plumbed through to each call of the kv.NewClient
Updated ChangeLog

I will test this PR w/ the cortex-mixin this week and ensure it operates correctly with the new metrics.

If you have anymore feedback about the naming of KV clients let me know. I wasn't sure if some of the names/suffixes I chose made sense.

pstibrany

I have couple of nits, but lgtm overall. Thanks!

pstibrany · 2020-06-02T14:09:20Z

pkg/distributor/distributor_test.go

 						UpdateTimeout:   100 * time.Millisecond,
 						FailoverTimeout: time.Second,
-					})
+					}, prometheus.NewRegistry())


Suggested change

}, prometheus.NewRegistry())

}, nil)

Since we don't verify metrics.

pstibrany · 2020-06-02T14:14:17Z

pkg/ring/kv/metrics.go

+	// If no Registerer is provided return the raw client
+	if reg == nil {
+		return c
+	}


Nit: I'd move this check into createClient function.

pstibrany · 2020-06-02T14:15:34Z

pkg/ring/kv/metrics.go

+		return nil
+	}
+
+	return prometheus.WrapRegistererWith(prometheus.Labels{"name": name}, reg)


Is it worth using something more unique, e.g. kv-name here?

That makes sense. I was just basing it off the label we use for the ring.

pstibrany · 2020-06-02T14:16:21Z

pkg/ring/kv/metrics.go

+	primaryLabel   = prometheus.Labels{"role": "primary"}
+	secondaryLabel = prometheus.Labels{"role": "secondary"}


Only used from client.go, I would move it into that file.

pstibrany · 2020-06-02T14:22:33Z

CHANGELOG.md


 ## master / unreleased

+* [CHANGE] Metric `cortex_kv_request_duration_seconds` now includes `name` label to denote which client is being used as well as the `backend` label to denote the KV backend implemnetation in use. #2648


Suggested change

* [CHANGE] Metric `cortex_kv_request_duration_seconds` now includes `name` label to denote which client is being used as well as the `backend` label to denote the KV backend implemnetation in use. #2648

* [CHANGE] Metric `cortex_kv_request_duration_seconds` now includes `name` label to denote which client is being used as well as the `backend` label to denote the KV backend implementation in use. #2648

pstibrany · 2020-06-02T14:23:53Z

pkg/distributor/distributor_ring.go

+var (
+	RingNameForClient = "distributor"
+)
+


Suggested change

var (

RingNameForClient = "distributor"

)

const (

ringNameForClient = "distributor"

)

And move to distributor_ring.go where it is used.

(Alternatively, just inline it)

pstibrany · 2020-06-02T14:28:59Z

pkg/ruler/ruler.go

+		ringStore, err := kv.NewClient(
+			cfg.Ring.KVStore,
+			ring.GetCodec(),
+			kv.RegistererWithKVName(reg, ring.RulerRingKey),


Suggested change

kv.RegistererWithKVName(reg, ring.RulerRingKey),

kv.RegistererWithKVName(reg, "ruler"),

ring.RulerRingKey is actually ring.

pstibrany · 2020-06-02T14:34:57Z

pkg/storegateway/gateway.go

+		ringStore, err = kv.NewClient(
+			gatewayCfg.ShardingRing.KVStore,
+			ring.GetCodec(),
+			kv.RegistererWithKVName(reg, RingNameForClient),


Suggested change

kv.RegistererWithKVName(reg, RingNameForClient),

kv.RegistererWithKVName(reg, "store-gateway"),

Signed-off-by: Jacob Lisi <[email protected]>

pracucci

LGTM, thanks for patiently address my comments!

pull-request-size bot added the size/L label May 28, 2020

jtlisi changed the title ~~initial attempt~~ Generic KV Client Instrumentation May 28, 2020

jtlisi force-pushed the 20200528_generic_kv_metrics branch 2 times, most recently from de55827 to 170eb06 Compare May 28, 2020 20:20

pstibrany reviewed May 29, 2020

View reviewed changes

pracucci reviewed May 29, 2020

View reviewed changes

jtlisi added 9 commits June 1, 2020 16:07

initial attempt

928a757

Signed-off-by: Jacob Lisi <[email protected]>

different name for block store queryable

0f4b134

Signed-off-by: Jacob Lisi <[email protected]>

revert circleci config

6ea374a

Signed-off-by: Jacob Lisi <[email protected]>

don't register nil reg in kv metrics

fc91e8f

Signed-off-by: Jacob Lisi <[email protected]>

refactor per PR comments

e1b95f9

Signed-off-by: Jacob Lisi <[email protected]>

ensure nil registry is not wrapped and provided to KV store

c8516c2

Signed-off-by: Jacob Lisi <[email protected]>

update distributor ha-tracker client name

a62c493

Signed-off-by: Jacob Lisi <[email protected]>

re-add consul instrumentation

5f3671e

Signed-off-by: Jacob Lisi <[email protected]>

update changelog

40c71c7

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi force-pushed the 20200528_generic_kv_metrics branch from ff4541e to 40c71c7 Compare June 1, 2020 20:07

pstibrany approved these changes Jun 2, 2020

View reviewed changes

refactor per PR comments

9097f3d

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi mentioned this pull request Jun 3, 2020

Update kv metrics to respect kv_name label grafana/cortex-jsonnet#86

Merged

pracucci approved these changes Jun 4, 2020

View reviewed changes

pracucci merged commit 1188a91 into cortexproject:master Jun 4, 2020

pracucci deleted the 20200528_generic_kv_metrics branch June 4, 2020 07:01

		primaryLabel = prometheus.Labels{"role": "primary"}
		secondaryLabel = prometheus.Labels{"role": "secondary"}


		## master / unreleased

		* [CHANGE] Metric `cortex_kv_request_duration_seconds` now includes `name` label to denote which client is being used as well as the `backend` label to denote the KV backend implemnetation in use. #2648

	kv.RegistererWithKVName(reg, ring.RulerRingKey),
	kv.RegistererWithKVName(reg, "ruler"),

	kv.RegistererWithKVName(reg, RingNameForClient),
	kv.RegistererWithKVName(reg, "store-gateway"),

Generic KV Client Instrumentation #2648

Generic KV Client Instrumentation #2648

Uh oh!

Conversation

jtlisi commented May 28, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtlisi commented Jun 1, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

pracucci commented May 29, 2020 •

edited

Loading

jtlisi Jun 1, 2020 •

edited

Loading