From 1e0ff4d4379b216c02405330f82d2fefec74bcd6 Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Thu, 11 Jan 2024 18:38:30 +0300 Subject: [PATCH] Pull request 2119: 6570-querylog-size-memory Updates #6570. Squashed commit of the following: commit 92b2723ac1b2a78138a55cb82a3222f66119bbda Merge: 2da12283b 0143c3aac Author: Stanislav Chzhen Date: Tue Jan 9 17:07:23 2024 +0300 Merge branch 'master' into 6570-querylog-size-memory commit 2da12283b5f504fa77f08fa6026fa9a57b806b38 Author: Stanislav Chzhen Date: Tue Jan 9 17:04:54 2024 +0300 all: imp tests commit 1cb404c65d4e9981b709d689fd281253eca01f82 Merge: 5f7d20516 94d437d40 Author: Stanislav Chzhen Date: Thu Dec 28 20:18:00 2023 +0300 Merge branch 'master' into 6570-querylog-size-memory commit 5f7d20516934867e1a141c19a97edb49407884ee Author: Stanislav Chzhen Date: Wed Dec 27 15:07:54 2023 +0300 all: imp docs commit 0b17cfc4243a88606c62e4b1ae893a09149655b5 Author: Stanislav Chzhen Date: Mon Dec 25 20:06:09 2023 +0300 all: querylog size memory --- CHANGELOG.md | 2 ++ internal/aghalg/ringbuffer.go | 18 +++++------------ internal/aghalg/ringbuffer_test.go | 32 +++++++++++++----------------- internal/home/config.go | 2 +- internal/querylog/qlog.go | 5 +++-- internal/querylog/querylog.go | 11 ++++++---- internal/querylog/search.go | 14 ++++++++++--- 7 files changed, 43 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cccbec4..91551163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,7 @@ NOTE: Add new changes BELOW THIS COMMENT. ### Fixed +- Zero value in `querylog.size_memory` disables logging ([#6570]). - Non-anonymized IP addresses on the dashboard ([#6584]). - Maximum cache TTL requirement when editing minimum cache TTL in the Web UI ([#6409]). @@ -80,6 +81,7 @@ NOTE: Add new changes BELOW THIS COMMENT. [#6541]: https://github.com/AdguardTeam/AdGuardHome/issues/6541 [#6545]: https://github.com/AdguardTeam/AdGuardHome/issues/6545 [#6568]: https://github.com/AdguardTeam/AdGuardHome/issues/6568 +[#6570]: https://github.com/AdguardTeam/AdGuardHome/issues/6570 [#6574]: https://github.com/AdguardTeam/AdGuardHome/issues/6574 [#6584]: https://github.com/AdguardTeam/AdGuardHome/issues/6584 diff --git a/internal/aghalg/ringbuffer.go b/internal/aghalg/ringbuffer.go index b97bcc67..3faa8cb5 100644 --- a/internal/aghalg/ringbuffer.go +++ b/internal/aghalg/ringbuffer.go @@ -1,23 +1,15 @@ package aghalg -import ( - "github.com/AdguardTeam/golibs/errors" -) - // RingBuffer is the implementation of ring buffer data structure. type RingBuffer[T any] struct { buf []T - cur int + cur uint full bool } // NewRingBuffer initializes the new instance of ring buffer. size must be // greater or equal to zero. -func NewRingBuffer[T any](size int) (rb *RingBuffer[T]) { - if size < 0 { - panic(errors.Error("ring buffer: size must be greater or equal to zero")) - } - +func NewRingBuffer[T any](size uint) (rb *RingBuffer[T]) { return &RingBuffer[T]{ buf: make([]T, size), } @@ -30,7 +22,7 @@ func (rb *RingBuffer[T]) Append(e T) { } rb.buf[rb.cur] = e - rb.cur = (rb.cur + 1) % cap(rb.buf) + rb.cur = (rb.cur + 1) % uint(cap(rb.buf)) if rb.cur == 0 { rb.full = true } @@ -87,12 +79,12 @@ func (rb *RingBuffer[T]) splitCur() (before, after []T) { } // Len returns a length of the buffer. -func (rb *RingBuffer[T]) Len() (l int) { +func (rb *RingBuffer[T]) Len() (l uint) { if !rb.full { return rb.cur } - return cap(rb.buf) + return uint(cap(rb.buf)) } // Clear clears the buffer. diff --git a/internal/aghalg/ringbuffer_test.go b/internal/aghalg/ringbuffer_test.go index 43fbb46b..0209d27f 100644 --- a/internal/aghalg/ringbuffer_test.go +++ b/internal/aghalg/ringbuffer_test.go @@ -9,13 +9,13 @@ import ( ) // elements is a helper function that returns n elements of the buffer. -func elements(b *aghalg.RingBuffer[int], n int, reverse bool) (es []int) { +func elements(b *aghalg.RingBuffer[int], n uint, reverse bool) (es []int) { fn := b.Range if reverse { fn = b.ReverseRange } - i := 0 + var i uint fn(func(e int) (cont bool) { if i >= n { return false @@ -42,19 +42,14 @@ func TestNewRingBuffer(t *testing.T) { assert.Zero(t, b.Len()) }) - t.Run("negative_size", func(t *testing.T) { - assert.PanicsWithError(t, "ring buffer: size must be greater or equal to zero", func() { - aghalg.NewRingBuffer[int](-5) - }) - }) - t.Run("zero", func(t *testing.T) { b := aghalg.NewRingBuffer[int](0) for i := 0; i < 10; i++ { b.Append(i) - assert.Equal(t, 0, b.Len()) - assert.Empty(t, elements(b, b.Len(), false)) - assert.Empty(t, elements(b, b.Len(), true)) + bufLen := b.Len() + assert.EqualValues(t, 0, bufLen) + assert.Empty(t, elements(b, bufLen, false)) + assert.Empty(t, elements(b, bufLen, true)) } }) @@ -62,9 +57,10 @@ func TestNewRingBuffer(t *testing.T) { b := aghalg.NewRingBuffer[int](1) for i := 0; i < 10; i++ { b.Append(i) - assert.Equal(t, 1, b.Len()) - assert.Equal(t, []int{i}, elements(b, b.Len(), false)) - assert.Equal(t, []int{i}, elements(b, b.Len(), true)) + bufLen := b.Len() + assert.EqualValues(t, 1, bufLen) + assert.Equal(t, []int{i}, elements(b, bufLen, false)) + assert.Equal(t, []int{i}, elements(b, bufLen, true)) } }) } @@ -78,7 +74,7 @@ func TestRingBuffer_Range(t *testing.T) { name string want []int count int - length int + length uint }{{ name: "three", count: 3, @@ -163,11 +159,11 @@ func TestRingBuffer_Range_increment(t *testing.T) { for i, tc := range testCases { t.Run(tc.name, func(t *testing.T) { b.Append(i) - - assert.Equal(t, tc.want, elements(b, b.Len(), false)) + bufLen := b.Len() + assert.Equal(t, tc.want, elements(b, bufLen, false)) slices.Reverse(tc.want) - assert.Equal(t, tc.want, elements(b, b.Len(), true)) + assert.Equal(t, tc.want, elements(b, bufLen, true)) }) } } diff --git a/internal/home/config.go b/internal/home/config.go index 231e6521..753e1260 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -267,7 +267,7 @@ type queryLogConfig struct { // MemSize is the number of entries kept in memory before they are flushed // to disk. - MemSize int `yaml:"size_memory"` + MemSize uint `yaml:"size_memory"` // Enabled defines if the query log is enabled. Enabled bool `yaml:"enabled"` diff --git a/internal/querylog/qlog.go b/internal/querylog/qlog.go index ae058541..73ac7e6a 100644 --- a/internal/querylog/qlog.go +++ b/internal/querylog/qlog.go @@ -196,7 +196,7 @@ func newLogEntry(params *AddParams) (entry *logEntry) { // Add implements the [QueryLog] interface for *queryLog. func (l *queryLog) Add(params *AddParams) { var isEnabled, fileIsEnabled bool - var memSize int + var memSize uint func() { l.confMu.RLock() defer l.confMu.RUnlock() @@ -205,7 +205,7 @@ func (l *queryLog) Add(params *AddParams) { memSize = l.conf.MemSize }() - if !isEnabled || memSize == 0 { + if !isEnabled { return } @@ -230,6 +230,7 @@ func (l *queryLog) Add(params *AddParams) { if !l.flushPending && fileIsEnabled && l.buffer.Len() >= memSize { l.flushPending = true + // TODO(s.chzhen): Fix occasional rewrite of entires. go func() { flushErr := l.flushLogBuffer() if flushErr != nil { diff --git a/internal/querylog/querylog.go b/internal/querylog/querylog.go index b9292c7d..189f1f57 100644 --- a/internal/querylog/querylog.go +++ b/internal/querylog/querylog.go @@ -63,7 +63,7 @@ type Config struct { // MemSize is the number of entries kept in a memory buffer before they are // flushed to disk. - MemSize int + MemSize uint // Enabled tells if the query log is enabled. Enabled bool @@ -143,14 +143,17 @@ func newQueryLog(conf Config) (l *queryLog, err error) { } } - if conf.MemSize < 0 { - return nil, errors.Error("memory size must be greater or equal to zero") + memSize := conf.MemSize + if memSize == 0 { + // If query log is enabled, we still need to write entries to a file. + // And all writing goes through a buffer. + memSize = 1 } l = &queryLog{ findClient: findClient, - buffer: aghalg.NewRingBuffer[*logEntry](conf.MemSize), + buffer: aghalg.NewRingBuffer[*logEntry](memSize), conf: &Config{}, confMu: &sync.RWMutex{}, diff --git a/internal/querylog/search.go b/internal/querylog/search.go index b4fd3ffe..1a992e20 100644 --- a/internal/querylog/search.go +++ b/internal/querylog/search.go @@ -47,7 +47,14 @@ func (l *queryLog) client(clientID, ip string, cache clientCache) (c *Client, er // searchMemory looks up log records which are currently in the in-memory // buffer. It optionally uses the client cache, if provided. It also returns // the total amount of records in the buffer at the moment of searching. +// l.confMu is expected to be locked. func (l *queryLog) searchMemory(params *searchParams, cache clientCache) (entries []*logEntry, total int) { + // We use this configuration check because a buffer can contain a single log + // record. See [newQueryLog]. + if l.conf.MemSize == 0 { + return nil, 0 + } + l.bufferLock.Lock() defer l.bufferLock.Unlock() @@ -73,11 +80,12 @@ func (l *queryLog) searchMemory(params *searchParams, cache clientCache) (entrie return true }) - return entries, l.buffer.Len() + return entries, int(l.buffer.Len()) } -// search - searches log entries in the query log using specified parameters -// returns the list of entries found + time of the oldest entry +// search searches log entries in memory buffer and log file using specified +// parameters and returns the list of entries found and the time of the oldest +// entry. l.confMu is expected to be locked. func (l *queryLog) search(params *searchParams) (entries []*logEntry, oldest time.Time) { start := time.Now()