diff --git a/monitoring/helpers.go b/monitoring/helpers.go index b5b3efa..2ccaf08 100644 --- a/monitoring/helpers.go +++ b/monitoring/helpers.go @@ -41,12 +41,30 @@ func (ms *MetricsSetup) get_metrics_name(name string, labels map[string]string) return buf.String() } +const unknownPodName = "unknown" + +var ( + // podNameOnce guards the one-time resolution of the local hostname. + // os.Hostname() is a syscall; resolving it on every metrics call (which + // happens several times per request) is wasteful since the pod name + // never changes for the lifetime of the process. + podNameOnce sync.Once + podNameValue string + // hostnameFunc resolves the local hostname. It is a package var (rather + // than a direct os.Hostname() call) so tests can substitute a counting + // stub and verify podNameOnce caches the result after the first call. + hostnameFunc = os.Hostname +) + func getPodName() string { - const unknownPodName = "unknown" - if hn, err := os.Hostname(); err == nil { - return hn - } - return unknownPodName + podNameOnce.Do(func() { + if hn, err := hostnameFunc(); err == nil { + podNameValue = hn + } else { + podNameValue = unknownPodName + } + }) + return podNameValue } func defaultLabels(podName string) map[string]string { @@ -144,13 +162,6 @@ func getSortedKeys(labels map[string]string) []string { return []string{} } - labelsKey := labelsToString(labels) - - // Check if the sorted keys are already cached - if keys, ok := sortedLabelKeysCache.m.Load(labelsKey); ok { - return keys.([]string) - } - // Compute the sorted keys - create a snapshot to avoid concurrent access issues keys := make([]string, 0, len(labels)) for k := range labels { @@ -158,8 +169,20 @@ func getSortedKeys(labels map[string]string) []string { } sort.Strings(keys) - // Store the sorted keys in the cache - sortedLabelKeysCache.m.Store(labelsKey, keys) + // The cache is keyed by the sorted set of label NAMES only, not values. + // The cached payload (the sorted key list) never depends on the label + // values, so keying by value (as before) created a separate, never- + // evicted cache entry per distinct value combination - an unbounded + // cardinality bomb. Keying by names collapses all maps sharing the same + // key set into a single entry, bounding growth to the number of + // distinct label-name sets used by the application. + namesKey := strings.Join(keys, ";") + + if cached, ok := sortedLabelKeysCache.m.Load(namesKey); ok { + return cached.([]string) + } + + sortedLabelKeysCache.m.Store(namesKey, keys) return keys } diff --git a/monitoring/helpers_test.go b/monitoring/helpers_test.go index f3f0981..c299a0b 100644 --- a/monitoring/helpers_test.go +++ b/monitoring/helpers_test.go @@ -1,10 +1,14 @@ package libpack_monitoring import ( + "strings" + "sync" + "sync/atomic" "testing" libpack_config "github.com/lukaszraczylo/graphql-monitoring-proxy/config" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGetMetricsName(t *testing.T) { @@ -201,6 +205,75 @@ func TestEnsureDefaultLabels(t *testing.T) { } } +// TestGetPodName_HostnameResolvedOnce verifies getPodName() resolves the +// hostname exactly once (via podNameOnce) no matter how many times it is +// called, including concurrently. Repeated os.Hostname() syscalls on every +// metrics call were the dominant per-request cost this caching removes. +func TestGetPodName_HostnameResolvedOnce(t *testing.T) { + origFn := hostnameFunc + t.Cleanup(func() { + // Restore the real hostname resolver and reset to a fresh, unfired + // Once (never copy an existing sync.Once - it must not be copied by + // value once it may have been used). The next getPodName() call will + // re-resolve the real hostname and re-cache it for later tests. + hostnameFunc = origFn + podNameOnce = sync.Once{} + }) + + podNameOnce = sync.Once{} + var callCount int32 + hostnameFunc = func() (string, error) { + atomic.AddInt32(&callCount, 1) + return "cached-test-host", nil + } + + // Sequential calls must all observe the cached value. + first := getPodName() + second := getPodName() + assert.Equal(t, "cached-test-host", first) + assert.Equal(t, first, second) + assert.Equal(t, int32(1), atomic.LoadInt32(&callCount), "hostnameFunc must be invoked exactly once") + + // Concurrent calls must not re-invoke hostnameFunc either. + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + assert.Equal(t, "cached-test-host", getPodName()) + }() + } + wg.Wait() + + assert.Equal(t, int32(1), atomic.LoadInt32(&callCount), "hostnameFunc must still be invoked exactly once after concurrent calls") +} + +// TestGetSortedKeysCache_SharedAcrossValues verifies the sortedLabelKeysCache +// is keyed by the sorted set of label NAMES only. Two maps sharing the same +// keys but different values must collapse into a single cache entry rather +// than growing the cache per distinct value combination (unbounded +// cardinality bomb). +func TestGetSortedKeysCache_SharedAcrossValues(t *testing.T) { + labelsA := map[string]string{"cache_key_test_a": "value1", "cache_key_test_b": "value2"} + labelsB := map[string]string{"cache_key_test_a": "totally-different", "cache_key_test_b": "also-different"} + + keysA := getSortedKeys(labelsA) + keysB := getSortedKeys(labelsB) + + require.Equal(t, []string{"cache_key_test_a", "cache_key_test_b"}, keysA) + require.Equal(t, keysA, keysB) + + // Same underlying slice (same cache entry) must be returned for both + // maps, proving the cache did not create a second, value-keyed entry. + assert.Same(t, &keysA[0], &keysB[0]) + + // The cache entry itself must be reachable only via a names-only key. + namesKey := strings.Join(keysA, ";") + cached, ok := sortedLabelKeysCache.m.Load(namesKey) + require.True(t, ok) + assert.Equal(t, keysA, cached) +} + func TestLabelsToString(t *testing.T) { tests := []struct { labels map[string]string diff --git a/monitoring/monitoring.go b/monitoring/monitoring.go index dbddee3..c760436 100644 --- a/monitoring/monitoring.go +++ b/monitoring/monitoring.go @@ -81,7 +81,12 @@ func (ms *MetricsSetup) startPrometheusEndpoint() { AppName: fmt.Sprintf("GraphQL Monitoring Proxy - %s v%s", libpack_config.PKG_NAME, libpack_config.PKG_VERSION), }) app.Get("/metrics", ms.metricsEndpoint) - if err := app.Listen(fmt.Sprintf(":%d", envutil.GetInt("MONITORING_PORT", 9393)), fiber.ListenConfig{DisableStartupMessage: true}); err != nil { + // S6: bind address for the monitoring listener. This package does not + // import the main package's config, so it reads BIND_ADDRESS directly, + // the same way it already reads MONITORING_PORT. Empty default preserves + // today's "bind all interfaces" behavior (":port"). + bindAddress := envutil.Getenv("BIND_ADDRESS", "") + if err := app.Listen(fmt.Sprintf("%s:%d", bindAddress, envutil.GetInt("MONITORING_PORT", 9393)), fiber.ListenConfig{DisableStartupMessage: true, GracefulContext: ms.ctx}); err != nil { log.Critical(&libpack_logger.LogMessage{ Message: "Can't start the MONITORING service", Pairs: map[string]any{"error": err},