fix panic when using cassandra in multiple periodic configs or as a store for both index and delete requests #2774

sandeepsukhani · 2020-06-24T08:46:54Z

What this PR does:
We create Cassandra clients for object store, index store and table manager. We sometimes need multiple instances of each of them at different places based on how Cortex is configured. While creating a single instance for each type of usage is not a problem, creating multiple of same types of client instances causes the code to panic due to duplicate metric registration. See #2772. The issue can be reproduced easily by using Cassandra as index store with multiple periodic schemas.

This code adds a new label called purpose which gets sets based on the purpose of creating the client.
When Cassandra is used in multiple periodic config, purpose is set to start date of that periodic config.

Note: Using the same date for multiple periodic configs with Cassandra can still cause the code to panic since it would try to register the metric with the same label values. Considering the chances of someone having a need for such config is negligible we should be good here.

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

Checklist

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

tomwilkie · 2020-06-24T12:33:52Z

Globals are bad (especially like this - I can forgive them for metrics).

Can we have the name of the schema (perhaps based on its start and end date) passed to the client so it can name it metrics appropriately and prevent duplicates? I should be enough to add this as an extra label in the registererer.

tomwilkie · 2020-06-24T12:34:17Z

Also, lets add a test that reproduces this issue first (ie starts Cortex with two Cassandra schemas)

tomwilkie · 2020-06-25T13:01:21Z

pkg/chunk/cassandra/sessions.go

+
+var (
+	sharedSessions map[string]*sharedSession
+	sessionsMtx    sync.Mutex


These are still globals. If we give easy session its own name, we won't get registrations error, and we won't need globals.

Sorry, I saw your review after pushing the code. I thought we didn't want to create multiple sessions per schema config where Cassandra is use so I went this way which I accept is ugly.
I didn't get a chance to work on this yet. Will fix this.

tomwilkie

We can solve this problem without globals.

sandeepsukhani · 2020-06-26T08:30:14Z

@jtlisi Thanks! Will check it out.

sandeepsukhani · 2020-06-28T12:30:28Z

Globals are bad (especially like this - I can forgive them for metrics).

Can we have the name of the schema (perhaps based on its start and end date) passed to the client so it can name it metrics appropriately and prevent duplicates? I should be enough to add this as an extra label in the registererer.

@tomwilkie I have just added the start date of the schema to the label to keep things simple.
Also please note that when using the same date for multiple periodic configs with Cassandra can still cause the code to panic since it would try to register the metric with the same label values. Considering the chances of someone having a need for such config is negligible we should be good here. We can always add an end date as well to the label if someone faces an issue.
Please let me know if you think we should have it now.

tomwilkie · 2020-06-30T09:01:52Z

pkg/chunk/cassandra/storage_client.go

@@ -89,7 +89,7 @@ func (cfg *Config) Validate() error {
 	return nil
 }

-func (cfg *Config) session(name string) (*gocql.Session, error) {
+func (cfg *Config) session(name, purpose string, registerer prometheus.Registerer) (*gocql.Session, error) {


Lets not add purpose here, but rather pass in prometheus.DefaultRegisterer(prometheus.Labels{"schema_start": ...}) to NewStorageClient

tomwilkie · 2020-06-30T09:06:12Z

pkg/chunk/storage/factory.go

@@ -180,7 +180,7 @@ func NewStore(cfg Config, storeCfg chunk.StoreConfig, schemaCfg chunk.SchemaConf
 }

 // NewIndexClient makes a new index client of the desired type.
-func NewIndexClient(name string, cfg Config, schemaCfg chunk.SchemaConfig) (chunk.IndexClient, error) {
+func NewIndexClient(name string, cfg Config, schemaCfg chunk.SchemaConfig, purpose string, registerer prometheus.Registerer) (chunk.IndexClient, error) {


Lets not pass a string here but the DayTime. And lets not pass the registerer here, but wrap a new registerer next to the instantiation of the Cassandra client. Otherwise I feel it sets expectation that the registerer will be used for all client.s

The problem is NewIndexClient is also used to create IndexClient for storing delete requests here, which is not tied to start date.
I added the purpose label to track the sessions based on the purpose of usage which could be for storing series index(using Daytime for this since there are multiple schemas) or for indexing delete requests or any other use case that we may have in future.

In that case leave the registerer plumbed through but don't plumb reason though - just wrap the registerer before each instantiation of NewIndexClient.

We can create the registerer with the purpose or something label for all the clients if you think that is more appropriate and clearer. I think there is no harm in having an extra-label.

I don't mind having the extra label (its needed) but there is no point in plumbing it through like that. We should add it close to where it semantically makes sense.

@tomwilkie I have done these changes and also made changes to use the same registerer for metrics exported by dynamodb. Thanks for the review!

tomwilkie

Looks much better, thank you Sandeep. A few nits - lets not call the label purpose, its pretty meaningless. Call it "schema_start_date" or something.

pracucci

Thanks @sandeepsukhani ! I appreciate the refactoring you did. I left few comments, minor things, but I would be glad if you could take a look. Thanks!

Also, is there any existing unit test we can enrich passing the registerer and asserting on the collected metrics?

pkg/chunk/aws/metrics_autoscaling.go

pkg/cortex/modules.go

pkg/chunk/storage/factory.go

pkg/cortex/modules.go

pkg/chunk/aws/metrics.go

pkg/chunk/aws/dynamodb_storage_client.go

sandeepsukhani · 2020-07-06T11:54:27Z

Thanks @sandeepsukhani ! I appreciate the refactoring you did. I left few comments, minor things, but I would be glad if you could take a look. Thanks!

Thanks for the detailed feedback @pracucci!

Also, is there any existing unit test we can enrich passing the registerer and asserting on the collected metrics?

Unfortunately, I don't think we have any such test. None of the index clients was accepting a registerer before.
Also, different index clients have different metrics(if any) so we would only have tests specific to the index clients.

…and relevant tests Signed-off-by: Sandeep Sukhani <[email protected]>

Signed-off-by: Sandeep Sukhani <[email protected]>

…ions Signed-off-by: Sandeep Sukhani <[email protected]>

Signed-off-by: Sandeep Sukhani <[email protected]>

pracucci

Thanks @sandeepsukhani for addressing my feedback, LGTM (modulo a couple of final nits)!

CHANGELOG.md

pkg/cortex/modules.go

jtlisi

LGTM.

However, I'm a bit concerned if NewTableClient or NewIndexClient is called without a wrapped registerer. Could that lead to a registration error since the labels for a metric vary? We recently had this problem in the KV client #2807. If that is the case it might make sense to wrap the registerer within the functions themselves and add a name as a function parameter.

pkg/chunk/cassandra/storage_client.go

Signed-off-by: Sandeep Sukhani <[email protected]>

Sandeep has changed the PR after your initial review and I think the current state is good to go

pracucci · 2020-07-23T16:41:26Z

@sandeepsukhani Sorry for the veeeeeery long wait. I just learned how to dismiss a review and unblock the merge. Thanks for your work here and... 🚀 let's go!

pull-request-size bot added the size/M label Jun 24, 2020

sandeepsukhani force-pushed the cassandra-clients-singleton branch from 7093efa to 9d6a2d4 Compare June 24, 2020 11:05

pull-request-size bot added size/L and removed size/M labels Jun 24, 2020

sandeepsukhani force-pushed the cassandra-clients-singleton branch from 9d6a2d4 to 0f25693 Compare June 24, 2020 12:05

tomwilkie self-requested a review June 24, 2020 12:30

sandeepsukhani force-pushed the cassandra-clients-singleton branch from 0f25693 to 6614f0e Compare June 24, 2020 13:15

pull-request-size bot added size/M and removed size/L labels Jun 24, 2020

tomwilkie reviewed Jun 25, 2020

View reviewed changes

tomwilkie suggested changes Jun 25, 2020

View reviewed changes

sandeepsukhani force-pushed the cassandra-clients-singleton branch from 6614f0e to 925d0ec Compare June 28, 2020 12:17

pull-request-size bot added size/L and removed size/M labels Jun 28, 2020

sandeepsukhani changed the title ~~make all clients for cassandra singleton~~ fix panic when using cassandra in multiple periodic configs or as a store for both index and delete requests Jun 28, 2020

tomwilkie reviewed Jun 30, 2020

View reviewed changes

tomwilkie previously requested changes Jun 30, 2020

View reviewed changes

sandeepsukhani force-pushed the cassandra-clients-singleton branch 2 times, most recently from dc69eb9 to 51ac653 Compare July 1, 2020 08:02

pracucci reviewed Jul 2, 2020

View reviewed changes

add purpose label to metric being used for tracing cassandra session …

454ffc6

…and relevant tests Signed-off-by: Sandeep Sukhani <[email protected]>

sandeepsukhani force-pushed the cassandra-clients-singleton branch from 3609819 to 7c0f7a2 Compare July 8, 2020 07:48

sandeepsukhani added 2 commits July 8, 2020 13:19

update changelog

0a2e2f2

Signed-off-by: Sandeep Sukhani <[email protected]>

passing registerer wrapped with purpose to index client factory funct…

361fdd7

…ions Signed-off-by: Sandeep Sukhani <[email protected]>

sandeepsukhani added 2 commits July 8, 2020 13:19

add registerer for metrics in dynamodb clients

b69195a

Signed-off-by: Sandeep Sukhani <[email protected]>

changes suggested from PR review

8a3e8ac

Signed-off-by: Sandeep Sukhani <[email protected]>

sandeepsukhani force-pushed the cassandra-clients-singleton branch from 7c0f7a2 to 8a3e8ac Compare July 8, 2020 07:50

pracucci approved these changes Jul 8, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

pkg/cortex/modules.go Outdated Show resolved Hide resolved

pracucci requested a review from tomwilkie July 8, 2020 15:58

jtlisi approved these changes Jul 8, 2020

View reviewed changes

pkg/chunk/cassandra/storage_client.go Outdated Show resolved Hide resolved

sandeepsukhani and others added 3 commits July 9, 2020 19:50

minor nit

df15535

Signed-off-by: Sandeep Sukhani <[email protected]>

changes suggested from PR review

7d701ef

Signed-off-by: Sandeep Sukhani <[email protected]>

Merge branch 'master' into cassandra-clients-singleton

690d7cc

pracucci requested review from tomwilkie and removed request for tomwilkie July 9, 2020 15:30

pracucci merged commit a20a386 into cortexproject:master Jul 23, 2020

pstibrany mentioned this pull request Jul 31, 2020

Cassandra storage panics on registering prometheus metrics #2969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix panic when using cassandra in multiple periodic configs or as a store for both index and delete requests #2774

fix panic when using cassandra in multiple periodic configs or as a store for both index and delete requests #2774

sandeepsukhani commented Jun 24, 2020 •

edited

Loading

tomwilkie commented Jun 24, 2020

tomwilkie commented Jun 24, 2020

tomwilkie Jun 25, 2020

sandeepsukhani Jun 26, 2020

tomwilkie left a comment

sandeepsukhani commented Jun 26, 2020

sandeepsukhani commented Jun 28, 2020

tomwilkie Jun 30, 2020

tomwilkie Jun 30, 2020

sandeepsukhani Jun 30, 2020

tomwilkie Jun 30, 2020

sandeepsukhani Jun 30, 2020

tomwilkie Jun 30, 2020

sandeepsukhani Jul 1, 2020

tomwilkie left a comment

pracucci left a comment

sandeepsukhani commented Jul 6, 2020

pracucci left a comment

jtlisi left a comment

pracucci commented Jul 23, 2020

fix panic when using cassandra in multiple periodic configs or as a store for both index and delete requests #2774

fix panic when using cassandra in multiple periodic configs or as a store for both index and delete requests #2774

Conversation

sandeepsukhani commented Jun 24, 2020 • edited Loading

tomwilkie commented Jun 24, 2020

tomwilkie commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie left a comment

Choose a reason for hiding this comment

sandeepsukhani commented Jun 26, 2020

sandeepsukhani commented Jun 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwilkie left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

sandeepsukhani commented Jul 6, 2020

pracucci left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

pracucci commented Jul 23, 2020

sandeepsukhani commented Jun 24, 2020 •

edited

Loading