From 78914a0632cc32c7a41e8dc621855bfb3fa37dbc Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Mon, 20 Feb 2023 11:17:28 +0300 Subject: [PATCH] all: fix tests --- CHANGELOG.md | 29 +++++++++++++++++-- internal/home/config.go | 2 +- internal/home/dns.go | 2 +- internal/querylog/http.go | 15 ++++++---- internal/stats/http.go | 10 +++---- internal/stats/http_test.go | 41 ++++++++++++++++++--------- internal/stats/stats.go | 30 ++++++++++---------- internal/stats/stats_internal_test.go | 3 +- internal/stats/stats_test.go | 6 ++-- 9 files changed, 90 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5407edfb..d70e0b18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,13 +25,38 @@ NOTE: Add new changes BELOW THIS COMMENT. ### Added -- The new HTTP APIs `PUT /control/stats/config/update`, `GET +- The new HTTP APIs `PUT /control/stats/config/update`, `GET control/stats/config` accept and return statisics config JSON file. The format of request body is described in `openapi/openapi.yaml`. -- The new HTTP APIs `PUT /control/querylog/config/update`, `GET +- The new HTTP APIs `PUT /control/querylog/config/update`, `GET control/querylog/config` accept and return query log config JSON file. The format of request body is described in `openapi/openapi.yaml`. +### Changed + +#### Configuration Changes + +In this release, the schema version has changed from 16 to 17. + +- Property `statistics.interval`, which in schema versions 16 and earlier used + to be an integer number of days, is now a string with a human-readable + duration: + + ```yaml + # BEFORE: + 'statistics': + # … + 'interval': 1 + + # AFTER: + 'statistics': + # … + 'interval': '24h' + ``` + + To rollback this change, convert the property back into days and change the + `schema_version` back to `16`. + ### Fixed - Failing service installation via script on FreeBSD ([#5431]). diff --git a/internal/home/config.go b/internal/home/config.go index 6797f734..6af776e4 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -486,7 +486,7 @@ func (c *configuration) write() (err error) { if Context.stats != nil { statsConf := stats.Config{} Context.stats.WriteDiskConfig(&statsConf) - config.Stats.Interval = timeutil.Duration{Duration: statsConf.LimitIvl} + config.Stats.Interval = timeutil.Duration{Duration: statsConf.Limit} config.Stats.Enabled = statsConf.Enabled config.Stats.Ignored = statsConf.Ignored.Values() sort.Strings(config.Stats.Ignored) diff --git a/internal/home/dns.go b/internal/home/dns.go index 3426b61a..14ad4b09 100644 --- a/internal/home/dns.go +++ b/internal/home/dns.go @@ -51,7 +51,7 @@ func initDNS() (err error) { statsConf := stats.Config{ Filename: filepath.Join(baseDir, "stats.db"), - LimitIvl: config.Stats.Interval.Duration, + Limit: config.Stats.Interval.Duration, ConfigModified: onConfigModified, HTTPRegister: httpRegister, Enabled: config.Stats.Enabled, diff --git a/internal/querylog/http.go b/internal/querylog/http.go index 73434be1..7c39ac1a 100644 --- a/internal/querylog/http.go +++ b/internal/querylog/http.go @@ -38,16 +38,19 @@ type configJSON struct { // configJSONv2 is the JSON structure for the querylog configuration. type configJSONv2 struct { - // Enabled shows if the querylog is enabled. It is an [aghalg.NullBool] - // to be able to tell when it's set without using pointers. + // Enabled shows if the querylog is enabled. It is an + // [aghalg.NullBool] to be able to tell when it's set without using + // pointers. Enabled aghalg.NullBool `json:"enabled"` - // AnonymizeClientIP shows if the clients' IP addresses must be anonymized. - // It is an [aghalg.NullBool] to be able to tell when it's set without using - // pointers. + // AnonymizeClientIP shows if the clients' IP addresses must be + // anonymized. It is an [aghalg.NullBool] to be able to tell when it's + // set without using pointers. + // + // TODO(a.garipov): Consider using separate setting for statistics. AnonymizeClientIP aghalg.NullBool `json:"anonymize_client_ip"` - // Interval is the querylog rotation interval. + // Interval is the querylog rotation interval in milliseconds Interval float64 `json:"interval"` // Ignored is the list of host names, which should not be written to diff --git a/internal/stats/http.go b/internal/stats/http.go index 71019b23..b61bf1b1 100644 --- a/internal/stats/http.go +++ b/internal/stats/http.go @@ -47,7 +47,7 @@ func (s *StatsCtx) handleStats(w http.ResponseWriter, r *http.Request) { defer s.lock.Unlock() start := time.Now() - resp, ok := s.getData(uint32(s.limitIvl.Hours())) + resp, ok := s.getData(uint32(s.limit.Hours())) log.Debug("stats: prepared data in %v", time.Since(start)) if !ok { @@ -72,7 +72,7 @@ type configRespV2 struct { // to be able to tell when it's set without using pointers. Enabled aghalg.NullBool `json:"enabled"` - // Interval is the statistics rotation interval. + // Interval is the statistics rotation interval in milliseconds. Interval float64 `json:"interval"` // Ignored is the list of host names, which should not be counted. @@ -86,7 +86,7 @@ func (s *StatsCtx) handleStatsInfo(w http.ResponseWriter, r *http.Request) { s.lock.Lock() defer s.lock.Unlock() - days := uint32(s.limitIvl.Hours() / 24) + days := uint32(s.limit.Hours() / 24) ok := checkInterval(days) if !ok || (s.enabled && days == 0) { @@ -107,7 +107,7 @@ func (s *StatsCtx) handleStatsInfoV2(w http.ResponseWriter, r *http.Request) { resp := configRespV2{ Enabled: aghalg.BoolToNullBool(s.enabled), - Interval: float64(s.limitIvl.Milliseconds()), + Interval: float64(s.limit.Milliseconds()), Ignored: s.ignored.Values(), } err := aghhttp.WriteJSONResponse(w, r, resp) @@ -169,7 +169,7 @@ func (s *StatsCtx) handleStatsConfigV2(w http.ResponseWriter, r *http.Request) { s.enabled = reqData.Enabled == aghalg.NBTrue - s.limitIvl = ivl + s.limit = ivl if len(reqData.Ignored) > 0 { set, serr := aghnet.NewDomainNameSet(reqData.Ignored) diff --git a/internal/stats/http_test.go b/internal/stats/http_test.go index acef23b0..2090c0a7 100644 --- a/internal/stats/http_test.go +++ b/internal/stats/http_test.go @@ -27,6 +27,7 @@ func TestHandleStatsV2(t *testing.T) { name string body configRespV2 wantCode int + wantErr string }{{ name: "set_ivl_1_hour", body: configRespV2{ @@ -35,6 +36,7 @@ func TestHandleStatsV2(t *testing.T) { Ignored: nil, }, wantCode: http.StatusOK, + wantErr: "", }, { name: "small_interval", body: configRespV2{ @@ -43,6 +45,7 @@ func TestHandleStatsV2(t *testing.T) { Ignored: []string{}, }, wantCode: http.StatusBadRequest, + wantErr: "unsupported interval\n", }, { name: "big_interval", body: configRespV2{ @@ -51,6 +54,7 @@ func TestHandleStatsV2(t *testing.T) { Ignored: []string{}, }, wantCode: http.StatusBadRequest, + wantErr: "unsupported interval\n", }, { name: "set_ignored_ivl_1_year", body: configRespV2{ @@ -58,10 +62,11 @@ func TestHandleStatsV2(t *testing.T) { Interval: float64(year.Milliseconds()), Ignored: []string{ "ignor.ed", - "ignor.ed", + "ignored.to", }, }, - wantCode: http.StatusBadRequest, + wantCode: http.StatusOK, + wantErr: "", }, { name: "ignored_duplicate", body: configRespV2{ @@ -73,6 +78,7 @@ func TestHandleStatsV2(t *testing.T) { }, }, wantCode: http.StatusBadRequest, + wantErr: "ignored: duplicate host name \"ignor.ed\"\n", }, { name: "ignored_empty", body: configRespV2{ @@ -83,6 +89,7 @@ func TestHandleStatsV2(t *testing.T) { }, }, wantCode: http.StatusBadRequest, + wantErr: "ignored: host name is empty\n", }} for _, tc := range testCases { @@ -91,7 +98,7 @@ func TestHandleStatsV2(t *testing.T) { conf := Config{ Filename: filepath.Join(t.TempDir(), "stats.db"), - LimitIvl: time.Hour * 24, + Limit: time.Hour * 24, Enabled: true, UnitID: func() (id uint32) { return 0 }, HTTPRegister: func( @@ -106,6 +113,7 @@ func TestHandleStatsV2(t *testing.T) { s, err := New(conf) require.NoError(t, err) + s.Start() testutil.CleanupAndRequireSuccess(t, s.Close) @@ -119,22 +127,27 @@ func TestHandleStatsV2(t *testing.T) { req := httptest.NewRequest(http.MethodPut, configPut, bytes.NewReader(buf)) rw := httptest.NewRecorder() + handlers[configPut].ServeHTTP(rw, req) require.Equal(t, tc.wantCode, rw.Code) - if tc.wantCode == http.StatusOK { - resp := httptest.NewRequest(http.MethodGet, configGet, nil) - rw = httptest.NewRecorder() - handlers[configGet].ServeHTTP(rw, resp) - require.Equal(t, http.StatusOK, rw.Code) - data := rw.Body.Bytes() + if tc.wantCode != http.StatusOK { + assert.Equal(t, tc.wantErr, rw.Body.String()) - ans := configRespV2{} - jerr := json.Unmarshal(data, &ans) - require.NoError(t, jerr) - - assert.Equal(t, tc.body, ans) + return } + + resp := httptest.NewRequest(http.MethodGet, configGet, nil) + rw = httptest.NewRecorder() + + handlers[configGet].ServeHTTP(rw, resp) + require.Equal(t, http.StatusOK, rw.Code) + + ans := configRespV2{} + jerr := json.Unmarshal(rw.Body.Bytes(), &ans) + require.NoError(t, jerr) + + assert.Equal(t, tc.body, ans) }) } } diff --git a/internal/stats/stats.go b/internal/stats/stats.go index 6d40b823..6fa05a54 100644 --- a/internal/stats/stats.go +++ b/internal/stats/stats.go @@ -43,9 +43,9 @@ type Config struct { // Filename is the name of the database file. Filename string - // LimitIvl is the interval for collecting statistics into the current - // unit. - LimitIvl time.Duration + // Limit is an upper limit for collecting statistics into the + // current unit. + Limit time.Duration // Enabled tells if the statistics are enabled. Enabled bool @@ -106,9 +106,9 @@ type StatsCtx struct { // enabled tells if the statistics are enabled. enabled bool - // limitIvl is the interval for collecting statistics into the current - // unit. - limitIvl time.Duration + // limit is an upper limit for collecting statistics into the + // current unit. + limit time.Duration // ignored is the list of host names, which should not be counted. ignored *stringutil.Set @@ -128,15 +128,15 @@ func New(conf Config) (s *StatsCtx, err error) { ignored: conf.Ignored, } - if conf.LimitIvl < time.Hour { + if conf.Limit < time.Hour { return nil, errors.Error("interval: less than an hour") } - if conf.LimitIvl > timeutil.Day*365 { + if conf.Limit > timeutil.Day*365 { return nil, errors.Error("interval: more than a year") } - s.limitIvl = conf.LimitIvl + s.limit = conf.Limit if s.unitIDGen = newUnitID; conf.UnitID != nil { s.unitIDGen = conf.UnitID @@ -157,7 +157,7 @@ func New(conf Config) (s *StatsCtx, err error) { return nil, fmt.Errorf("stats: opening a transaction: %w", err) } - deleted := deleteOldUnits(tx, id-uint32(s.limitIvl.Hours())-1) + deleted := deleteOldUnits(tx, id-uint32(s.limit.Hours())-1) udb = loadUnitFromDB(tx, id) err = finishTxn(tx, deleted > 0) @@ -238,7 +238,7 @@ func (s *StatsCtx) Update(e Entry) { s.lock.Lock() defer s.lock.Unlock() - if !s.enabled || s.limitIvl == 0 { + if !s.enabled || s.limit == 0 { return } @@ -270,7 +270,7 @@ func (s *StatsCtx) WriteDiskConfig(dc *Config) { s.lock.Lock() defer s.lock.Unlock() - dc.LimitIvl = s.limitIvl + dc.Limit = s.limit dc.Enabled = s.enabled dc.Ignored = s.ignored } @@ -280,7 +280,7 @@ func (s *StatsCtx) TopClientsIP(maxCount uint) (ips []netip.Addr) { s.lock.Lock() defer s.lock.Unlock() - limit := uint32(s.limitIvl.Hours()) + limit := uint32(s.limit.Hours()) if !s.enabled || limit == 0 { return nil } @@ -384,7 +384,7 @@ func (s *StatsCtx) flush() (cont bool, sleepFor time.Duration) { return false, 0 } - limit := uint32(s.limitIvl.Hours()) + limit := uint32(s.limit.Hours()) if limit == 0 || ptr.id == id { return true, time.Second } @@ -450,7 +450,7 @@ func (s *StatsCtx) periodicFlush() { func (s *StatsCtx) setLimitLocked(limitDays int) { if limitDays != 0 { s.enabled = true - s.limitIvl = time.Duration(int(timeutil.Day) * limitDays) + s.limit = time.Duration(int(timeutil.Day) * limitDays) log.Debug("stats: set limit: %d days", limitDays) return diff --git a/internal/stats/stats_internal_test.go b/internal/stats/stats_internal_test.go index 11b1a08d..115805fc 100644 --- a/internal/stats/stats_internal_test.go +++ b/internal/stats/stats_internal_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/AdguardTeam/golibs/testutil" + "github.com/AdguardTeam/golibs/timeutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,7 +38,7 @@ func TestStats_races(t *testing.T) { conf := Config{ UnitID: idGen, Filename: filepath.Join(t.TempDir(), "./stats.db"), - LimitIvl: time.Hour * 24, + Limit: timeutil.Day, } s, err := New(conf) diff --git a/internal/stats/stats_test.go b/internal/stats/stats_test.go index 4c17721f..36f9d102 100644 --- a/internal/stats/stats_test.go +++ b/internal/stats/stats_test.go @@ -9,11 +9,11 @@ import ( "path/filepath" "sync/atomic" "testing" - "time" "github.com/AdguardTeam/AdGuardHome/internal/stats" "github.com/AdguardTeam/golibs/netutil" "github.com/AdguardTeam/golibs/testutil" + "github.com/AdguardTeam/golibs/timeutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -53,7 +53,7 @@ func TestStats(t *testing.T) { handlers := map[string]http.Handler{} conf := stats.Config{ Filename: filepath.Join(t.TempDir(), "stats.db"), - LimitIvl: time.Hour * 24, + Limit: timeutil.Day, Enabled: true, UnitID: constUnitID, HTTPRegister: func(_, url string, handler http.HandlerFunc) { @@ -159,7 +159,7 @@ func TestLargeNumbers(t *testing.T) { conf := stats.Config{ Filename: filepath.Join(t.TempDir(), "stats.db"), - LimitIvl: time.Hour * 24, + Limit: timeutil.Day, Enabled: true, UnitID: func() (id uint32) { return atomic.LoadUint32(&curHour) }, HTTPRegister: func(_, url string, handler http.HandlerFunc) { handlers[url] = handler },