fix(ingest-limits): Get usage from owing instances only #16937

periklis · 2025-03-27T19:31:41Z

What this PR does / why we need it:
This PR updates the stream usage retrieval routine in the ingest-limits-frontend component to query only ingest-limits instances that own the requested streams. The purpose of this optimization is to streamline traffic between these components to necessary minimum per ExceedLimits request.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

github-actions · 2025-03-27T19:33:38Z

💻 Deploy preview deleted.

pkg/limits/frontend/mock_test.go

grobinson-grafana · 2025-04-02T14:00:36Z

docs/sources/shared/configuration.md

  [recheck_period: <duration> | default = 10s]

+  # The number of partitions to use for the ring.
+  # CLI flag: -ingest-limits-frontend.num-partitions


I am a bit worried that someone configures this for ingest-limits and forgets to configure it for ingest-limits-frontend. Perhaps we could share the configuration here? I'm not sure what's best...

grobinson-grafana · 2025-04-02T14:04:40Z

Can we remove partitions from GetStreamUsageRequest in logproto.proto? Does it still serve a use?

pkg/limits/frontend/ring.go

periklis · 2025-04-03T09:32:08Z

Can we remove partitions from GetStreamUsageRequest in logproto.proto? Does it still serve a use?

~~Yes it does because we want to check the requested patitions vs. the assigned ones. Between a the two RPC calls we still can get a re-assignment.~~

pkg/limits/frontend/ring.go

grobinson-grafana · 2025-04-03T13:49:36Z

pkg/limits/frontend/ring_test.go

 			instances := make([]ring.InstanceDesc, len(clients))

 			for i := 0; i < len(test.expectedAssignedPartitionsRequest); i++ {
+				expectedStreamUsageReq := (*logproto.GetStreamUsageRequest)(nil)


What does this do (the whole block of code rather than the specific line)? I'm not sure I follow here

Glad that you point that out. The whole loop needs an overhaul here. This setup was written when the two requests types matched one to one.

If I delete this, the tests still pass 😕

clients[i] = &mockIngestLimitsClient{ expectedAssignedPartitionsRequest: test.expectedAssignedPartitionsRequest[i], getAssignedPartitionsResponse: test.getAssignedPartitionsResponses[i], expectedStreamUsageRequest: test.expectedStreamUsageRequest[i], getStreamUsageResponse: test.getStreamUsageResponses[i], t: t, }

grobinson-grafana · 2025-04-03T14:30:42Z

One of the tests is panicking in frontend.go it seems:

// Sum the number of active streams and rates of all responses.
	for _, resp := range resps {
		activeStreamsTotal += resp.Response.ActiveStreams
		rateTotal += float64(resp.Response.Rate)
	}

grobinson-grafana · 2025-04-03T14:52:21Z

One of the tests is panicking in frontend.go it seems:

// Sum the number of active streams and rates of all responses.
	for _, resp := range resps {
		activeStreamsTotal += resp.Response.ActiveStreams
		rateTotal += float64(resp.Response.Rate)
	}

It's because of j := 0 instead of j := i.

grobinson-grafana · 2025-04-04T12:15:42Z

pkg/limits/frontend/ring_test.go

 				}
+			}
+
+			// Set up the mock clients for the stream usage requests.


Sorry but I still don't understand this? The tests pass without it. If you want to put in nils, wouldn't this work?

diff --git a/pkg/limits/frontend/ring_test.go b/pkg/limits/frontend/ring_test.go index e7621d0cf..b3f2b7a48 100644 --- a/pkg/limits/frontend/ring_test.go +++ b/pkg/limits/frontend/ring_test.go @@ -106,7 +106,7 @@ func TestRingStreamUsageGatherer_GetStreamUsage(t *testing.T) { Tenant: "test", StreamHashes: []uint64{1}, // Hash 1 maps to partition 1 }, - expectedAssignedPartitionsRequest: []*logproto.GetAssignedPartitionsRequest{{}, {}}, + expectedAssignedPartitionsRequest: []*logproto.GetAssignedPartitionsRequest{nil, nil}, getAssignedPartitionsResponses: []*logproto.GetAssignedPartitionsResponse{{ AssignedPartitions: map[int32]int64{ 1: time.Now().Add(-time.Second).UnixNano(), @@ -116,15 +116,17 @@ func TestRingStreamUsageGatherer_GetStreamUsage(t *testing.T) { 1: time.Now().UnixNano(), }, }}, - expectedStreamUsageRequest: []*logproto.GetStreamUsageRequest{{}, { - Tenant: "test", - StreamHashes: []uint64{1}, - }}, - getStreamUsageResponses: []*logproto.GetStreamUsageResponse{{}, { - Tenant: "test", - ActiveStreams: 1, - Rate: 10, - }}, + expectedStreamUsageRequest: []*logproto.GetStreamUsageRequest{ + nil, { + Tenant: "test", + StreamHashes: []uint64{1}, + }}, + getStreamUsageResponses: []*logproto.GetStreamUsageResponse{ + nil, { + Tenant: "test", + ActiveStreams: 1, + Rate: 10, + }}, expectedResponses: []GetStreamUsageResponse{{ Addr: "instance-1", Response: &logproto.GetStreamUsageResponse{ @@ -148,17 +150,11 @@ func TestRingStreamUsageGatherer_GetStreamUsage(t *testing.T) { t: t, expectedAssignedPartitionsRequest: test.expectedAssignedPartitionsRequest[i], getAssignedPartitionsResponse: test.getAssignedPartitionsResponses[i], + expectedStreamUsageRequest: test.expectedStreamUsageRequest[i], + getStreamUsageResponse: test.getStreamUsageResponses[i], } } - // Set up the mock clients for the stream usage requests. - // Note: Not every client will have a stream usage request because the - // requested stream hashes are owned only by a subset of partition consumers. - for i := range test.expectedStreamUsageRequest { - clients[i].(*mockIngestLimitsClient).expectedStreamUsageRequest = test.expectedStreamUsageRequest[i] - clients[i].(*mockIngestLimitsClient).getStreamUsageResponse = test.getStreamUsageResponses[i] - } - // Set up the instances for the ring. for i := range len(clients) { instances[i] = ring.InstanceDesc{

This commit fixes a bug where limits were incorrect after the change in #16937. This bug occurred because we stopped asking each limit instance if it knew about all stream hashes, and instead starting asking each limit instance if it knew about the stream hashes for its assigned partitions. This broke the logic in the frontend that was taking the intersection of all responses to calculate if a stream was unknown. It should now take the union instead.

Contains backports of these commits: ``` 2cde9b1 fix: skip streams over limits in dry-run mode (#17114) 805125c fix: fix a bug where limits were incorrect after #16937 (#17224) d106042 fix: fix bug where expectedStreamUsageRequest was not used (#17223) 69aeda1 feat: add tests for enforcing limits in distributors (#17124) ```

periklis self-assigned this Mar 27, 2025

periklis requested a review from a team as a code owner March 27, 2025 19:31

pull-request-size bot added the size/L label Mar 27, 2025

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Mar 27, 2025

periklis changed the title ~~Query only owning consumers~~ fix(ingest-limits): Get usage from owing instances only Mar 27, 2025

grobinson-grafana reviewed Apr 2, 2025

View reviewed changes

pkg/limits/frontend/mock_test.go Show resolved Hide resolved

grobinson-grafana reviewed Apr 2, 2025

View reviewed changes

pkg/limits/frontend/ring.go Outdated Show resolved Hide resolved

periklis force-pushed the query-only-owning-consumers branch 2 times, most recently from 249a12c to 0e9d003 Compare April 3, 2025 11:44

grobinson-grafana reviewed Apr 3, 2025

View reviewed changes

pkg/limits/frontend/ring.go Outdated Show resolved Hide resolved

periklis force-pushed the query-only-owning-consumers branch from d9b6cf5 to 0558080 Compare April 3, 2025 13:39

grobinson-grafana reviewed Apr 3, 2025

View reviewed changes

pkg/limits/frontend/ring.go Outdated Show resolved Hide resolved

grobinson-grafana reviewed Apr 3, 2025

View reviewed changes

periklis added 11 commits April 4, 2025 09:59

fix(ingest-limits): Get usage from owing instances only

473c705

Add docs

56a704c

Improve owner selection

4b20c14

Fix accidental remove

566907d

Map stream hashses to owning consumers

d43b247

Remove obsolete proto field

746986e

Fix data rase

ad65604

Apply code review suggestions

aefe73b

Add accidentally removed check

e7c8609

Fix typo

8ddd01a

Make clients setup more readable

744b63d

periklis force-pushed the query-only-owning-consumers branch from 1c3b77e to 744b63d Compare April 4, 2025 07:59

grobinson-grafana approved these changes Apr 4, 2025

View reviewed changes

periklis merged commit 5229a2a into main Apr 4, 2025
63 checks passed

periklis deleted the query-only-owning-consumers branch April 4, 2025 13:41

grobinson-grafana mentioned this pull request Apr 14, 2025

fix: fix a bug where limits were incorrect after grafana/loki#16937 #17224

Merged

6 tasks

grobinson-grafana added a commit that referenced this pull request Apr 15, 2025

fix: fix a bug where limits were incorrect after #16937 (#17224)

805125c

grobinson-grafana added a commit that referenced this pull request Apr 15, 2025

fix: fix a bug where limits were incorrect after #16937 (#17224)

1555341

periklis mentioned this pull request Apr 28, 2025

fix(ingest-limits): Query all assigned partitions #17473

Merged

6 tasks

This was referenced May 26, 2025

chore(k256): release 3.5.0 #17828

Open

chore(k257): release 3.5.0 #17926

Open

This was referenced Jun 24, 2025

chore(k260): release 3.5.0 #18210

Open

chore(k261): release 3.5.0 #18278

Open

This was referenced Jul 7, 2025

chore(k262): release 3.5.0 #18343

Open

chore(k263): release 3.5.0 #18442

Open

This was referenced Jul 21, 2025

chore(k264): release 3.5.0 #18516

Open

chore(k265): release 3.5.0 #18605

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ingest-limits): Get usage from owing instances only #16937

fix(ingest-limits): Get usage from owing instances only #16937

Uh oh!

periklis commented Mar 27, 2025

Uh oh!

github-actions bot commented Mar 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

grobinson-grafana Apr 2, 2025

Uh oh!

grobinson-grafana commented Apr 2, 2025

Uh oh!

Uh oh!

periklis commented Apr 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

grobinson-grafana Apr 3, 2025 •

edited

Loading

Uh oh!

periklis Apr 3, 2025

Uh oh!

grobinson-grafana Apr 3, 2025

Uh oh!

grobinson-grafana commented Apr 3, 2025

Uh oh!

grobinson-grafana commented Apr 3, 2025

Uh oh!

grobinson-grafana Apr 4, 2025

Uh oh!

Uh oh!

Uh oh!

fix(ingest-limits): Get usage from owing instances only #16937

fix(ingest-limits): Get usage from owing instances only #16937

Uh oh!

Conversation

periklis commented Mar 27, 2025

Uh oh!

github-actions bot commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

grobinson-grafana Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

grobinson-grafana commented Apr 2, 2025

Uh oh!

Uh oh!

periklis commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grobinson-grafana Apr 3, 2025 • 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!

periklis Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

grobinson-grafana Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

grobinson-grafana commented Apr 3, 2025

Uh oh!

grobinson-grafana commented Apr 3, 2025

Uh oh!

grobinson-grafana Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 27, 2025 •

edited

Loading

periklis commented Apr 3, 2025 •

edited

Loading

grobinson-grafana Apr 3, 2025 •

edited

Loading