Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Use package local metrics #1605

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: Use package local metrics #1605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cmd/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@ import (
"github.com/0xERR0R/blocky/config"
"github.com/0xERR0R/blocky/evt"
"github.com/0xERR0R/blocky/log"
"github.com/0xERR0R/blocky/metrics"
"github.com/0xERR0R/blocky/server"
"github.com/0xERR0R/blocky/util"

"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/spf13/cobra"
)

Expand All @@ -21,6 +24,13 @@ var (
done = make(chan bool, 1)
isConfigMandatory = true
signals = make(chan os.Signal, 1)

versionInfoMetric = promauto.With(metrics.Reg).NewGaugeVec(
prometheus.GaugeOpts{
Name: "blocky_build_info",
Help: "Version number and build info",
}, []string{"version", "build_time"},
)
)

func newServeCommand() *cobra.Command {
Expand Down Expand Up @@ -76,6 +86,7 @@ func startServer(_ *cobra.Command, _ []string) error {
}()

evt.Bus().Publish(evt.ApplicationStarted, util.Version, util.BuildTime)
versionInfoMetric.WithLabelValues(util.Version, util.BuildTime).Set(1)
<-done

return terminationErr
Expand Down
6 changes: 0 additions & 6 deletions evt/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ const (
// BlockingEnabledEvent fires if blocking status will be changed. Parameter: boolean (enabled = true)
BlockingEnabledEvent = "blocking:enabled"

// BlockingCacheGroupChanged fires, if a list group is changed. Parameter: list type, group name, element count
BlockingCacheGroupChanged = "blocking:cachingGroupChanged"

// CachingDomainPrefetched fires if a domain will be prefetched, Parameter: domain name
CachingDomainPrefetched = "caching:prefetched"

Expand All @@ -23,9 +20,6 @@ const (
// CachingDomainsToPrefetchCountChanged fires, if a number of domains being prefetched changed, Parameter: new count
CachingDomainsToPrefetchCountChanged = "caching:domainsToPrefetchCountChanged"

// CachingFailedDownloadChanged fires, if a download of a blocking list or hosts file fails
CachingFailedDownloadChanged = "caching:failedDownload"

// ApplicationStarted fires on start of the application. Parameter: version number, build time
ApplicationStarted = "application:started"
)
Expand Down
21 changes: 15 additions & 6 deletions lists/downloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,21 @@ import (
"net/http"

"github.com/0xERR0R/blocky/config"
"github.com/0xERR0R/blocky/evt"
"github.com/0xERR0R/blocky/metrics"

"github.com/avast/retry-go/v4"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
)

//nolint:gochecknoglobals
var (
failedDownloadsTotal = promauto.With(metrics.Reg).NewCounter(
prometheus.CounterOpts{
Name: "blocky_failed_downloads_total",
Help: "Failed download counter",
},
)
)

// TransientError represents a temporary error like timeout, network errors...
Expand Down Expand Up @@ -105,12 +118,8 @@ func (d *httpDownloader) DownloadFile(ctx context.Context, link string) (io.Read
logger.Warnf("Can't download file: %s", err)
}

onDownloadError(link)
failedDownloadsTotal.Inc()
}))

return body, err
}

func onDownloadError(link string) {
evt.Bus().Publish(evt.CachingFailedDownloadChanged, link)
}
34 changes: 3 additions & 31 deletions lists/downloader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"time"

"github.com/0xERR0R/blocky/config"
. "github.com/0xERR0R/blocky/evt"
. "github.com/0xERR0R/blocky/helpertest"
"github.com/0xERR0R/blocky/log"
. "github.com/onsi/ginkgo/v2"
Expand All @@ -22,27 +21,16 @@ import (

var _ = Describe("Downloader", func() {
var (
sutConfig config.Downloader
sut *httpDownloader
failedDownloadCountEvtChannel chan string
loggerHook *test.Hook
sutConfig config.Downloader
sut *httpDownloader
loggerHook *test.Hook
)
BeforeEach(func() {
var err error

sutConfig, err = config.WithDefaults[config.Downloader]()
Expect(err).Should(Succeed())

failedDownloadCountEvtChannel = make(chan string, 5)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now we don't test if metrics counters are being incremented. Maybe we can try to read the counter value and assert it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is something we could test for. But I find it not very useful. The Prometheus client Inc() type functions are basically a simple Go atomic increment. There's basically no way for it to go wrong in a way that testing would help with.

// collect received events in the channel
fn := func(url string) {
failedDownloadCountEvtChannel <- url
}
Expect(Bus().Subscribe(CachingFailedDownloadChanged, fn)).Should(Succeed())
DeferCleanup(func() {
Expect(Bus().Unsubscribe(CachingFailedDownloadChanged, fn)).Should(Succeed())
})

loggerHook = test.NewGlobal()
log.Log().AddHook(loggerHook)
DeferCleanup(loggerHook.Reset)
Expand Down Expand Up @@ -106,8 +94,6 @@ var _ = Describe("Downloader", func() {
Expect(err).Should(HaveOccurred())
Expect(reader).Should(BeNil())
Expect(err.Error()).Should(Equal("got status code 404"))
Expect(failedDownloadCountEvtChannel).Should(HaveLen(3))
Expect(failedDownloadCountEvtChannel).Should(Receive(Equal(server.URL)))
})
})
When("Wrong URL is defined", func() {
Expand All @@ -119,9 +105,6 @@ var _ = Describe("Downloader", func() {

Expect(err).Should(HaveOccurred())
Expect(loggerHook.LastEntry().Message).Should(ContainSubstring("Can't download file: "))
// failed download event was emitted only once
Expect(failedDownloadCountEvtChannel).Should(HaveLen(1))
Expect(failedDownloadCountEvtChannel).Should(Receive(Equal("somewrongurl")))
})
})

Expand Down Expand Up @@ -158,9 +141,6 @@ var _ = Describe("Downloader", func() {
Expect(err).Should(Succeed())
Expect(buf.String()).Should(Equal("blocked1.com"))

// failed download event was emitted only once
Expect(failedDownloadCountEvtChannel).Should(HaveLen(1))
Expect(failedDownloadCountEvtChannel).Should(Receive(Equal(server.URL)))
Expect(loggerHook.LastEntry().Message).Should(ContainSubstring("Temporary network err / Timeout occurred: "))
})
})
Expand All @@ -184,10 +164,6 @@ var _ = Describe("Downloader", func() {
Expect(errors.As(err, new(*TransientError))).Should(BeTrue())
Expect(err.Error()).Should(ContainSubstring("Timeout"))
Expect(reader).Should(BeNil())

// failed download event was emitted 3 times
Expect(failedDownloadCountEvtChannel).Should(HaveLen(3))
Expect(failedDownloadCountEvtChannel).Should(Receive(Equal(server.URL)))
})
})
When("DNS resolution of passed URL fails", func() {
Expand All @@ -206,10 +182,6 @@ var _ = Describe("Downloader", func() {
var dnsError *net.DNSError
Expect(errors.As(err, &dnsError)).Should(BeTrue(), "received error %w", err)
Expect(reader).Should(BeNil())

// failed download event was emitted 3 times
Expect(failedDownloadCountEvtChannel).Should(HaveLen(3))
Expect(failedDownloadCountEvtChannel).Should(Receive(Equal("http://some.domain.which.does.not.exist")))
Expect(loggerHook.LastEntry().Message).Should(ContainSubstring("Name resolution err: "))
})
})
Expand Down
44 changes: 40 additions & 4 deletions lists/list_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,48 @@
"errors"
"fmt"
"net"

"github.com/sirupsen/logrus"
"time"

"github.com/0xERR0R/blocky/cache/stringcache"
"github.com/0xERR0R/blocky/config"
"github.com/0xERR0R/blocky/evt"
"github.com/0xERR0R/blocky/lists/parsers"
"github.com/0xERR0R/blocky/log"
"github.com/0xERR0R/blocky/metrics"

"github.com/ThinkChaos/parcour"
"github.com/ThinkChaos/parcour/jobgroup"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/sirupsen/logrus"
)

const (
groupProducersBufferCap = 1000
regexWarningThreshold = 500
)

//nolint:gochecknoglobals
var (
lastListGroupRefreshTimestamp = promauto.With(metrics.Reg).NewGauge(
prometheus.GaugeOpts{
Name: "blocky_last_list_group_refresh_timestamp_seconds",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the same definition in metrics_event_publisher.go?

Help: "Timestamp of last list refresh",
},
)
denylistEntries = promauto.With(metrics.Reg).NewGaugeVec(
prometheus.GaugeOpts{
Name: "blocky_denylist_cache_entries",
Help: "Number of entries in the denylist cache",
}, []string{"group"},
)
allowlistEntries = promauto.With(metrics.Reg).NewGaugeVec(
prometheus.GaugeOpts{
Name: "blocky_allowlist_cache_entries",
Help: "Number of entries in the allowlist cache",
}, []string{"group"},
)
)

// ListCacheType represents the type of cached list ENUM(
// denylist // is a list with blocked domains
// allowlist // is a list with allowlisted domains / IPs
Expand Down Expand Up @@ -144,7 +169,7 @@

count := b.groupedCache.ElementCount(group)

evt.Bus().Publish(evt.BlockingCacheGroupChanged, b.listType, group, count)
updateGroupMetrics(b.listType, group, count)

logger().WithFields(logrus.Fields{
"group": group,
Expand Down Expand Up @@ -277,3 +302,14 @@

return nil
}

func updateGroupMetrics(listType ListCacheType, group string, count int) {
lastListGroupRefreshTimestamp.Set(float64(time.Now().Unix()))

switch listType {
case ListCacheTypeDenylist:
denylistEntries.WithLabelValues(group).Set(float64(count))
case ListCacheTypeAllowlist:
allowlistEntries.WithLabelValues(group).Set(float64(count))

Check warning on line 313 in lists/list_cache.go

View check run for this annotation

Codecov / codecov/patch

lists/list_cache.go#L312-L313 

Added lines #L312 - L313 were not covered by tests
}
}
20 changes: 0 additions & 20 deletions lists/list_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"strings"

"github.com/0xERR0R/blocky/config"
. "github.com/0xERR0R/blocky/evt"
"github.com/0xERR0R/blocky/lists/parsers"
"github.com/0xERR0R/blocky/log"
"github.com/google/uuid"
Expand Down Expand Up @@ -251,25 +250,6 @@ var _ = Describe("ListCache", func() {
Expect(group).Should(ContainElement("gr2"))
})
})
When("List will be updated", func() {
resultCnt := 0

BeforeEach(func() {
lists = map[string][]config.BytesSource{
"gr1": config.NewBytesSources(server1.URL),
}

_ = Bus().SubscribeOnce(BlockingCacheGroupChanged, func(listType ListCacheType, group string, cnt int) {
resultCnt = cnt
})
})

It("event should be fired and contain count of elements in downloaded lists", func() {
group := sut.Match("blocked1.com", []string{})
Expect(group).Should(BeEmpty())
Expect(resultCnt).Should(Equal(3))
})
})
When("multiple groups are passed", func() {
BeforeEach(func() {
lists = map[string][]config.BytesSource{
Expand Down
Loading