From 488bbaacab2509f6f9f03d173de500ffbf57a757 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Fri, 22 Mar 2024 12:54:35 +0100 Subject: [PATCH] Chore: Remove unused dependenices in plugin client middlewares (#84624) * Chore: Remove unused dependenices in plugin client middlewares * refactor logger middleware to remove cfg dependency * hack to make tracing work in api group builders --- pkg/cmd/grafana/apiserver/cmd.go | 29 ++++++++++++++++++- pkg/cmd/grafana/apiserver/server.go | 5 ++-- pkg/services/apiserver/standalone/factory.go | 10 +++---- .../clientmiddleware/logger_middleware.go | 18 ++++-------- .../clientmiddleware/metrics_middleware.go | 9 ++---- .../metrics_middleware_test.go | 5 ++-- .../pluginsintegration/pluginsintegration.go | 12 +++++--- 7 files changed, 54 insertions(+), 34 deletions(-) diff --git a/pkg/cmd/grafana/apiserver/cmd.go b/pkg/cmd/grafana/apiserver/cmd.go index d029f65671a..cb3530b7c2c 100644 --- a/pkg/cmd/grafana/apiserver/cmd.go +++ b/pkg/cmd/grafana/apiserver/cmd.go @@ -9,6 +9,7 @@ import ( "github.com/grafana/grafana/pkg/cmd/grafana-server/commands" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/server" "github.com/grafana/grafana/pkg/services/apiserver/standalone" ) @@ -47,8 +48,14 @@ func newCommandStartExampleAPIServer(o *APIServerOptions, stopCh <-chan struct{} return err } + // Currently TracingOptions.ApplyTo, which will configure/initialize tracing, + // happens after loadAPIGroupBuilders. Hack to workaround this for now to allow + // the tracer to be initialized at a later stage, when tracer is available. + // TODO: Fix so that TracingOptions.ApplyTo happens before or during loadAPIGroupBuilders. + tracer := newLateInitializedTracingService() + // Load each group from the args - if err := o.loadAPIGroupBuilders(apis); err != nil { + if err := o.loadAPIGroupBuilders(tracer, apis); err != nil { return err } @@ -62,6 +69,10 @@ func newCommandStartExampleAPIServer(o *APIServerOptions, stopCh <-chan struct{} return err } + if o.Options.TracingOptions.TracingService != nil { + tracer.InitTracer(o.Options.TracingOptions.TracingService) + } + if err := o.RunAPIServer(config, stopCh); err != nil { return err } @@ -87,3 +98,19 @@ func RunCLI(opts commands.ServerOptions) int { return cli.Run(cmd) } + +type lateInitializedTracingService struct { + tracing.Tracer +} + +func newLateInitializedTracingService() *lateInitializedTracingService { + return &lateInitializedTracingService{ + Tracer: tracing.InitializeTracerForTest(), + } +} + +func (s *lateInitializedTracingService) InitTracer(tracer tracing.Tracer) { + s.Tracer = tracer +} + +var _ tracing.Tracer = &lateInitializedTracingService{} diff --git a/pkg/cmd/grafana/apiserver/server.go b/pkg/cmd/grafana/apiserver/server.go index d91cf77ac22..18407831d7a 100644 --- a/pkg/cmd/grafana/apiserver/server.go +++ b/pkg/cmd/grafana/apiserver/server.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/apiserver/builder" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/infra/tracing" grafanaAPIServer "github.com/grafana/grafana/pkg/services/apiserver" "github.com/grafana/grafana/pkg/services/apiserver/standalone" standaloneoptions "github.com/grafana/grafana/pkg/services/apiserver/standalone/options" @@ -50,10 +51,10 @@ func newAPIServerOptions(out, errOut io.Writer) *APIServerOptions { } } -func (o *APIServerOptions) loadAPIGroupBuilders(apis []schema.GroupVersion) error { +func (o *APIServerOptions) loadAPIGroupBuilders(tracer tracing.Tracer, apis []schema.GroupVersion) error { o.builders = []builder.APIGroupBuilder{} for _, gv := range apis { - api, err := o.factory.MakeAPIServer(gv) + api, err := o.factory.MakeAPIServer(tracer, gv) if err != nil { return err } diff --git a/pkg/services/apiserver/standalone/factory.go b/pkg/services/apiserver/standalone/factory.go index 9979c5d39db..19c18cab5e0 100644 --- a/pkg/services/apiserver/standalone/factory.go +++ b/pkg/services/apiserver/standalone/factory.go @@ -34,7 +34,7 @@ type APIServerFactory interface { GetEnabled(runtime []RuntimeConfig) ([]schema.GroupVersion, error) // Make an API server for a given group+version - MakeAPIServer(gv schema.GroupVersion) (builder.APIGroupBuilder, error) + MakeAPIServer(tracer tracing.Tracer, gv schema.GroupVersion) (builder.APIGroupBuilder, error) } // Zero dependency provider for testing @@ -62,7 +62,7 @@ func (p *DummyAPIFactory) GetEnabled(runtime []RuntimeConfig) ([]schema.GroupVer return gv, nil } -func (p *DummyAPIFactory) MakeAPIServer(gv schema.GroupVersion) (builder.APIGroupBuilder, error) { +func (p *DummyAPIFactory) MakeAPIServer(tracer tracing.Tracer, gv schema.GroupVersion) (builder.APIGroupBuilder, error) { if gv.Version != "v0alpha1" { return nil, fmt.Errorf("only alpha supported now") } @@ -79,9 +79,9 @@ func (p *DummyAPIFactory) MakeAPIServer(gv schema.GroupVersion) (builder.APIGrou Client: client.NewTestDataClient(), }, client.NewTestDataRegistry(), - nil, // legacy lookup - prometheus.NewRegistry(), // ??? - tracing.InitializeTracerForTest(), // ??? + nil, // legacy lookup + prometheus.NewRegistry(), // ??? + tracer, ) case "featuretoggle.grafana.app": diff --git a/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go b/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go index 9982aefcb66..2a8c6e65388 100644 --- a/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go +++ b/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go @@ -10,30 +10,22 @@ import ( "github.com/grafana/grafana/pkg/plugins" plog "github.com/grafana/grafana/pkg/plugins/log" "github.com/grafana/grafana/pkg/plugins/pluginrequestmeta" - "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/setting" ) // NewLoggerMiddleware creates a new plugins.ClientMiddleware that will // log requests. -func NewLoggerMiddleware(cfg *setting.Cfg, logger plog.Logger, features featuremgmt.FeatureToggles) plugins.ClientMiddleware { +func NewLoggerMiddleware(logger plog.Logger) plugins.ClientMiddleware { return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { - if !cfg.PluginLogBackendRequests { - return next - } - return &LoggerMiddleware{ - next: next, - logger: logger, - features: features, + next: next, + logger: logger, } }) } type LoggerMiddleware struct { - next plugins.Client - logger plog.Logger - features featuremgmt.FeatureToggles + next plugins.Client + logger plog.Logger } func (m *LoggerMiddleware) logRequest(ctx context.Context, fn func(ctx context.Context) (requestStatus, error)) error { diff --git a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go index 8b7cd2dc02d..08274d8248b 100644 --- a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go +++ b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go @@ -11,7 +11,6 @@ import ( "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/plugins/manager/registry" "github.com/grafana/grafana/pkg/plugins/pluginrequestmeta" - "github.com/grafana/grafana/pkg/services/featuremgmt" ) // pluginMetrics contains the prometheus metrics used by the MetricsMiddleware. @@ -27,11 +26,10 @@ type pluginMetrics struct { type MetricsMiddleware struct { pluginMetrics pluginRegistry registry.Service - features featuremgmt.FeatureToggles next plugins.Client } -func newMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service, features featuremgmt.FeatureToggles) *MetricsMiddleware { +func newMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service) *MetricsMiddleware { additionalLabels := []string{"status_source"} pluginRequestCounter := prometheus.NewCounterVec(prometheus.CounterOpts{ Namespace: "grafana", @@ -72,13 +70,12 @@ func newMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry r pluginRequestDurationSeconds: pluginRequestDurationSeconds, }, pluginRegistry: pluginRegistry, - features: features, } } // NewMetricsMiddleware returns a new MetricsMiddleware. -func NewMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service, features featuremgmt.FeatureToggles) plugins.ClientMiddleware { - imw := newMetricsMiddleware(promRegisterer, pluginRegistry, features) +func NewMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service) plugins.ClientMiddleware { + imw := newMetricsMiddleware(promRegisterer, pluginRegistry) return plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { imw.next = next return imw diff --git a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go index 478a65469d7..6fe03743b12 100644 --- a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go +++ b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go @@ -17,7 +17,6 @@ import ( "github.com/grafana/grafana/pkg/plugins/manager/client/clienttest" "github.com/grafana/grafana/pkg/plugins/manager/fakes" "github.com/grafana/grafana/pkg/plugins/pluginrequestmeta" - "github.com/grafana/grafana/pkg/services/featuremgmt" ) const ( @@ -76,7 +75,7 @@ func TestInstrumentationMiddleware(t *testing.T) { JSONData: plugins.JSONData{ID: pluginID, Backend: true}, })) - mw := newMetricsMiddleware(promRegistry, pluginsRegistry, featuremgmt.WithFeatures()) + mw := newMetricsMiddleware(promRegistry, pluginsRegistry) cdt := clienttest.NewClientDecoratorTest(t, clienttest.WithMiddlewares( plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { mw.next = next @@ -153,7 +152,7 @@ func TestInstrumentationMiddlewareStatusSource(t *testing.T) { require.NoError(t, pluginsRegistry.Add(context.Background(), &plugins.Plugin{ JSONData: plugins.JSONData{ID: pluginID, Backend: true}, })) - metricsMw := newMetricsMiddleware(promRegistry, pluginsRegistry, featuremgmt.WithFeatures()) + metricsMw := newMetricsMiddleware(promRegistry, pluginsRegistry) cdt := clienttest.NewClientDecoratorTest(t, clienttest.WithMiddlewares( NewPluginRequestMetaMiddleware(), plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { diff --git a/pkg/services/pluginsintegration/pluginsintegration.go b/pkg/services/pluginsintegration/pluginsintegration.go index 43d5ec020ac..d00fdaaa1bd 100644 --- a/pkg/services/pluginsintegration/pluginsintegration.go +++ b/pkg/services/pluginsintegration/pluginsintegration.go @@ -160,14 +160,18 @@ func NewClientDecorator( func CreateMiddlewares(cfg *setting.Cfg, oAuthTokenService oauthtoken.OAuthTokenService, tracer tracing.Tracer, cachingService caching.CachingService, features *featuremgmt.FeatureManager, promRegisterer prometheus.Registerer, registry registry.Service) []plugins.ClientMiddleware { middlewares := []plugins.ClientMiddleware{ clientmiddleware.NewPluginRequestMetaMiddleware(), + clientmiddleware.NewTracingMiddleware(tracer), + clientmiddleware.NewMetricsMiddleware(promRegisterer, registry), + clientmiddleware.NewContextualLoggerMiddleware(), + } + + if cfg.PluginLogBackendRequests { + middlewares = append(middlewares, clientmiddleware.NewLoggerMiddleware(log.New("plugin.instrumentation"))) } skipCookiesNames := []string{cfg.LoginCookieName} + middlewares = append(middlewares, - clientmiddleware.NewTracingMiddleware(tracer), - clientmiddleware.NewMetricsMiddleware(promRegisterer, registry, features), - clientmiddleware.NewContextualLoggerMiddleware(), - clientmiddleware.NewLoggerMiddleware(cfg, log.New("plugin.instrumentation"), features), clientmiddleware.NewTracingHeaderMiddleware(), clientmiddleware.NewClearAuthHeadersMiddleware(), clientmiddleware.NewOAuthTokenMiddleware(oAuthTokenService),