From 68fe045ec7433feee025f3e7c3f26c5f78cbb7d6 Mon Sep 17 00:00:00 2001 From: Giuseppe Guerra Date: Wed, 21 Feb 2024 12:57:40 +0100 Subject: [PATCH] Plugins: Remove pluginsInstrumentationStatusSource feature toggle (#83067) * Plugins: Remove pluginsInstrumentationStatusSource feature toggle * update tests * Inline pluginRequestDurationWithLabels, pluginRequestCounterWithLabels, pluginRequestDurationSecondsWithLabels --- .../feature-toggles/index.md | 1 - .../src/types/featureToggles.gen.ts | 1 - pkg/services/featuremgmt/registry.go | 7 -- pkg/services/featuremgmt/toggles_gen.csv | 1 - pkg/services/featuremgmt/toggles_gen.go | 4 -- pkg/services/featuremgmt/toggles_gen.json | 3 +- .../clientmiddleware/logger_middleware.go | 12 ++-- .../clientmiddleware/metrics_middleware.go | 21 ++---- .../metrics_middleware_test.go | 71 +++++-------------- .../pluginsintegration/pluginsintegration.go | 16 ++--- 10 files changed, 34 insertions(+), 103 deletions(-) diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index 10380c910ea..6231b09a317 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -157,7 +157,6 @@ Experimental features might be changed or removed without prior notice. | `kubernetesSnapshots` | Routes snapshot requests from /api to the /apis endpoint | | `teamHttpHeaders` | Enables datasources to apply team headers to the client requests | | `cachingOptimizeSerializationMemoryUsage` | If enabled, the caching backend gradually serializes query responses for the cache, comparing against the configured `[caching]max_value_mb` value as it goes. This can can help prevent Grafana from running out of memory while attempting to cache very large query responses. | -| `pluginsInstrumentationStatusSource` | Include a status source label for plugin request metrics and logs | | `prometheusPromQAIL` | Prometheus and AI/ML to assist users in creating a query | | `alertmanagerRemoteSecondary` | Enable Grafana to sync configuration and state with a remote Alertmanager. | | `alertmanagerRemotePrimary` | Enable Grafana to have a remote Alertmanager instance as the primary Alertmanager. | diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index f5a2e721c16..74d38c26b52 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -139,7 +139,6 @@ export interface FeatureToggles { awsDatasourcesNewFormStyling?: boolean; cachingOptimizeSerializationMemoryUsage?: boolean; panelTitleSearchInV1?: boolean; - pluginsInstrumentationStatusSource?: boolean; managedPluginsInstall?: boolean; prometheusPromQAIL?: boolean; addFieldFromCalculationStatFunctions?: boolean; diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 38e6caeaec5..cf57c68952a 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -905,13 +905,6 @@ var ( Stage: FeatureStageExperimental, Owner: grafanaBackendPlatformSquad, }, - { - Name: "pluginsInstrumentationStatusSource", - Description: "Include a status source label for plugin request metrics and logs", - FrontendOnly: false, - Stage: FeatureStageExperimental, - Owner: grafanaPluginsPlatformSquad, - }, { Name: "managedPluginsInstall", Description: "Install managed plugins directly from plugins catalog", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index ab21b586a43..3098345d97f 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -120,7 +120,6 @@ teamHttpHeaders,experimental,@grafana/identity-access-team,false,false,false awsDatasourcesNewFormStyling,preview,@grafana/aws-datasources,false,false,true cachingOptimizeSerializationMemoryUsage,experimental,@grafana/grafana-operator-experience-squad,false,false,false panelTitleSearchInV1,experimental,@grafana/backend-platform,true,false,false -pluginsInstrumentationStatusSource,experimental,@grafana/plugins-platform-backend,false,false,false managedPluginsInstall,preview,@grafana/plugins-platform-backend,false,false,false prometheusPromQAIL,experimental,@grafana/observability-metrics,false,false,true addFieldFromCalculationStatFunctions,preview,@grafana/dataviz-squad,false,false,true diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index e5ec782a983..10a8223f5c5 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -491,10 +491,6 @@ const ( // Enable searching for dashboards using panel title in search v1 FlagPanelTitleSearchInV1 = "panelTitleSearchInV1" - // FlagPluginsInstrumentationStatusSource - // Include a status source label for plugin request metrics and logs - FlagPluginsInstrumentationStatusSource = "pluginsInstrumentationStatusSource" - // FlagManagedPluginsInstall // Install managed plugins directly from plugins catalog FlagManagedPluginsInstall = "managedPluginsInstall" diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index 2e32e1e9545..a04e47e1fe4 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -82,7 +82,8 @@ "metadata": { "name": "pluginsInstrumentationStatusSource", "resourceVersion": "1708108588074", - "creationTimestamp": "2024-02-16T18:36:28Z" + "creationTimestamp": "2024-02-16T18:36:28Z", + "deletionTimestamp": "2024-02-19T14:18:02Z" }, "spec": { "description": "Include a status source label for plugin request metrics and logs", diff --git a/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go b/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go index 5dc5eea6094..9982aefcb66 100644 --- a/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go +++ b/pkg/services/pluginsintegration/clientmiddleware/logger_middleware.go @@ -50,9 +50,7 @@ func (m *LoggerMiddleware) logRequest(ctx context.Context, fn func(ctx context.C if err != nil { logParams = append(logParams, "error", err) } - if m.features.IsEnabled(ctx, featuremgmt.FlagPluginsInstrumentationStatusSource) { - logParams = append(logParams, "statusSource", pluginrequestmeta.StatusSourceFromContext(ctx)) - } + logParams = append(logParams, "statusSource", pluginrequestmeta.StatusSourceFromContext(ctx)) ctxLogger := m.logger.FromContext(ctx) logFunc := ctxLogger.Info @@ -81,9 +79,11 @@ func (m *LoggerMiddleware) QueryData(ctx context.Context, req *backend.QueryData ctxLogger := m.logger.FromContext(ctx) for refID, dr := range resp.Responses { if dr.Error != nil { - logParams := []any{"refID", refID, "status", int(dr.Status), "error", dr.Error} - if m.features.IsEnabled(ctx, featuremgmt.FlagPluginsInstrumentationStatusSource) { - logParams = append(logParams, "statusSource", pluginrequestmeta.StatusSourceFromPluginErrorSource(dr.ErrorSource)) + logParams := []any{ + "refID", refID, + "status", int(dr.Status), + "error", dr.Error, + "statusSource", pluginrequestmeta.StatusSourceFromPluginErrorSource(dr.ErrorSource), } ctxLogger.Error("Partial data response error", logParams...) } diff --git a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go index 3ea7f566118..8b7cd2dc02d 100644 --- a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go +++ b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware.go @@ -32,10 +32,7 @@ type MetricsMiddleware struct { } func newMetricsMiddleware(promRegisterer prometheus.Registerer, pluginRegistry registry.Service, features featuremgmt.FeatureToggles) *MetricsMiddleware { - var additionalLabels []string - if features.IsEnabledGlobally(featuremgmt.FlagPluginsInstrumentationStatusSource) { - additionalLabels = []string{"status_source"} - } + additionalLabels := []string{"status_source"} pluginRequestCounter := prometheus.NewCounterVec(prometheus.CounterOpts{ Namespace: "grafana", Name: "plugin_request_total", @@ -119,19 +116,11 @@ func (m *MetricsMiddleware) instrumentPluginRequest(ctx context.Context, pluginC status, err := fn(ctx) elapsed := time.Since(start) - pluginRequestDurationLabels := []string{pluginCtx.PluginID, endpoint, target} - pluginRequestCounterLabels := []string{pluginCtx.PluginID, endpoint, status.String(), target} - pluginRequestDurationSecondsLabels := []string{"grafana-backend", pluginCtx.PluginID, endpoint, status.String(), target} - if m.features.IsEnabled(ctx, featuremgmt.FlagPluginsInstrumentationStatusSource) { - statusSource := pluginrequestmeta.StatusSourceFromContext(ctx) - pluginRequestDurationLabels = append(pluginRequestDurationLabels, string(statusSource)) - pluginRequestCounterLabels = append(pluginRequestCounterLabels, string(statusSource)) - pluginRequestDurationSecondsLabels = append(pluginRequestDurationSecondsLabels, string(statusSource)) - } + statusSource := pluginrequestmeta.StatusSourceFromContext(ctx) - pluginRequestDurationWithLabels := m.pluginRequestDuration.WithLabelValues(pluginRequestDurationLabels...) - pluginRequestCounterWithLabels := m.pluginRequestCounter.WithLabelValues(pluginRequestCounterLabels...) - pluginRequestDurationSecondsWithLabels := m.pluginRequestDurationSeconds.WithLabelValues(pluginRequestDurationSecondsLabels...) + pluginRequestDurationWithLabels := m.pluginRequestDuration.WithLabelValues(pluginCtx.PluginID, endpoint, target, string(statusSource)) + pluginRequestCounterWithLabels := m.pluginRequestCounter.WithLabelValues(pluginCtx.PluginID, endpoint, status.String(), target, string(statusSource)) + pluginRequestDurationSecondsWithLabels := m.pluginRequestDurationSeconds.WithLabelValues("grafana-backend", pluginCtx.PluginID, endpoint, status.String(), target, string(statusSource)) if traceID := tracing.TraceIDFromContext(ctx, true); traceID != "" { pluginRequestDurationWithLabels.(prometheus.ExemplarObserver).ObserveWithExemplar( diff --git a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go index 66a1bc813d8..478a65469d7 100644 --- a/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go +++ b/pkg/services/pluginsintegration/clientmiddleware/metrics_middleware_test.go @@ -90,7 +90,7 @@ func TestInstrumentationMiddleware(t *testing.T) { require.Equal(t, 1, testutil.CollectAndCount(promRegistry, metricRequestDurationMs)) require.Equal(t, 1, testutil.CollectAndCount(promRegistry, metricRequestDurationS)) - counter := mw.pluginMetrics.pluginRequestCounter.WithLabelValues(pluginID, tc.expEndpoint, requestStatusOK.String(), string(backendplugin.TargetUnknown)) + counter := mw.pluginMetrics.pluginRequestCounter.WithLabelValues(pluginID, tc.expEndpoint, requestStatusOK.String(), string(backendplugin.TargetUnknown), string(pluginrequestmeta.DefaultStatusSource)) require.Equal(t, 1.0, testutil.ToFloat64(counter)) for _, m := range []string{metricRequestDurationMs, metricRequestDurationS} { require.NoError(t, checkHistogram(promRegistry, m, map[string]string{ @@ -115,12 +115,6 @@ func TestInstrumentationMiddleware(t *testing.T) { func TestInstrumentationMiddlewareStatusSource(t *testing.T) { const labelStatusSource = "status_source" - queryDataOKCounterLabels := prometheus.Labels{ - "plugin_id": pluginID, - "endpoint": endpointQueryData, - "status": requestStatusOK.String(), - "target": string(backendplugin.TargetUnknown), - } queryDataErrorCounterLabels := prometheus.Labels{ "plugin_id": pluginID, "endpoint": endpointQueryData, @@ -159,8 +153,7 @@ func TestInstrumentationMiddlewareStatusSource(t *testing.T) { require.NoError(t, pluginsRegistry.Add(context.Background(), &plugins.Plugin{ JSONData: plugins.JSONData{ID: pluginID, Backend: true}, })) - features := featuremgmt.WithFeatures(featuremgmt.FlagPluginsInstrumentationStatusSource) - metricsMw := newMetricsMiddleware(promRegistry, pluginsRegistry, features) + metricsMw := newMetricsMiddleware(promRegistry, pluginsRegistry, featuremgmt.WithFeatures()) cdt := clienttest.NewClientDecoratorTest(t, clienttest.WithMiddlewares( NewPluginRequestMetaMiddleware(), plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { @@ -171,53 +164,21 @@ func TestInstrumentationMiddlewareStatusSource(t *testing.T) { )) t.Run("Metrics", func(t *testing.T) { - t.Run("Should ignore ErrorSource if feature flag is disabled", func(t *testing.T) { - // Use different middleware without feature flag - metricsMw := newMetricsMiddleware(prometheus.NewRegistry(), pluginsRegistry, featuremgmt.WithFeatures()) - cdt := clienttest.NewClientDecoratorTest(t, clienttest.WithMiddlewares( - plugins.ClientMiddlewareFunc(func(next plugins.Client) plugins.Client { - metricsMw.next = next - return metricsMw - }), - )) + metricsMw.pluginMetrics.pluginRequestCounter.Reset() - cdt.TestClient.QueryDataFunc = func(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { - return &backend.QueryDataResponse{Responses: map[string]backend.DataResponse{"A": downstreamErrorResponse}}, nil - } - _, err := cdt.Decorator.QueryData(context.Background(), &backend.QueryDataRequest{PluginContext: pCtx}) - require.NoError(t, err) - counter, err := metricsMw.pluginMetrics.pluginRequestCounter.GetMetricWith(newLabels(queryDataErrorCounterLabels, nil)) - require.NoError(t, err) - require.Equal(t, 1.0, testutil.ToFloat64(counter)) - - // error_source should not be defined at all - _, err = metricsMw.pluginMetrics.pluginRequestCounter.GetMetricWith(newLabels( - queryDataOKCounterLabels, - prometheus.Labels{ - labelStatusSource: string(backend.ErrorSourceDownstream), - }), - ) - require.Error(t, err) - require.ErrorContains(t, err, "inconsistent label cardinality") - }) - - t.Run("Should add error_source label if feature flag is enabled", func(t *testing.T) { - metricsMw.pluginMetrics.pluginRequestCounter.Reset() - - cdt.TestClient.QueryDataFunc = func(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { - return &backend.QueryDataResponse{Responses: map[string]backend.DataResponse{"A": downstreamErrorResponse}}, nil - } - _, err := cdt.Decorator.QueryData(context.Background(), &backend.QueryDataRequest{PluginContext: pCtx}) - require.NoError(t, err) - counter, err := metricsMw.pluginMetrics.pluginRequestCounter.GetMetricWith(newLabels( - queryDataErrorCounterLabels, - prometheus.Labels{ - labelStatusSource: string(backend.ErrorSourceDownstream), - }), - ) - require.NoError(t, err) - require.Equal(t, 1.0, testutil.ToFloat64(counter)) - }) + cdt.TestClient.QueryDataFunc = func(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { + return &backend.QueryDataResponse{Responses: map[string]backend.DataResponse{"A": downstreamErrorResponse}}, nil + } + _, err := cdt.Decorator.QueryData(context.Background(), &backend.QueryDataRequest{PluginContext: pCtx}) + require.NoError(t, err) + counter, err := metricsMw.pluginMetrics.pluginRequestCounter.GetMetricWith(newLabels( + queryDataErrorCounterLabels, + prometheus.Labels{ + labelStatusSource: string(backend.ErrorSourceDownstream), + }), + ) + require.NoError(t, err) + require.Equal(t, 1.0, testutil.ToFloat64(counter)) }) t.Run("Priority", func(t *testing.T) { diff --git a/pkg/services/pluginsintegration/pluginsintegration.go b/pkg/services/pluginsintegration/pluginsintegration.go index 32f74d7d40a..0a10913d60e 100644 --- a/pkg/services/pluginsintegration/pluginsintegration.go +++ b/pkg/services/pluginsintegration/pluginsintegration.go @@ -153,12 +153,8 @@ 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 { - var middlewares []plugins.ClientMiddleware - - if features.IsEnabledGlobally(featuremgmt.FlagPluginsInstrumentationStatusSource) { - middlewares = []plugins.ClientMiddleware{ - clientmiddleware.NewPluginRequestMetaMiddleware(), - } + middlewares := []plugins.ClientMiddleware{ + clientmiddleware.NewPluginRequestMetaMiddleware(), } skipCookiesNames := []string{cfg.LoginCookieName} @@ -189,11 +185,9 @@ func CreateMiddlewares(cfg *setting.Cfg, oAuthTokenService oauthtoken.OAuthToken middlewares = append(middlewares, clientmiddleware.NewHTTPClientMiddleware()) - if features.IsEnabledGlobally(featuremgmt.FlagPluginsInstrumentationStatusSource) { - // StatusSourceMiddleware should be at the very bottom, or any middlewares below it won't see the - // correct status source in their context.Context - middlewares = append(middlewares, clientmiddleware.NewStatusSourceMiddleware()) - } + // StatusSourceMiddleware should be at the very bottom, or any middlewares below it won't see the + // correct status source in their context.Context + middlewares = append(middlewares, clientmiddleware.NewStatusSourceMiddleware()) return middlewares }