From 96b4b3d6577a52c70dfb2f9732bbd5b740bbfbe7 Mon Sep 17 00:00:00 2001 From: Jianbo Sun Date: Tue, 24 Dec 2024 15:52:10 +0800 Subject: [PATCH] remove extra logs and fix test --- api/pkg/filtermanager/api_impl.go | 4 ---- api/pkg/filtermanager/config.go | 8 +++++--- api/pkg/filtermanager/filtermanager.go | 2 +- .../tests/integration/controlplane/control_plane.go | 6 +----- api/plugins/tests/pkg/envoy/capi.go | 7 +++++++ api/tests/integration/filtermanager_latest_test.go | 3 ++- api/tests/integration/test_plugins.go | 4 ++-- 7 files changed, 18 insertions(+), 16 deletions(-) diff --git a/api/pkg/filtermanager/api_impl.go b/api/pkg/filtermanager/api_impl.go index 5d2ed1d1..44ea830d 100644 --- a/api/pkg/filtermanager/api_impl.go +++ b/api/pkg/filtermanager/api_impl.go @@ -215,10 +215,6 @@ func (cb *filterManagerCallbackHandler) GetCounterMetrics(pluginName, metricName api.LogErrorf("metrics not exist or not initialized for plugin %s", pluginName) return nil } - api.LogInfo("[metrics] printing:") - for k, v := range cb.metrics { - api.LogInfof("[metrics] %s: %v", k, v) - } writer, ok := cb.metrics[pluginName] if !ok { api.LogErrorf("metrics writer for plugin %s not found", pluginName) diff --git a/api/pkg/filtermanager/config.go b/api/pkg/filtermanager/config.go index 44a150a8..5baed145 100644 --- a/api/pkg/filtermanager/config.go +++ b/api/pkg/filtermanager/config.go @@ -20,6 +20,7 @@ import ( "fmt" "reflect" "sort" + "strings" "sync" xds "github.com/cncf/xds/go/xds/type/v3" @@ -96,7 +97,6 @@ func (conf *filterManagerConfig) Merge(another *filterManagerConfig) *filterMana } // Pass LDS metrics writers to the merged config for golang filter to use at route level - capi.LogInfof("[metrics] merging http filter, filtermanager config: %+v", another.metricsWriters) conf.metricsWriters = another.metricsWriters // It's tough to do the data plane merge right. We don't use shallow copy, which may share @@ -192,11 +192,13 @@ func (p *FilterManagerConfigParser) Parse(any *anypb.Any, callbacks capi.ConfigC // If callbacks is not nil, it means this filter is configured in the LDS level. // We need to initialize the metrics for all golang plugins here. registers := plugins.GetMetricsDefinitions() + registeredPlugins := []string{} for pluginName, register := range registers { - api.LogInfof("initializing metrics for plugin %s", pluginName) + api.LogInfof("registering metrics for golang plugin %s", pluginName) metricsWriters[pluginName] = register(callbacks) + registeredPlugins = append(registeredPlugins, pluginName) } - capi.LogInfof("[metrics] initialized http filter, filtermanager config: %+v", metricsWriters) + capi.LogInfof("metrics registered for plugins: [%s]", strings.Join(registeredPlugins, ", ")) } // No configuration diff --git a/api/pkg/filtermanager/filtermanager.go b/api/pkg/filtermanager/filtermanager.go index 241be929..69c19540 100644 --- a/api/pkg/filtermanager/filtermanager.go +++ b/api/pkg/filtermanager/filtermanager.go @@ -152,7 +152,7 @@ func FilterManagerFactory(c interface{}, cb capi.FilterCallbackHandler) (streamF } fm.callbacks.FilterCallbackHandler = cb - capi.LogInfof("[metrics] filter manager metrics Writers %v", conf.metricsWriters) + fm.callbacks.metrics = conf.metricsWriters canSkipMethods := fm.canSkipMethods diff --git a/api/plugins/tests/integration/controlplane/control_plane.go b/api/plugins/tests/integration/controlplane/control_plane.go index 60ba1aee..1434e54b 100644 --- a/api/plugins/tests/integration/controlplane/control_plane.go +++ b/api/plugins/tests/integration/controlplane/control_plane.go @@ -165,14 +165,10 @@ func (cp *ControlPlane) UseGoPluginConfig(t *testing.T, config *filtermanager.Fi }, } if config != nil { - pluginName := os.Getenv("plugin_name_for_test") - if pluginName == "" { - pluginName = "fm" - } testRoute.TypedPerFilterConfig = map[string]*any1.Any{ "htnn.filters.http.golang": proto.MessageToAny(&golang.ConfigsPerRoute{ PluginsConfig: map[string]*golang.RouterPlugin{ - pluginName: { + "fm": { Override: &golang.RouterPlugin_Config{ Config: proto.MessageToAny( FilterManagerConfigToTypedStruct(config)), diff --git a/api/plugins/tests/pkg/envoy/capi.go b/api/plugins/tests/pkg/envoy/capi.go index e26d7328..ed10d146 100644 --- a/api/plugins/tests/pkg/envoy/capi.go +++ b/api/plugins/tests/pkg/envoy/capi.go @@ -583,6 +583,13 @@ func (i *filterCallbackHandler) PluginState() api.PluginState { return i.pluginState } +func (i *filterCallbackHandler) GetCounterMetrics(pluginName, metricsName string) capi.CounterMetric { + return nil +} +func (i *filterCallbackHandler) GetGaugeMetrics(pluginName, metricsName string) capi.GaugeMetric { + return nil +} + func (i *filterCallbackHandler) WithLogArg(key string, value any) api.StreamFilterCallbacks { return i } diff --git a/api/tests/integration/filtermanager_latest_test.go b/api/tests/integration/filtermanager_latest_test.go index a3c6ecba..033d2dd3 100644 --- a/api/tests/integration/filtermanager_latest_test.go +++ b/api/tests/integration/filtermanager_latest_test.go @@ -377,6 +377,7 @@ func TestMetricsEnabledPlugin(t *testing.T) { lines := strings.Split(string(body), "\n") var found int + for _, l := range lines { if !strings.Contains(l, "metrics-test") { continue @@ -390,5 +391,5 @@ func TestMetricsEnabledPlugin(t *testing.T) { assert.Contains(t, "metrics-test.usage.gauge: 2", l) } } - assert.Equal(t, 1, found, "expect to have metrics usage.counter and usage.gauge") + assert.Equal(t, 2, found, "expect to have metrics usage.counter and usage.gauge") } diff --git a/api/tests/integration/test_plugins.go b/api/tests/integration/test_plugins.go index a105afb6..9ac0b005 100644 --- a/api/tests/integration/test_plugins.go +++ b/api/tests/integration/test_plugins.go @@ -652,7 +652,7 @@ type metricsFilter struct { } const metricsUsageCounter = "metrics-test.usage.counter" -const metricsGauge = "metrics-test.usage.guage" +const metricsGauge = "metrics-test.usage.gauge" func RegisterMetrics(c capi.ConfigCallbacks) plugins.MetricsWriter { writer := plugins.MetricsWriter{ @@ -699,5 +699,5 @@ func init() { plugins.RegisterPlugin("metrics", mp) // register metrics definition for plugin "metrics" plugins.RegisterMetricsDefinitions("metrics", RegisterMetrics) - // TODO(wonderflow): allow metrics to contains runtime information especially for listener name + // TODO(wonderflow): allow metrics to contains runtime information especially for listener name, this require support from envoy upstream: https://github.com/envoyproxy/envoy/issues/37808 }