From 13afce5b2b27e21184c14646da8041f6bfab0fe8 Mon Sep 17 00:00:00 2001 From: Roberto Santalla Date: Tue, 22 Aug 2023 14:32:05 +0200 Subject: [PATCH] agent/metrics: allow preinitializing supporting metrics to zero --- pkg/agent/protocol/grpc/proxy.go | 6 +++- pkg/agent/protocol/grpc/proxy_test.go | 48 +++++++++++++++++++-------- pkg/agent/protocol/http/proxy.go | 16 +++++++-- pkg/agent/protocol/http/proxy_test.go | 14 +++++--- pkg/agent/protocol/metricmap.go | 20 ++++++++--- pkg/agent/protocol/metricmap_test.go | 28 ++++++++++++---- pkg/agent/protocol/protocol.go | 3 +- 7 files changed, 102 insertions(+), 33 deletions(-) diff --git a/pkg/agent/protocol/grpc/proxy.go b/pkg/agent/protocol/grpc/proxy.go index 3074de49..c3a42de2 100644 --- a/pkg/agent/protocol/grpc/proxy.go +++ b/pkg/agent/protocol/grpc/proxy.go @@ -80,7 +80,11 @@ func NewProxy(c ProxyConfig, d Disruption) (protocol.Proxy, error) { return &proxy{ disruption: d, config: c, - metrics: &protocol.MetricMap{}, + metrics: protocol.NewMetricMap( + protocol.MetricRequests, + protocol.MetricRequestsExcluded, + protocol.MetricRequestsDisrupted, + ), }, nil } diff --git a/pkg/agent/protocol/grpc/proxy_test.go b/pkg/agent/protocol/grpc/proxy_test.go index 8a4bb463..93d48760 100644 --- a/pkg/agent/protocol/grpc/proxy_test.go +++ b/pkg/agent/protocol/grpc/proxy_test.go @@ -341,11 +341,28 @@ func Test_ProxyMetrics(t *testing.T) { type TestCase struct { title string disruption Disruption + skipRequest bool expectedMetrics map[string]uint } // TODO: Add test for excluded endpoints testCases := []TestCase{ + { + title: "no requests", + disruption: Disruption{ + AverageDelay: 0, + DelayVariation: 0, + ErrorRate: 0.0, + StatusCode: 0, + StatusMessage: "", + }, + skipRequest: true, + expectedMetrics: map[string]uint{ + protocol.MetricRequests: 0, + protocol.MetricRequestsDisrupted: 0, + protocol.MetricRequestsExcluded: 0, + }, + }, { title: "passthrough", disruption: Disruption{ @@ -356,7 +373,9 @@ func Test_ProxyMetrics(t *testing.T) { StatusMessage: "", }, expectedMetrics: map[string]uint{ - protocol.MetricRequests: 1, + protocol.MetricRequests: 1, + protocol.MetricRequestsDisrupted: 0, + protocol.MetricRequestsExcluded: 0, }, }, { @@ -371,6 +390,7 @@ func Test_ProxyMetrics(t *testing.T) { expectedMetrics: map[string]uint{ protocol.MetricRequests: 1, protocol.MetricRequestsDisrupted: 1, + protocol.MetricRequestsExcluded: 0, }, }, } @@ -435,18 +455,20 @@ func Test_ProxyMetrics(t *testing.T) { _ = conn.Close() }() - client := ping.NewPingServiceClient(conn) - - var headers metadata.MD - _, _ = client.Ping( - context.TODO(), - &ping.PingRequest{ - Error: 0, - Message: "ping", - }, - grpc.Header(&headers), - grpc.WaitForReady(true), - ) + if !tc.skipRequest { + client := ping.NewPingServiceClient(conn) + + var headers metadata.MD + _, _ = client.Ping( + context.TODO(), + &ping.PingRequest{ + Error: 0, + Message: "ping", + }, + grpc.Header(&headers), + grpc.WaitForReady(true), + ) + } metrics := proxy.Metrics() diff --git a/pkg/agent/protocol/http/proxy.go b/pkg/agent/protocol/http/proxy.go index df78ecf5..75479609 100644 --- a/pkg/agent/protocol/http/proxy.go +++ b/pkg/agent/protocol/http/proxy.go @@ -44,7 +44,7 @@ type proxy struct { config ProxyConfig disruption Disruption srv *http.Server - metrics protocol.MetricMap + metrics *protocol.MetricMap } // NewProxy return a new Proxy for HTTP requests @@ -72,6 +72,7 @@ func NewProxy(c ProxyConfig, d Disruption) (protocol.Proxy, error) { return &proxy{ disruption: d, config: c, + metrics: protocol.NewMetricMap(supportedMetrics()...), }, nil } @@ -176,7 +177,7 @@ func (p *proxy) Start() error { handler := &httpHandler{ upstreamURL: *upstreamURL, disruption: p.disruption, - metrics: &p.metrics, + metrics: p.metrics, } p.srv = &http.Server{ @@ -211,3 +212,14 @@ func (p *proxy) Force() error { } return nil } + +// supportedMetrics is a helper function that returns the metrics that the http proxy supports and thus should be +// pre-initialized to zero. This function is defined due to the testing limitations mentioned in +// https://github.com/grafana/xk6-disruptor/issues/314, as httpHandler tests currently need this information. +func supportedMetrics() []string { + return []string{ + protocol.MetricRequests, + protocol.MetricRequestsExcluded, + protocol.MetricRequestsDisrupted, + } +} diff --git a/pkg/agent/protocol/http/proxy_test.go b/pkg/agent/protocol/http/proxy_test.go index 079b5260..59dad2d9 100644 --- a/pkg/agent/protocol/http/proxy_test.go +++ b/pkg/agent/protocol/http/proxy_test.go @@ -335,7 +335,7 @@ func Test_ProxyHandler(t *testing.T) { handler := &httpHandler{ upstreamURL: *upstreamURL, disruption: tc.disruption, - metrics: &protocol.MetricMap{}, + metrics: protocol.NewMetricMap(supportedMetrics()...), } proxyServer := httptest.NewServer(handler) @@ -388,8 +388,12 @@ func Test_Metrics(t *testing.T) { expectedMetrics map[string]uint }{ { - name: "no requests", - expectedMetrics: map[string]uint{}, + name: "no requests", + expectedMetrics: map[string]uint{ + protocol.MetricRequests: 0, + protocol.MetricRequestsExcluded: 0, + protocol.MetricRequestsDisrupted: 0, + }, }, { name: "requests", @@ -419,12 +423,12 @@ func Test_Metrics(t *testing.T) { t.Fatalf("error parsing httptest url") } - metrics := protocol.MetricMap{} + metrics := protocol.NewMetricMap(supportedMetrics()...) handler := &httpHandler{ upstreamURL: *upstreamURL, disruption: tc.config, - metrics: &metrics, + metrics: metrics, } proxyServer := httptest.NewServer(handler) diff --git a/pkg/agent/protocol/metricmap.go b/pkg/agent/protocol/metricmap.go index 68c7a0fc..71377b01 100644 --- a/pkg/agent/protocol/metricmap.go +++ b/pkg/agent/protocol/metricmap.go @@ -8,15 +8,25 @@ type MetricMap struct { mutex sync.RWMutex } -// Inc increases the value of the specified counter by one. +// NewMetricMap returns a MetricMap with the specified metrics initialized to zero. +func NewMetricMap(metrics ...string) *MetricMap { + mm := &MetricMap{ + metrics: map[string]uint{}, + } + + for _, metric := range metrics { + mm.metrics[metric] = 0 + } + + return mm +} + +// Inc increases the value of the specified counter by one. If the metric hasn't been initialized or incremented before, +// it is set to 1. func (m *MetricMap) Inc(name string) { m.mutex.Lock() defer m.mutex.Unlock() - if m.metrics == nil { - m.metrics = make(map[string]uint) - } - m.metrics[name]++ } diff --git a/pkg/agent/protocol/metricmap_test.go b/pkg/agent/protocol/metricmap_test.go index 0e024557..ace9eb48 100644 --- a/pkg/agent/protocol/metricmap_test.go +++ b/pkg/agent/protocol/metricmap_test.go @@ -9,18 +9,34 @@ import ( func TestMetricMap(t *testing.T) { t.Parallel() + t.Run("initializes metrics", func(t *testing.T) { + t.Parallel() + + const foo = "foo_metric" + + mm := protocol.NewMetricMap(foo) + fooMetric, hasFoo := mm.Map()[foo] + if !hasFoo { + t.Fatalf("foo should exist in the output map") + } + + if fooMetric != 0 { + t.Fatalf("foo should be zero") + } + }) + t.Run("increases counters", func(t *testing.T) { t.Parallel() - const name = "foo_metric" + const foo = "foo_metric" - mm := protocol.MetricMap{} - if current := mm.Map(); current[name] != 0 { - t.Fatalf("map should start containing zero") + mm := protocol.NewMetricMap() + if current := mm.Map(); current[foo] != 0 { + t.Fatalf("uninitialized foo should be zero") } - mm.Inc(name) - if updated := mm.Map(); updated[name] != 1 { + mm.Inc(foo) + if updated := mm.Map(); updated[foo] != 1 { t.Fatalf("metric was not incremented") } }) diff --git a/pkg/agent/protocol/protocol.go b/pkg/agent/protocol/protocol.go index 1048d954..1425e29a 100644 --- a/pkg/agent/protocol/protocol.go +++ b/pkg/agent/protocol/protocol.go @@ -30,7 +30,8 @@ type Proxy interface { Start() error Stop() error // Metrics returns a map of counter-type metrics. Implementations may return zero or more of the metrics defined - // below, as well as any number of implementation-defined metrics. + // below, as well as any number of implementation-defined metrics. Callers can check if a metric exists in the map + // returned by Metrics to distinguish a counter with a value of zero from an unsupported metric. Metrics() map[string]uint Force() error }