Skip to content

Commit

Permalink
remove extra logs and fix test
Browse files Browse the repository at this point in the history
  • Loading branch information
wonderflow committed Dec 25, 2024
1 parent dd12883 commit 96b4b3d
Show file tree
Hide file tree
Showing 7 changed files with 18 additions and 16 deletions.
4 changes: 0 additions & 4 deletions api/pkg/filtermanager/api_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 5 additions & 3 deletions api/pkg/filtermanager/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"reflect"
"sort"
"strings"
"sync"

xds "github.com/cncf/xds/go/xds/type/v3"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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, ", "))

Check warning on line 201 in api/pkg/filtermanager/config.go

View check run for this annotation

Codecov / codecov/patch

api/pkg/filtermanager/config.go#L192-L201

Added lines #L192 - L201 were not covered by tests
}

// No configuration
Expand Down
2 changes: 1 addition & 1 deletion api/pkg/filtermanager/filtermanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 1 addition & 5 deletions api/plugins/tests/integration/controlplane/control_plane.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down
7 changes: 7 additions & 0 deletions api/plugins/tests/pkg/envoy/capi.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion api/tests/integration/filtermanager_latest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
}
4 changes: 2 additions & 2 deletions api/tests/integration/test_plugins.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
}

0 comments on commit 96b4b3d

Please sign in to comment.