From bbf19e8b506ff1ee7a133ae6ac32a4cea7146bac Mon Sep 17 00:00:00 2001 From: Owen Cabalceta Date: Fri, 20 Sep 2024 10:38:23 -0400 Subject: [PATCH] patch: replace prometheus `WithLabelValues` with `With` - simplify the prometheus code - fix misunderstanding of `WithLabelValues` --- deviceAccess.go | 28 ++++++++++++++-------------- deviceAccess_test.go | 7 ++++--- mocks_test.go | 19 +++++++++++++------ 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/deviceAccess.go b/deviceAccess.go index 5606a4a..191a78a 100644 --- a/deviceAccess.go +++ b/deviceAccess.go @@ -84,19 +84,19 @@ type talariaDeviceAccess struct { logger *zap.Logger } -func (t *talariaDeviceAccess) withFailure(labelValues ...string) prometheus.Counter { +func (t *talariaDeviceAccess) withFailure(reason string) prometheus.Counter { if !t.strict { - return t.withSuccess(labelValues...) + return t.withSuccess(reason) } - return t.wrpMessagesCounter.WithLabelValues(append(labelValues, outcomeLabel, rejected)...) + return t.wrpMessagesCounter.With(prometheus.Labels{reasonLabel: reason, outcomeLabel: rejected}) } -func (t *talariaDeviceAccess) withFatal(labelValues ...string) prometheus.Counter { - return t.wrpMessagesCounter.WithLabelValues(append(labelValues, outcomeLabel, rejected)...) +func (t *talariaDeviceAccess) withFatal(reason string) prometheus.Counter { + return t.wrpMessagesCounter.With(prometheus.Labels{reasonLabel: reason, outcomeLabel: rejected}) } -func (t *talariaDeviceAccess) withSuccess(labelValues ...string) prometheus.Counter { - return t.wrpMessagesCounter.WithLabelValues(append(labelValues, outcomeLabel, accepted)...) +func (t *talariaDeviceAccess) withSuccess(reason string) prometheus.Counter { + return t.wrpMessagesCounter.With(prometheus.Labels{reasonLabel: reason, outcomeLabel: accepted}) } func getRight(check *parsedCheck, wrpCredentials *gojsonq.JSONQ) interface{} { @@ -112,13 +112,13 @@ func getRight(check *parsedCheck, wrpCredentials *gojsonq.JSONQ) interface{} { func (t *talariaDeviceAccess) authorizeWRP(_ context.Context, message *wrp.Message) error { ID, err := device.ParseID(message.Destination) if err != nil { - t.withFatal(reasonLabel, invalidWRPDest).Add(1) + t.withFatal(invalidWRPDest).Add(1) return errInvalidWRPDestination } d, ok := t.deviceRegistry.Get(ID) if !ok { - t.withFatal(reasonLabel, deviceNotFound).Add(1) + t.withFatal(deviceNotFound).Add(1) return errDeviceNotFound } deviceCredentials := gojsonq.New(gojsonq.WithSeparator(t.sep)).FromInterface(d.Metadata().Claims()) @@ -128,7 +128,7 @@ func (t *talariaDeviceAccess) authorizeWRP(_ context.Context, message *wrp.Messa left := deviceCredentials.Reset().Find(c.deviceCredentialPath) if left == nil { - t.withFailure(reasonLabel, missingDeviceCredential).Add(1) + t.withFailure(missingDeviceCredential).Add(1) if t.strict { return errDeviceCredentialMissing } @@ -137,7 +137,7 @@ func (t *talariaDeviceAccess) authorizeWRP(_ context.Context, message *wrp.Messa right := getRight(c, wrpCredentials) if right == nil { - t.withFailure(reasonLabel, missingWRPCredential).Add(1) + t.withFailure(missingWRPCredential).Add(1) if t.strict { return errWRPCredentialsMissing } @@ -153,7 +153,7 @@ func (t *talariaDeviceAccess) authorizeWRP(_ context.Context, message *wrp.Messa ok, err := c.assertion.evaluate(left, right) if err != nil { t.logger.Debug("Check failed to complete", zap.String("check", c.name), zap.Error(err)) - t.withFailure(reasonLabel, incompleteCheck).Add(1) + t.withFailure(incompleteCheck).Add(1) if t.strict { return errIncompleteCheck @@ -163,7 +163,7 @@ func (t *talariaDeviceAccess) authorizeWRP(_ context.Context, message *wrp.Messa if !ok { t.logger.Debug("WRP is unauthorized to reach device", zap.String("check", c.name)) - t.withFailure(reasonLabel, denied).Add(1) + t.withFailure(denied).Add(1) if t.strict { return errDeniedDeviceAccess @@ -173,6 +173,6 @@ func (t *talariaDeviceAccess) authorizeWRP(_ context.Context, message *wrp.Messa } } - t.withSuccess(reasonLabel, authorized).Add(1) + t.withSuccess(authorized).Add(1) return nil } diff --git a/deviceAccess_test.go b/deviceAccess_test.go index fb0775a..217b47e 100644 --- a/deviceAccess_test.go +++ b/deviceAccess_test.go @@ -7,6 +7,7 @@ import ( "errors" "testing" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/xmidt-org/webpa-common/v2/device" @@ -40,7 +41,7 @@ func testAuthorizeWRP(t *testing.T, testCases []deviceAccessTestCase, strict boo mockDevice = new(device.MockDevice) mockBinOp = new(mockBinOp) testLogger = zaptest.NewLogger(t) - counter = mockCounter{labelPairs: make(map[string]string)} + counter = mockCounter{labelPairs: make(map[string]string), labels: []string{reasonLabel, outcomeLabel}} expectedLabels = getLabelMaps(testCase.ExpectedError, testCase.IsFatal, strict, testCase.BaseLabelPairs) wrpMsg = &wrp.Message{ @@ -66,7 +67,7 @@ func testAuthorizeWRP(t *testing.T, testCases []deviceAccessTestCase, strict boo checks = getChecks(t, mockBinOp, testCase.IncompleteCheck, testCase.Authorized) } - counter.On("WithLabelValues", []string{reasonLabel, invalidWRPDest, outcomeLabel, rejected}).Return().Once() + counter.On("With", expectedLabels).Return().Once() counter.On("Add", 1.).Return().Once() deviceAccessAuthority := &talariaDeviceAccess{ strict: strict, @@ -166,7 +167,7 @@ func TestAuthorizeWRP(t *testing.T) { }) } -func getLabelMaps(err error, isFatal, strict bool, baseLabelPairs map[string]string) map[string]string { +func getLabelMaps(err error, isFatal, strict bool, baseLabelPairs map[string]string) prometheus.Labels { out := make(map[string]string) for k, v := range baseLabelPairs { diff --git a/mocks_test.go b/mocks_test.go index d411a32..2624ff8 100644 --- a/mocks_test.go +++ b/mocks_test.go @@ -139,7 +139,8 @@ type mockCounter struct { // port over testCounter functionality count float64 - labelPairs map[string]string + labels []string + labelPairs prometheus.Labels } func (m *mockCounter) Add(delta float64) { @@ -149,8 +150,8 @@ func (m *mockCounter) Add(delta float64) { func (m *mockCounter) Inc() {} -func (m *mockCounter) With(labelValues prometheus.Labels) prometheus.Counter { - for k, v := range labelValues { +func (m *mockCounter) With(labelPairs prometheus.Labels) prometheus.Counter { + for k, v := range labelPairs { if !utf8.ValidString(k) { panic(fmt.Sprintf("key `%s`, value `%s`: key is not UTF-8", k, v)) } else if !utf8.ValidString(v) { @@ -158,7 +159,9 @@ func (m *mockCounter) With(labelValues prometheus.Labels) prometheus.Counter { } } - m.Called(labelValues) + m.labelPairs = labelPairs + m.Called(labelPairs) + return m } @@ -180,8 +183,12 @@ func (m *mockCounter) MustCurryWith(labels prometheus.Labels) (c *prometheus.Cou func (m *mockCounter) WithLabelValues(lvs ...string) (c prometheus.Counter) { // port over testCounter functionality - for i := 0; i < len(lvs)-1; i += 2 { - m.labelPairs[lvs[i]] = lvs[i+1] + if len(lvs) != len(m.labels) { + panic(fmt.Sprintf("expected %d label values but got %d", len(m.labels), len(lvs))) + } + + for i, l := range m.labels { + m.labelPairs[l] = lvs[i] } return m