Skip to content

Commit

Permalink
fix(background cache): increment queue size exactly once (grafana#11776)
Browse files Browse the repository at this point in the history
**What this PR does / why we need it**:
While looking at the metrics from one of our cells, we noticed that the
queue size is set to a very high value even when the length is pretty
much 0.

Background cache is incrementing the queue size twice for each enqueued
key. This pr removes the additional increment call.

`TestBackgroundSizeLimit` might be flaky because of the
[writeBackLoop](https://github.com/grafana/loki/blob/8eb09c78c842b61f6619fb3755a43b180536b761/pkg/storage/chunk/cache/background.go#L193)
which dequeues from the channel and reduces the queue size concurrently.
To make the test predictable, I have set the `WriteBackGoroutines` to 0.

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [x] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/setup/upgrade/_index.md`
- [ ] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](grafana@d10549e)
- [ ] 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](grafana@0d4416a)
  • Loading branch information
ashwanthgoli authored and Gordejj committed Jan 29, 2024
1 parent 78a9099 commit 111089b
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
* [11606](https://github.com/grafana/loki/pull/11606) **dannykopping** Fixed regression adding newlines to HTTP error response bodies which may break client integrations.
* [11657](https://github.com/grafana/loki/pull/11657) **ashwanthgoli** Log results cache: compose empty response based on the request being served to avoid returning incorrect limit or direction.
* [11587](https://github.com/grafana/loki/pull/11587) **trevorwhitney** Fix semantics of label parsing logic of metrics and logs queries. Both only parse the first label if multiple extractions into the same label are requested.
* [11776](https://github.com/grafana/loki/pull/11776) **ashwanthgoli** Background Cache: Fixes a bug that is causing the background queue size to be incremented twice for each enqueued item.

##### Changes

Expand Down
1 change: 0 additions & 1 deletion pkg/storage/chunk/cache/background.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ func (c *backgroundCache) Store(ctx context.Context, keys []string, bufs [][]byt

select {
case c.bgWrites <- bgWrite:
c.size.Add(int64(size))
c.queueBytes.Set(float64(c.size.Load()))
c.queueLength.Add(float64(num))
c.enqueuedBytes.Add(float64(size))
Expand Down
5 changes: 5 additions & 0 deletions pkg/storage/chunk/cache/background_extra_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,8 @@ func Flush(c Cache) {
close(b.bgWrites)
b.wg.Wait()
}

func QueueSize(c Cache) int64 {
b := c.(*backgroundCache)
return b.size.Load()
}
10 changes: 5 additions & 5 deletions pkg/storage/chunk/cache/background_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestBackgroundSizeLimit(t *testing.T) {
require.NoError(t, err)

c := cache.NewBackground("mock", cache.BackgroundConfig{
WriteBackGoroutines: 1,
WriteBackGoroutines: 0,
WriteBackBuffer: 100,
WriteBackSizeLimit: flagext.ByteSize(limit),
}, cache.NewMockCache(), nil)
Expand All @@ -63,10 +63,10 @@ func TestBackgroundSizeLimit(t *testing.T) {

// store the first 10KB
require.NoError(t, c.Store(ctx, []string{firstKey}, [][]byte{first}))
require.Equal(t, cache.QueueSize(c), int64(10e3))

// second key will not be stored because it will exceed the 15KB limit
require.NoError(t, c.Store(ctx, []string{secondKey}, [][]byte{second}))
cache.Flush(c)

found, _, _, _ := c.Fetch(ctx, []string{firstKey, secondKey})
require.Equal(t, []string{firstKey}, found)
require.Equal(t, cache.QueueSize(c), int64(10e3))
c.Stop()
}

0 comments on commit 111089b

Please sign in to comment.