Loki: Modifies TableManager to use IndexGateway ring #5972

DylanGuedes · 2022-04-20T17:27:58Z

What this PR does / why we need it:

Adds a new entity indexgateway.RingManager, responsible for taking care of the index gateway ring for both, index gateway clients and servers. Now, the IndexGatewayRing service is basically a wrapper for the indexgateway.RingManager.
Add the IndexGatewayRing service as a dependency for the IndexGateway
Adds a new OwnsTenant function as a parameter to the shipper. This function replies to whether a tenant is assigned to some instance or not.
Modifies the TableManager to only download data from tenants assigned to it through the new OwnsTenant function.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
This depends on PR #5946.

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

- Add a new `query_readiness_duration_seconds` metric, that reports query readiness duration of a tablemanager/index gateway instance. We should use it later to report performance against the ring mode - Add a new `usersToBeQueryReadyForTotal` metric, that reports number of users involved in the query readiness operation. We should use it later to correlate number of users with the query readiness duration.

- It will report all users always for now, so it isn't too helpful the way it is.

pkg/storage/stores/shipper/indexgateway/gateway.go

JordanRushing

LGTM!

- Add a new `query_readiness_duration_seconds` metric, that reports query readiness duration of a tablemanager/index gateway instance. We should use it later to report performance against the ring mode - Add a new `usersToBeQueryReadyForTotal` metric, that reports number of users involved in the query readiness operation. We should use it later to correlate number of users with the query readiness duration.

- It will report all users always for now, so it isn't too helpful the way it is.

- Separate the assigning of indexClient in the IndexGateway to allow initializing the shipper with the IndexGateway ring - Add new TenantBoundariesClient entity, that answers if a given tenantID should be ignored or not - Use the TenantBoundariesClient implemented by the IndexGateway in the downloads TableManager

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Rewrite QueryIndex error phrase. Co-authored-by: JordanRushing <rushing.jordan@gmail.com>

grafanabot · 2022-05-06T11:24:10Z

./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%
+           chunkenc	0%
+              logql	0%
-               loki	-4.2%

grafanabot · 2022-05-06T13:58:22Z

./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.4%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-05-06T15:00:54Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0%

…ng-ring

grafanabot · 2022-05-18T16:49:45Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-05-18T17:43:08Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0%

sandeepsukhani

I have added few minor nits but overall code looks great to me.

pkg/loki/modules.go

pkg/storage/stores/shipper/downloads/table_manager.go

pkg/storage/stores/shipper/indexgateway/ringmanager.go

pkg/storage/stores/shipper/gateway_client.go

sandeepsukhani · 2022-05-19T07:58:36Z

pkg/util/ring.go

+	bufDescs, bufHosts, bufZones := ring.MakeBuffersForGet()
+	token := TokenFor(key, "" /* labels */)
+	rs, err := ringClient.Get(token, ring.WriteNoExtend, bufDescs, bufHosts, bufZones)
+	if err == nil {


Let us log the error if not nil

sandeepsukhani · 2022-05-19T08:03:05Z

pkg/util/ring.go

+}
+
+// HasDedicatedAddress specify an instance support for replying its own address.
+type HasDedicatedAddress interface {


Naming is hard :)
It doesn't sound correct to me. What do you think about using RingInstanceInfoReader or simply going back to previous implementation to pass the address directly?

Hahaha it is!

Agreed, it looks odd. I returned to pass the address directly, lmk what you think.

Document that tenant filtering is only applied during query readiness. Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

grafanabot · 2022-05-19T11:31:18Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0%

- Also removes HasDedicatedAddress, since it isn't necessary anymore

grafanabot · 2022-05-19T11:40:07Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0%

sandeepsukhani

just a minor nit, otherwise it LGTM

pkg/util/ring.go

grafanabot · 2022-05-19T14:20:43Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0.1%

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

grafanabot · 2022-05-19T16:39:16Z

./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%
+           chunkenc	0%
+              logql	0%
+               loki	0.1%

sandeepsukhani

LGTM! Thanks for patiently taking care of all the feedback!

DylanGuedes added 4 commits April 18, 2022 10:27

Remove usersToBeQueryReadyForTotal.

63321f9

- It will report all users always for now, so it isn't too helpful the way it is.

Rename metric help text to not mislead people.

cbbd4b6

Log queryReadiness duration.

f9c520a

pull-request-size bot added the size/L label Apr 20, 2022

DylanGuedes force-pushed the table-manager-using-ring branch 2 times, most recently from 7193545 to 5551b48 Compare April 20, 2022 18:42

DylanGuedes marked this pull request as ready for review April 20, 2022 21:26

DylanGuedes requested a review from a team as a code owner April 20, 2022 21:26

DylanGuedes requested review from cyriltovena and sandeepsukhani April 22, 2022 13:27

DylanGuedes requested a review from KMiller-Grafana as a code owner April 22, 2022 16:43

JordanRushing reviewed Apr 22, 2022

View reviewed changes

pkg/storage/stores/shipper/indexgateway/gateway.go Outdated Show resolved Hide resolved

JordanRushing approved these changes Apr 22, 2022

View reviewed changes

Fix where log message and duration and triggered.

b4737dc

DylanGuedes force-pushed the table-manager-using-ring branch from ab4ea75 to 57340b6 Compare April 25, 2022 15:27

pull-request-size bot added size/XXL and removed size/L labels Apr 25, 2022

DylanGuedes and others added 10 commits April 25, 2022 12:28

Remove usersToBeQueryReadyForTotal.

02da788

- It will report all users always for now, so it isn't too helpful the way it is.

Rename metric help text to not mislead people.

0be5dcd

Log queryReadiness duration.

860f855

Fix where log message and duration and triggered.

7bac294

Add IndexGateway configuration docs.

c1a83ac

Add tests for pkg/util/ring TokenFor() and IsAssignedKey()

db60fdb

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Small whitespace fix in pkg/util/ring_test.go

e9f9b28

Signed-off-by: JordanRushing <rushing.jordan@gmail.com>

Apply suggestions from code review

44e249c

Rewrite QueryIndex error phrase. Co-authored-by: JordanRushing <rushing.jordan@gmail.com>

DylanGuedes force-pushed the table-manager-using-ring branch from 57340b6 to 44e249c Compare April 25, 2022 15:29

pull-request-size bot removed the size/XXL label Apr 25, 2022

Log len of users instead of all of them to avoid log msgs.

f747d82

Merge branch 'main' of github.com:grafana/loki into table-manager-usi…

1ec30f0

…ng-ring

Add docstrings to new public functions.

7d90edd

sandeepsukhani requested changes May 19, 2022

View reviewed changes

Apply suggestions from code review

3500c5a

Document that tenant filtering is only applied during query readiness. Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Modify IsAssignedKey to expect address directly instead of lifecycler.

a55bdc9

- Also removes HasDedicatedAddress, since it isn't necessary anymore

DylanGuedes added 5 commits May 19, 2022 08:40

Log error message when IsInReplicationSet fails.

a6a2b47

Modify IsAssignedKey to return true by default.

da4c7ee

Remove wrong assigning of Service for clientMode.

d4207a7

Log gateway requests before error checks.

3d32ed2

Remove unnecessary endpoint registration.

103c977

sandeepsukhani approved these changes May 19, 2022

View reviewed changes

pkg/util/ring.go Outdated Show resolved Hide resolved

Fix lint.

69f19d1

Update pkg/util/ring.go

bbbcd5e

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

sandeepsukhani approved these changes May 20, 2022

View reviewed changes

sandeepsukhani merged commit 440fb7d into grafana:main May 20, 2022

chaudum removed the type/docs label Jun 13, 2022

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

osg-grafana added type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Modifies TableManager to use IndexGateway ring #5972

Loki: Modifies TableManager to use IndexGateway ring #5972

DylanGuedes commented Apr 20, 2022 •

edited

Loading

JordanRushing left a comment

grafanabot commented May 6, 2022

grafanabot commented May 6, 2022

grafanabot commented May 6, 2022

grafanabot commented May 18, 2022

grafanabot commented May 18, 2022

sandeepsukhani left a comment

sandeepsukhani May 19, 2022

sandeepsukhani May 19, 2022

DylanGuedes May 19, 2022

grafanabot commented May 19, 2022

grafanabot commented May 19, 2022

sandeepsukhani left a comment

grafanabot commented May 19, 2022

grafanabot commented May 19, 2022

sandeepsukhani left a comment

Loki: Modifies TableManager to use IndexGateway ring #5972

Loki: Modifies TableManager to use IndexGateway ring #5972

Conversation

DylanGuedes commented Apr 20, 2022 • edited Loading

JordanRushing left a comment

Choose a reason for hiding this comment

grafanabot commented May 6, 2022

grafanabot commented May 6, 2022

grafanabot commented May 6, 2022

grafanabot commented May 18, 2022

grafanabot commented May 18, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

sandeepsukhani May 19, 2022

Choose a reason for hiding this comment

sandeepsukhani May 19, 2022

Choose a reason for hiding this comment

DylanGuedes May 19, 2022

Choose a reason for hiding this comment

grafanabot commented May 19, 2022

grafanabot commented May 19, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

grafanabot commented May 19, 2022

grafanabot commented May 19, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

DylanGuedes commented Apr 20, 2022 •

edited

Loading