tsdb index gateway #6158

sandeepsukhani · 2022-05-13T11:28:12Z

What this PR does / why we need it:

enable new grpc calls on index gateway and add GetSeries call
Use index gateway client when enabled for boltdb-shipper index store.

Special notes for your reviewer:

Please do not merge this PR until we are ready to enable new grpc calls or we add a flag to control it since we had disabled them until we can improve index gateway performance.
We rely on boltdb-shipper config for now to initialize the index gateway client. We will have to discuss and decide how we want to proceed with tsdb config. I feel that we should share the same config to avoid complexities.

owen-d

Did a quick review; this is looking good!

Please do not merge this PR until we are ready to enable new grpc calls or we add a flag to control it since we had disabled them until we can improve index gateway performance.

Instead, can we create two clients, one which uses the new form and one which uses the old form? Then we can enable the new form when tsdb is enabled, but keep the old form elsewhere.

owen-d · 2022-05-16T13:53:20Z

pkg/storage/store.go


-		// TODO(owen-d): add TSDB index-gateway support
+		// ToDo(Sandeep): Refactor code to not use boltdb-shipper index gateway client config
+		if shouldUseBoltDBIndexGatewayClient(s.cfg) {


It may be better to have this function accept on the tsdb-shipper config as an argument instead.

This should all be cleaned up once I get to migrate boltdb-shipper to index-shipper.

owen-d · 2022-05-16T14:01:33Z

pkg/storage/stores/composite_store_entry.go

@@ -25,17 +25,6 @@ type StoreLimits interface {
 	MaxQueryLength(userID string) time.Duration
 }

-type Index interface {


Why did we move this into series.IndexStore?

I wanted to pass an interface to series.NewIndexGatewayClientStore without which it was not possible to pass tsdb index client. I had to move it to series.IndexStore to avoid circular dependency.

owen-d · 2022-05-16T14:01:53Z

pkg/storage/stores/series/series_index_gateway_store.go

 }

 type IndexGatewayClient interface {
 	GetChunkRef(ctx context.Context, in *indexgatewaypb.GetChunkRefRequest, opts ...grpc.CallOption) (*indexgatewaypb.GetChunkRefResponse, error)
+	GetSeries(ctx context.Context, in *indexgatewaypb.GetSeriesRequest, opts ...grpc.CallOption) (*indexgatewaypb.GetSeriesResponse, error)


<3 Adding this here, thanks.

owen-d · 2022-05-16T14:08:50Z

pkg/storage/store.go

-		idx := tsdb.NewIndexClient(headManager, p)
-		writer := tsdb.NewChunkWriter(f, p, headManager)
+		// ToDo(Sandeep): Avoid initializing writer when in read only mode
+		writer, idx, err := tsdb.NewStore(s.cfg.TSDBShipperConfig, p, f, objectClient, s.limits, indexClientReg)


I don't see tsdb.NewStore implemented yet, is that right? I'm guessing we'll just copy this over to the new function (we can also add a check on whether to enable the head manager as well).

Sorry, I missed pushing the file. I have pushed it yesterday.

sandeepsukhani · 2022-05-18T07:14:39Z

Did a quick review; this is looking good!

Please do not merge this PR until we are ready to enable new grpc calls or we add a flag to control it since we had disabled them until we can improve index gateway performance.

Instead, can we create two clients, one which uses the new form and one which uses the old form? Then we can enable the new form when tsdb is enabled, but keep the old form elsewhere.

We already have a fallback mechanism for backwards compatibility. I think we should add a flag to reuse most of the code then.

grafanabot · 2022-05-18T08:00:55Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0.1%
+           chunkenc	0%
+              logql	0%
+               loki	0%

…re ready to enable it again

sandeepsukhani · 2022-05-18T10:40:07Z

Did a quick review; this is looking good!

Please do not merge this PR until we are ready to enable new grpc calls or we add a flag to control it since we had disabled them until we can improve index gateway performance.

Instead, can we create two clients, one which uses the new form and one which uses the old form? Then we can enable the new form when tsdb is enabled, but keep the old form elsewhere.

We already have a fallback mechanism for backwards compatibility. I think we should add a flag to reuse most of the code then.

I have temporarily disabled using IndexGatewayClientStore for boltdb-shipper since that is what tries the new storage grpc calls on index gateway. Index gateway should continue to work as is since we create index gateway client instance here anyways.

grafanabot · 2022-05-18T11:14:37Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0.5%
+           chunkenc	0%
+              logql	0%
+               loki	0.6%

owen-d · 2022-05-18T13:50:09Z

I think we should move around the IndexStore definition to it's own location (doesn't make sense in the series pkg to me), but LGTM. I'd probably put it in a pkg/storage/stores/index package instead, but this is a nit.

sandeepsukhani added 2 commits May 13, 2022 16:18

enable new grpc calls on index gateway and add GetSeries call

d6ea1ca

use index gateway for tsdb index store queries

df56ae1

pull-request-size bot added the size/XXL label May 13, 2022

owen-d reviewed May 16, 2022

View reviewed changes

add missing file

9015649

lint

a4fb967

disable IndexGatewayClientStore for stores other than tsdb until we a…

85fe22f

…re ready to enable it again

sandeepsukhani marked this pull request as ready for review May 18, 2022 13:43

sandeepsukhani requested a review from a team as a code owner May 18, 2022 13:43

owen-d approved these changes May 18, 2022

View reviewed changes

owen-d merged commit 62f5e59 into grafana:main May 18, 2022

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsdb index gateway #6158

tsdb index gateway #6158

sandeepsukhani commented May 13, 2022

owen-d left a comment

owen-d May 16, 2022

sandeepsukhani May 18, 2022

owen-d May 16, 2022

sandeepsukhani May 18, 2022 •

edited

Loading

owen-d May 16, 2022

owen-d May 16, 2022

sandeepsukhani May 18, 2022

sandeepsukhani commented May 18, 2022

grafanabot commented May 18, 2022

sandeepsukhani commented May 18, 2022 •

edited

Loading

grafanabot commented May 18, 2022

owen-d commented May 18, 2022 •

edited

Loading

tsdb index gateway #6158

tsdb index gateway #6158

Conversation

sandeepsukhani commented May 13, 2022

owen-d left a comment

Choose a reason for hiding this comment

owen-d May 16, 2022

Choose a reason for hiding this comment

sandeepsukhani May 18, 2022

Choose a reason for hiding this comment

owen-d May 16, 2022

Choose a reason for hiding this comment

sandeepsukhani May 18, 2022 • edited Loading

Choose a reason for hiding this comment

owen-d May 16, 2022

Choose a reason for hiding this comment

owen-d May 16, 2022

Choose a reason for hiding this comment

sandeepsukhani May 18, 2022

Choose a reason for hiding this comment

sandeepsukhani commented May 18, 2022

grafanabot commented May 18, 2022

sandeepsukhani commented May 18, 2022 • edited Loading

grafanabot commented May 18, 2022

owen-d commented May 18, 2022 • edited Loading

sandeepsukhani May 18, 2022 •

edited

Loading

sandeepsukhani commented May 18, 2022 •

edited

Loading

owen-d commented May 18, 2022 •

edited

Loading