From 177c16f32dabdc31e4e71c60fb1a091d1efb0f4e Mon Sep 17 00:00:00 2001 From: Will Browne Date: Tue, 10 Feb 2026 10:38:31 +0000 Subject: [PATCH] Plugins: Improve instrumentation for plugins app (#117587) add metrics for plugins app --- apps/plugins/go.mod | 2 +- apps/plugins/pkg/app/app.go | 13 ++- .../pkg/app/install/child_reconcile.go | 44 ++++++--- .../pkg/app/install/child_reconcile_test.go | 11 ++- apps/plugins/pkg/app/install/registrar.go | 57 +++++++++++- .../plugins/pkg/app/install/registrar_test.go | 17 ++-- apps/plugins/pkg/app/meta/catalog.go | 35 +++++-- apps/plugins/pkg/app/meta/catalog_test.go | 93 +++++++++---------- apps/plugins/pkg/app/meta/converter.go | 29 +++--- apps/plugins/pkg/app/meta/converter_test.go | 23 ++--- apps/plugins/pkg/app/meta/core.go | 14 +-- apps/plugins/pkg/app/meta/core_test.go | 27 +++--- apps/plugins/pkg/app/metrics/metrics.go | 82 ++++++++++++++++ apps/plugins/pkg/app/storage.go | 22 ++++- pkg/registry/apps/plugins/register.go | 13 ++- pkg/server/wire_gen.go | 4 +- .../pluginsintegration/installsync/syncer.go | 2 +- .../installsync/syncer_test.go | 7 +- 18 files changed, 347 insertions(+), 148 deletions(-) create mode 100644 apps/plugins/pkg/app/metrics/metrics.go diff --git a/apps/plugins/go.mod b/apps/plugins/go.mod index 4cc73a67656..1ac9269ec7b 100644 --- a/apps/plugins/go.mod +++ b/apps/plugins/go.mod @@ -19,6 +19,7 @@ require ( github.com/grafana/grafana-app-sdk/logging v0.49.0 github.com/grafana/grafana/pkg/apimachinery v0.0.0 github.com/grafana/grafana/pkg/plugins v0.0.0 + github.com/prometheus/client_golang v1.23.2 github.com/stretchr/testify v1.11.1 k8s.io/apimachinery v0.34.3 k8s.io/apiserver v0.34.3 @@ -171,7 +172,6 @@ require ( github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/alertmanager v0.28.2 // indirect - github.com/prometheus/client_golang v1.23.2 // indirect github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.67.5 // indirect github.com/prometheus/exporter-toolkit v0.15.1 // indirect diff --git a/apps/plugins/pkg/app/app.go b/apps/plugins/pkg/app/app.go index d9f1e0632f7..190b7ca5f67 100644 --- a/apps/plugins/pkg/app/app.go +++ b/apps/plugins/pkg/app/app.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana-app-sdk/app" "github.com/grafana/grafana-app-sdk/k8s" appsdkapiserver "github.com/grafana/grafana-app-sdk/k8s/apiserver" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana-app-sdk/operator" "github.com/grafana/grafana-app-sdk/simple" "k8s.io/apimachinery/pkg/runtime/schema" @@ -37,9 +38,10 @@ func New(cfg app.Config) (app.App, error) { } if specificConfig.EnableChildReconciler { + logger := logging.DefaultLogger.With("app", "plugins.app") clientGenerator := k8s.NewClientRegistry(cfg.KubeConfig, k8s.DefaultClientConfig()) - registrar := install.NewInstallRegistrar(clientGenerator) - pluginKind.Reconciler = install.NewChildPluginReconciler(specificConfig.MetaProviderManager, registrar) + registrar := install.NewInstallRegistrar(logger, clientGenerator) + pluginKind.Reconciler = install.NewChildPluginReconciler(logger, specificConfig.MetaProviderManager, registrar) } simpleConfig := simple.AppConfig{ @@ -76,7 +78,8 @@ type PluginAppConfig struct { EnableChildReconciler bool } -func ProvideAppInstaller( +func NewPluginsAppInstaller( + logger logging.Logger, authorizer authorizer.Authorizer, metaProviderManager *meta.ProviderManager, enableChildReconciler bool, @@ -100,6 +103,7 @@ func ProvideAppInstaller( AppInstaller: defaultInstaller, authorizer: authorizer, metaManager: metaProviderManager, + logger: logger, ready: make(chan struct{}), } return appInstaller, nil @@ -109,6 +113,7 @@ type PluginAppInstaller struct { appsdkapiserver.AppInstaller metaManager *meta.ProviderManager authorizer authorizer.Authorizer + logger logging.Logger // restConfig is set during InitializeApp and used by the client factory restConfig *restclient.Config @@ -149,7 +154,7 @@ func (p *PluginAppInstaller) InstallAPIs( pluginMetaGVR := pluginsv0alpha1.MetaKind().GroupVersionResource() replacedStorage := map[schema.GroupVersionResource]rest.Storage{ - pluginMetaGVR: NewMetaStorage(p.metaManager, clientFactory), + pluginMetaGVR: NewMetaStorage(p.logger, p.metaManager, clientFactory), } wrappedServer := &customStorageWrapper{ wrapped: server, diff --git a/apps/plugins/pkg/app/install/child_reconcile.go b/apps/plugins/pkg/app/install/child_reconcile.go index d7a93704c8b..01a4ea69712 100644 --- a/apps/plugins/pkg/app/install/child_reconcile.go +++ b/apps/plugins/pkg/app/install/child_reconcile.go @@ -11,6 +11,7 @@ import ( pluginsv0alpha1 "github.com/grafana/grafana/apps/plugins/pkg/apis/plugins/v0alpha1" "github.com/grafana/grafana/apps/plugins/pkg/app/meta" + "github.com/grafana/grafana/apps/plugins/pkg/app/metrics" ) var ( @@ -22,14 +23,16 @@ type ChildPluginReconciler struct { operator.TypedReconciler[*pluginsv0alpha1.Plugin] metaManager *meta.ProviderManager registrar Registrar + logger logging.Logger } // NewChildPluginReconciler creates a new ChildPluginReconciler instance. -func NewChildPluginReconciler(metaManager *meta.ProviderManager, registrar Registrar) *ChildPluginReconciler { +func NewChildPluginReconciler(logger logging.Logger, metaManager *meta.ProviderManager, registrar Registrar) *ChildPluginReconciler { reconciler := &ChildPluginReconciler{ TypedReconciler: operator.TypedReconciler[*pluginsv0alpha1.Plugin]{}, metaManager: metaManager, registrar: registrar, + logger: logger, } reconciler.ReconcileFunc = reconciler.reconcile return reconciler @@ -37,10 +40,15 @@ func NewChildPluginReconciler(metaManager *meta.ProviderManager, registrar Regis // reconcile is the main reconciliation loop for ChildPlugin resources. func (r *ChildPluginReconciler) reconcile(ctx context.Context, req operator.TypedReconcileRequest[*pluginsv0alpha1.Plugin]) (operator.ReconcileResult, error) { + start := time.Now() + defer func() { + metrics.ChildReconciliationDurationSeconds.Observe(time.Since(start).Seconds()) + }() + plugin := req.Object - logger := logging.FromContext(ctx).With( + logger := r.logger.WithContext(ctx).With( "pluginId", plugin.Spec.Id, - "namespace", plugin.Namespace, + "requestNamespace", plugin.Namespace, "version", plugin.Spec.Version, "action", req.Action, "parentId", plugin.Spec.ParentId, @@ -58,6 +66,7 @@ func (r *ChildPluginReconciler) reconcile(ctx context.Context, req operator.Type }) if err != nil { logger.Error("Failed to get plugin metadata", "error", err) + metrics.ChildReconciliationTotal.WithLabelValues("error").Inc() return operator.ReconcileResult{ RequeueAfter: &requeueAfter, }, nil @@ -65,6 +74,7 @@ func (r *ChildPluginReconciler) reconcile(ctx context.Context, req operator.Type if len(result.Meta.Children) == 0 { logger.Debug("Plugin has no children, skipping child plugin reconciliation") + metrics.ChildReconciliationTotal.WithLabelValues("success").Inc() return operator.ReconcileResult{}, nil } @@ -73,24 +83,36 @@ func (r *ChildPluginReconciler) reconcile(ctx context.Context, req operator.Type "action", req.Action, ) + var reconcileResult operator.ReconcileResult + var reconcileErr error + switch req.Action { case operator.ReconcileActionCreated, operator.ReconcileActionUpdated, operator.ReconcileActionResynced: - return r.registerChildren(ctx, plugin, result.Meta.Children) + reconcileResult, reconcileErr = r.registerChildren(ctx, plugin, result.Meta.Children) case operator.ReconcileActionDeleted: - return r.unregisterChildren(ctx, plugin.Namespace, result.Meta.Children) + reconcileResult, reconcileErr = r.unregisterChildren(ctx, plugin.Namespace, result.Meta.Children) case operator.ReconcileActionUnknown: - break // handled by return statement below + reconcileErr = fmt.Errorf("invalid action: %d", req.Action) + default: + reconcileErr = fmt.Errorf("invalid action: %d", req.Action) } - return operator.ReconcileResult{}, fmt.Errorf("invalid action: %d", req.Action) + + status := "success" + if reconcileErr != nil || reconcileResult.RequeueAfter != nil { + status = "error" + } + metrics.ChildReconciliationTotal.WithLabelValues(status).Inc() + + return reconcileResult, reconcileErr } func (r *ChildPluginReconciler) unregisterChildren(ctx context.Context, namespace string, children []string) (operator.ReconcileResult, error) { - logger := logging.FromContext(ctx) + logger := r.logger.WithContext(ctx).With("requestNamespace", namespace) retry := false for _, childID := range children { err := r.registrar.Unregister(ctx, namespace, childID, SourceChildPluginReconciler) if err != nil && !errorsK8s.IsNotFound(err) { - logger.Error("Failed to unregister child plugin", "error", err, "childId", childID) + logger.Error("Failed to unregister child plugin", "error", err, "pluginId", childID) retry = true } } @@ -102,7 +124,7 @@ func (r *ChildPluginReconciler) unregisterChildren(ctx context.Context, namespac } func (r *ChildPluginReconciler) registerChildren(ctx context.Context, parent *pluginsv0alpha1.Plugin, children []string) (operator.ReconcileResult, error) { - logger := logging.FromContext(ctx) + logger := r.logger.WithContext(ctx).With("requestNamespace", parent.Namespace) retry := false for _, childID := range children { childInstall := &PluginInstall{ @@ -113,7 +135,7 @@ func (r *ChildPluginReconciler) registerChildren(ctx context.Context, parent *pl } err := r.registrar.Register(ctx, parent.Namespace, childInstall) if err != nil { - logger.Error("Failed to register child plugin", "error", err, "childId", childID) + logger.Error("Failed to register child plugin", "error", err, "pluginId", childID) retry = true } } diff --git a/apps/plugins/pkg/app/install/child_reconcile_test.go b/apps/plugins/pkg/app/install/child_reconcile_test.go index c55789e8c30..dadfe8b52a1 100644 --- a/apps/plugins/pkg/app/install/child_reconcile_test.go +++ b/apps/plugins/pkg/app/install/child_reconcile_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana-app-sdk/operator" "github.com/stretchr/testify/require" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -80,7 +81,7 @@ func TestChildPluginReconciler_ReconcileWithChildren(t *testing.T) { metaManager := meta.NewProviderManager(mockProv) mockReg := newMockPluginRegistrar() - reconciler := NewChildPluginReconciler(metaManager, mockReg) + reconciler := NewChildPluginReconciler(&logging.NoOpLogger{}, metaManager, mockReg) plugin := &pluginsv0alpha1.Plugin{ ObjectMeta: metav1.ObjectMeta{ @@ -182,7 +183,7 @@ func TestChildPluginReconciler_ReconcileSpecialCases(t *testing.T) { metaManager := meta.NewProviderManager(mockProv) mockReg := newMockPluginRegistrar() - reconciler := NewChildPluginReconciler(metaManager, mockReg) + reconciler := NewChildPluginReconciler(&logging.NoOpLogger{}, metaManager, mockReg) plugin := &pluginsv0alpha1.Plugin{ ObjectMeta: metav1.ObjectMeta{ @@ -242,7 +243,7 @@ func TestChildPluginReconciler_ReconcileMetaErrors(t *testing.T) { metaManager := meta.NewProviderManager(mockProv) mockReg := newMockPluginRegistrar() - reconciler := NewChildPluginReconciler(metaManager, mockReg) + reconciler := NewChildPluginReconciler(&logging.NoOpLogger{}, metaManager, mockReg) plugin := &pluginsv0alpha1.Plugin{ ObjectMeta: metav1.ObjectMeta{ @@ -354,7 +355,7 @@ func TestChildPluginReconciler_PartialFailures(t *testing.T) { } } - reconciler := NewChildPluginReconciler(metaManager, mockReg) + reconciler := NewChildPluginReconciler(&logging.NoOpLogger{}, metaManager, mockReg) plugin := &pluginsv0alpha1.Plugin{ ObjectMeta: metav1.ObjectMeta{ @@ -421,7 +422,7 @@ func TestChildPluginReconciler_ReconcileInvalidAction(t *testing.T) { metaManager := meta.NewProviderManager(mockProv) mockReg := newMockPluginRegistrar() - reconciler := NewChildPluginReconciler(metaManager, mockReg) + reconciler := NewChildPluginReconciler(&logging.NoOpLogger{}, metaManager, mockReg) plugin := &pluginsv0alpha1.Plugin{ ObjectMeta: metav1.ObjectMeta{ diff --git a/apps/plugins/pkg/app/install/registrar.go b/apps/plugins/pkg/app/install/registrar.go index ca74e22957d..3f358b4d31b 100644 --- a/apps/plugins/pkg/app/install/registrar.go +++ b/apps/plugins/pkg/app/install/registrar.go @@ -3,12 +3,15 @@ package install import ( "context" "sync" + "time" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana-app-sdk/resource" errorsK8s "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" pluginsv0alpha1 "github.com/grafana/grafana/apps/plugins/pkg/apis/plugins/v0alpha1" + "github.com/grafana/grafana/apps/plugins/pkg/app/metrics" ) const ( @@ -95,12 +98,14 @@ type InstallRegistrar struct { client *pluginsv0alpha1.PluginClient clientErr error clientOnce sync.Once + logger logging.Logger } -func NewInstallRegistrar(clientGenerator resource.ClientGenerator) *InstallRegistrar { +func NewInstallRegistrar(logger logging.Logger, clientGenerator resource.ClientGenerator) *InstallRegistrar { return &InstallRegistrar{ clientGenerator: clientGenerator, clientOnce: sync.Once{}, + logger: logger, } } @@ -120,8 +125,17 @@ func (r *InstallRegistrar) GetClient() (*pluginsv0alpha1.PluginClient, error) { // Register creates or updates a plugin install in the registry. func (r *InstallRegistrar) Register(ctx context.Context, namespace string, install *PluginInstall) error { + start := time.Now() + defer func() { + metrics.RegistrationDurationSeconds.WithLabelValues("register").Observe(time.Since(start).Seconds()) + }() + + logger := r.logger.WithContext(ctx).With("requestNamespace", namespace, "pluginId", install.ID, "version", install.Version) + client, err := r.GetClient() if err != nil { + logger.Error("Failed to get plugin client", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("register", "error").Inc() return err } identifier := resource.Identifier{ @@ -131,25 +145,49 @@ func (r *InstallRegistrar) Register(ctx context.Context, namespace string, insta existing, err := client.Get(ctx, identifier) if err != nil && !errorsK8s.IsNotFound(err) { + logger.Error("Failed to get existing plugin", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("register", "error").Inc() return err } if existing != nil { if install.ShouldUpdate(existing) { _, err = client.Update(ctx, install.ToPluginInstallV0Alpha1(namespace), resource.UpdateOptions{ResourceVersion: existing.ResourceVersion}) - return err + if err != nil { + logger.Error("Failed to update plugin", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("register", "error").Inc() + return err + } + metrics.RegistrationOperationsTotal.WithLabelValues("register", "success").Inc() + return nil } + metrics.RegistrationOperationsTotal.WithLabelValues("register", "success").Inc() return nil } _, err = client.Create(ctx, install.ToPluginInstallV0Alpha1(namespace), resource.CreateOptions{}) - return err + if err != nil { + logger.Error("Failed to create plugin", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("register", "error").Inc() + return err + } + metrics.RegistrationOperationsTotal.WithLabelValues("register", "success").Inc() + return nil } // Unregister removes a plugin install from the registry. func (r *InstallRegistrar) Unregister(ctx context.Context, namespace string, name string, source Source) error { + start := time.Now() + defer func() { + metrics.RegistrationDurationSeconds.WithLabelValues("unregister").Observe(time.Since(start).Seconds()) + }() + + logger := r.logger.WithContext(ctx).With("requestNamespace", namespace, "pluginId", name, "source", source) + client, err := r.GetClient() if err != nil { + logger.Error("Failed to get plugin client", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("unregister", "error").Inc() return err } identifier := resource.Identifier{ @@ -158,15 +196,26 @@ func (r *InstallRegistrar) Unregister(ctx context.Context, namespace string, nam } existing, err := client.Get(ctx, identifier) if err != nil && !errorsK8s.IsNotFound(err) { + logger.Error("Failed to get existing plugin", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("unregister", "error").Inc() return err } // if the plugin doesn't exist, nothing to unregister if existing == nil { + metrics.RegistrationOperationsTotal.WithLabelValues("unregister", "success").Inc() return nil } // if the source is different, do not unregister if existingSource, ok := existing.Annotations[PluginInstallSourceAnnotation]; ok && existingSource != source { + metrics.RegistrationOperationsTotal.WithLabelValues("unregister", "success").Inc() return nil } - return client.Delete(ctx, identifier, resource.DeleteOptions{}) + err = client.Delete(ctx, identifier, resource.DeleteOptions{}) + if err != nil { + logger.Error("Failed to delete plugin", "error", err) + metrics.RegistrationOperationsTotal.WithLabelValues("unregister", "error").Inc() + return err + } + metrics.RegistrationOperationsTotal.WithLabelValues("unregister", "success").Inc() + return err } diff --git a/apps/plugins/pkg/app/install/registrar_test.go b/apps/plugins/pkg/app/install/registrar_test.go index 6961acb98f8..c1a8410ecc0 100644 --- a/apps/plugins/pkg/app/install/registrar_test.go +++ b/apps/plugins/pkg/app/install/registrar_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana-app-sdk/resource" "github.com/stretchr/testify/require" errorsK8s "k8s.io/apimachinery/pkg/api/errors" @@ -192,7 +193,7 @@ func TestInstallRegistrar_Register(t *testing.T) { }, } - registrar := NewInstallRegistrar(&fakeClientGenerator{client: fakeClient}) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, &fakeClientGenerator{client: fakeClient}) err := registrar.Register(ctx, "org-1", tt.install) if tt.expectError { @@ -590,7 +591,7 @@ func TestInstallRegistrar_GetClient(t *testing.T) { t.Run("successfully creates client on first call", func(t *testing.T) { fakeClient := &fakePluginInstallClient{} generator := &fakeClientGenerator{client: fakeClient} - registrar := NewInstallRegistrar(generator) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, generator) client, err := registrar.GetClient() require.NoError(t, err) @@ -600,7 +601,7 @@ func TestInstallRegistrar_GetClient(t *testing.T) { t.Run("returns same client on subsequent calls", func(t *testing.T) { fakeClient := &fakePluginInstallClient{} generator := &fakeClientGenerator{client: fakeClient} - registrar := NewInstallRegistrar(generator) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, generator) client1, err1 := registrar.GetClient() require.NoError(t, err1) @@ -613,7 +614,7 @@ func TestInstallRegistrar_GetClient(t *testing.T) { t.Run("returns error when client generation fails", func(t *testing.T) { generator := &fakeClientGenerator{client: nil, shouldError: true} - registrar := NewInstallRegistrar(generator) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, generator) client, err := registrar.GetClient() require.Error(t, err) @@ -683,7 +684,7 @@ func TestInstallRegistrar_Register_ErrorCases(t *testing.T) { fakeClient := &fakePluginInstallClient{} tt.setupClient(fakeClient) - registrar := NewInstallRegistrar(&fakeClientGenerator{client: fakeClient}) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, &fakeClientGenerator{client: fakeClient}) err := registrar.Register(ctx, "org-1", tt.install) if tt.expectError { @@ -813,7 +814,7 @@ func TestInstallRegistrar_Unregister(t *testing.T) { }, } - registrar := NewInstallRegistrar(&fakeClientGenerator{client: fakeClient}) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, &fakeClientGenerator{client: fakeClient}) err := registrar.Unregister(ctx, tt.namespace, tt.pluginName, tt.source) @@ -831,7 +832,7 @@ func TestInstallRegistrar_GetClientError(t *testing.T) { t.Run("Register returns error with nil client", func(t *testing.T) { ctx := context.Background() generator := &fakeClientGenerator{client: nil, shouldError: true} - registrar := NewInstallRegistrar(generator) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, generator) install := &PluginInstall{ ID: "plugin-1", @@ -846,7 +847,7 @@ func TestInstallRegistrar_GetClientError(t *testing.T) { t.Run("Unregister returns error with nil client", func(t *testing.T) { ctx := context.Background() generator := &fakeClientGenerator{client: nil, shouldError: true} - registrar := NewInstallRegistrar(generator) + registrar := NewInstallRegistrar(&logging.NoOpLogger{}, generator) err := registrar.Unregister(ctx, "org-1", "plugin-1", SourcePluginStore) require.Error(t, err) diff --git a/apps/plugins/pkg/app/meta/catalog.go b/apps/plugins/pkg/app/meta/catalog.go index 3b131565e7d..b4ac10282c2 100644 --- a/apps/plugins/pkg/app/meta/catalog.go +++ b/apps/plugins/pkg/app/meta/catalog.go @@ -10,6 +10,8 @@ import ( "time" "github.com/grafana/grafana-app-sdk/logging" + + "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" ) const ( @@ -22,15 +24,16 @@ type CatalogProvider struct { grafanaComAPIURL string grafanaComAPIToken string ttl time.Duration + logger logging.Logger } // NewCatalogProvider creates a new CatalogProvider that fetches metadata from grafana.com. -func NewCatalogProvider(grafanaComAPIURL, grafanaComAPIToken string) *CatalogProvider { - return NewCatalogProviderWithTTL(grafanaComAPIURL, grafanaComAPIToken, defaultCatalogTTL) +func NewCatalogProvider(logger logging.Logger, grafanaComAPIURL, grafanaComAPIToken string) *CatalogProvider { + return NewCatalogProviderWithTTL(logger, grafanaComAPIURL, grafanaComAPIToken, defaultCatalogTTL) } // NewCatalogProviderWithTTL creates a new CatalogProvider with a custom TTL. -func NewCatalogProviderWithTTL(grafanaComAPIURL, grafanaComAPIToken string, ttl time.Duration) *CatalogProvider { +func NewCatalogProviderWithTTL(logger logging.Logger, grafanaComAPIURL, grafanaComAPIToken string, ttl time.Duration) *CatalogProvider { if grafanaComAPIURL == "" { grafanaComAPIURL = "https://grafana.com/api/plugins" } @@ -42,6 +45,7 @@ func NewCatalogProviderWithTTL(grafanaComAPIURL, grafanaComAPIToken string, ttl grafanaComAPIURL: grafanaComAPIURL, grafanaComAPIToken: grafanaComAPIToken, ttl: ttl, + logger: logger, } } @@ -50,6 +54,11 @@ func NewCatalogProviderWithTTL(grafanaComAPIURL, grafanaComAPIToken string, ttl // If ParentID is set in the query, it fetches the parent plugin's version and // filters for the child plugin ID in the children field. func (p *CatalogProvider) GetMeta(ctx context.Context, ref PluginRef) (*Result, error) { + logger := p.logger.WithContext(ctx) + if ns, nsErr := request.NamespaceInfoFrom(ctx, false); nsErr == nil && ns.Value != "" { + logger = logger.With("requestNamespace", ns.Value) + } + u, err := url.Parse(p.grafanaComAPIURL) if err != nil { return nil, fmt.Errorf("invalid grafana.com API URL: %w", err) @@ -77,12 +86,12 @@ func (p *CatalogProvider) GetMeta(ctx context.Context, ref PluginRef) (*Result, } defer func() { if err = resp.Body.Close(); err != nil { - logging.FromContext(ctx).Warn("CatalogProvider: Failed to close response body", "error", err) + logger.Warn("Failed to close response body", "error", err) } }() if resp.StatusCode == http.StatusNotFound { - logging.FromContext(ctx).Warn("CatalogProvider: Plugin metadata not found", "pluginID", lookupID, "version", ref.Version, "url", u.String()) + logger.Debug("Plugin metadata not found", "pluginId", lookupID, "version", ref.Version, "url", u.String()) return nil, ErrMetaNotFound } @@ -97,10 +106,13 @@ func (p *CatalogProvider) GetMeta(ctx context.Context, ref PluginRef) (*Result, // If we're looking up a child plugin, filter for it in the children field if ref.HasParent() { - return p.findChildMeta(ctx, ref.ID, gcomMeta) + return p.findChildMeta(ctx, ref.ID, gcomMeta, logger) } - metaSpec := grafanaComPluginVersionMetaToMetaSpec(ctx, gcomMeta, "") + metaSpec, err := grafanaComPluginVersionMetaToMetaSpec(logger, gcomMeta, "") + if err != nil { + return nil, fmt.Errorf("failed to convert plugin metadata: %w", err) + } return &Result{ Meta: metaSpec, TTL: p.ttl, @@ -108,10 +120,13 @@ func (p *CatalogProvider) GetMeta(ctx context.Context, ref PluginRef) (*Result, } // findChildMeta searches for a child plugin in the parent's children field. -func (p *CatalogProvider) findChildMeta(ctx context.Context, childID string, parentMeta grafanaComPluginVersionMeta) (*Result, error) { +func (p *CatalogProvider) findChildMeta(ctx context.Context, childID string, parentMeta grafanaComPluginVersionMeta, logger logging.Logger) (*Result, error) { for _, child := range parentMeta.Children { if child.JSON.Id == childID { - metaSpec := grafanaComChildPluginVersionToMetaSpec(ctx, child, parentMeta) + metaSpec, err := grafanaComChildPluginVersionToMetaSpec(logger, child, parentMeta) + if err != nil { + return nil, fmt.Errorf("failed to convert child plugin metadata: %w", err) + } return &Result{ Meta: metaSpec, TTL: p.ttl, @@ -119,7 +134,7 @@ func (p *CatalogProvider) findChildMeta(ctx context.Context, childID string, par } } - logging.FromContext(ctx).Debug("CatalogProvider: Child plugin not found in parent's children", + logger.Debug("Child plugin not found in parent's children", "childId", childID, "parentId", parentMeta.PluginSlug, "childrenCount", len(parentMeta.Children), diff --git a/apps/plugins/pkg/app/meta/catalog_test.go b/apps/plugins/pkg/app/meta/catalog_test.go index c52b9814251..9b63d9ccfb8 100644 --- a/apps/plugins/pkg/app/meta/catalog_test.go +++ b/apps/plugins/pkg/app/meta/catalog_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/grafana/grafana-app-sdk/logging" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -66,7 +67,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) require.NotNil(t, result) @@ -118,7 +119,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "nonexistent-plugin", Version: "1.0.0"}) assert.Error(t, err) @@ -132,7 +133,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) assert.Error(t, err) @@ -148,7 +149,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) assert.Error(t, err) @@ -157,7 +158,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { }) t.Run("returns error for invalid API URL", func(t *testing.T) { - provider := NewCatalogProvider("://invalid-url", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, "://invalid-url", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) assert.Error(t, err) @@ -178,6 +179,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { PluginSlug: "test-plugin", Version: "1.0.0", JSON: expectedMeta, + CDNURL: "https://cdn.grafana.com", } w.Header().Set("Content-Type", "application/json") @@ -186,7 +188,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProviderWithTTL(server.URL+"/api/plugins", "", customTTL) + provider := NewCatalogProviderWithTTL(&logging.NoOpLogger{}, server.URL+"/api/plugins", "", customTTL) result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) @@ -204,7 +206,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) assert.Error(t, err) @@ -230,6 +232,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { PluginSlug: "test-plugin", Version: "1.0.0", JSON: expectedMeta, + CDNURL: "https://cdn.grafana.com", } w.Header().Set("Content-Type", "application/json") @@ -238,7 +241,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", expectedToken) + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", expectedToken) result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) @@ -250,11 +253,10 @@ func TestCatalogProvider_GetMeta(t *testing.T) { t.Run("handles missing module hash gracefully", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { response := grafanaComPluginVersionMeta{ - PluginSlug: "test-plugin", - Version: "1.0.0", - JSON: pluginsv0alpha1.MetaJSONData{Id: "test-plugin"}, - CDNURL: "https://cdn.grafana.com", - CreatePluginVersion: "4.15.0", + PluginSlug: "test-plugin", + Version: "1.0.0", + JSON: pluginsv0alpha1.MetaJSONData{Id: "test-plugin"}, + CDNURL: "https://cdn.grafana.com", Manifest: grafanaComPluginManifest{ Files: map[string]string{}, }, @@ -266,13 +268,12 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) require.NotNil(t, result.Meta.Module) - require.NotNil(t, result.Meta.Module.Hash) - assert.Equal(t, "", *result.Meta.Module.Hash) + assert.Nil(t, result.Meta.Module.Hash) }) t.Run("calculates loading strategy correctly", func(t *testing.T) { @@ -298,7 +299,9 @@ func TestCatalogProvider_GetMeta(t *testing.T) { CDNURL: "https://cdn.grafana.com", CreatePluginVersion: tc.createPluginVersion, Manifest: grafanaComPluginManifest{ - Files: map[string]string{"/module.js": "hash123"}, + Files: map[string]string{ + "module.js": "hash123", + }, }, } @@ -308,7 +311,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) @@ -325,15 +328,11 @@ func TestCatalogProvider_GetMeta(t *testing.T) { t.Run("handles empty children list", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { response := grafanaComPluginVersionMeta{ - PluginSlug: "test-plugin", - Version: "1.0.0", - JSON: pluginsv0alpha1.MetaJSONData{Id: "test-plugin"}, - CDNURL: "https://cdn.grafana.com", - CreatePluginVersion: "4.15.0", - Manifest: grafanaComPluginManifest{ - Files: map[string]string{"/module.js": "hash123"}, - }, - Children: []grafanaComChildPluginVersion{}, + PluginSlug: "test-plugin", + Version: "1.0.0", + JSON: pluginsv0alpha1.MetaJSONData{Id: "test-plugin"}, + CDNURL: "https://cdn.grafana.com", + Children: []grafanaComChildPluginVersion{}, } w.Header().Set("Content-Type", "application/json") @@ -342,7 +341,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) @@ -352,14 +351,10 @@ func TestCatalogProvider_GetMeta(t *testing.T) { t.Run("handles missing translations gracefully", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { response := grafanaComPluginVersionMeta{ - PluginSlug: "test-plugin", - Version: "1.0.0", - JSON: pluginsv0alpha1.MetaJSONData{Id: "test-plugin", Languages: []string{"en", "fr"}}, - CDNURL: "https://cdn.grafana.com", - CreatePluginVersion: "4.15.0", - Manifest: grafanaComPluginManifest{ - Files: map[string]string{"/module.js": "hash123"}, - }, + PluginSlug: "test-plugin", + Version: "1.0.0", + JSON: pluginsv0alpha1.MetaJSONData{Id: "test-plugin", Languages: []string{"en", "fr"}}, + CDNURL: "https://cdn.grafana.com", } w.Header().Set("Content-Type", "application/json") @@ -368,7 +363,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") result, err := provider.GetMeta(ctx, PluginRef{ID: "test-plugin", Version: "1.0.0"}) require.NoError(t, err) @@ -381,14 +376,10 @@ func TestCatalogProvider_GetMeta(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { response := grafanaComPluginVersionMeta{ - PluginSlug: parentID, - Version: "1.0.0", - JSON: pluginsv0alpha1.MetaJSONData{Id: parentID}, - CDNURL: "https://cdn.grafana.com", - CreatePluginVersion: "4.15.0", - Manifest: grafanaComPluginManifest{ - Files: map[string]string{"/module.js": "hash123"}, - }, + PluginSlug: parentID, + Version: "1.0.0", + JSON: pluginsv0alpha1.MetaJSONData{Id: parentID}, + CDNURL: "https://cdn.grafana.com", Children: []grafanaComChildPluginVersion{ {Slug: "other-child", JSON: pluginsv0alpha1.MetaJSONData{Id: "other-child"}}, }, @@ -400,7 +391,7 @@ func TestCatalogProvider_GetMeta(t *testing.T) { })) defer server.Close() - provider := NewCatalogProvider(server.URL+"/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, server.URL+"/api/plugins", "") parentIDPtr := parentID result, err := provider.GetMeta(ctx, PluginRef{ ID: childID, @@ -416,14 +407,14 @@ func TestCatalogProvider_GetMeta(t *testing.T) { func TestNewCatalogProvider(t *testing.T) { t.Run("creates provider with default TTL", func(t *testing.T) { - provider := NewCatalogProvider("https://grafana.com/api/plugins", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, "https://grafana.com/api/plugins", "") assert.Equal(t, defaultCatalogTTL, provider.ttl) assert.NotNil(t, provider.httpClient) assert.Equal(t, "https://grafana.com/api/plugins", provider.grafanaComAPIURL) }) t.Run("uses default URL when empty", func(t *testing.T) { - provider := NewCatalogProvider("", "") + provider := NewCatalogProvider(&logging.NoOpLogger{}, "", "") assert.Equal(t, "https://grafana.com/api/plugins", provider.grafanaComAPIURL) }) } @@ -431,17 +422,17 @@ func TestNewCatalogProvider(t *testing.T) { func TestNewCatalogProviderWithTTL(t *testing.T) { t.Run("creates provider with custom TTL", func(t *testing.T) { customTTL := 2 * time.Hour - provider := NewCatalogProviderWithTTL("https://grafana.com/api/plugins", "", customTTL) + provider := NewCatalogProviderWithTTL(&logging.NoOpLogger{}, "https://grafana.com/api/plugins", "", customTTL) assert.Equal(t, customTTL, provider.ttl) }) t.Run("accepts zero TTL", func(t *testing.T) { - provider := NewCatalogProviderWithTTL("https://grafana.com/api/plugins", "", 0) + provider := NewCatalogProviderWithTTL(&logging.NoOpLogger{}, "https://grafana.com/api/plugins", "", 0) assert.Equal(t, time.Duration(0), provider.ttl) }) t.Run("uses default URL when empty", func(t *testing.T) { - provider := NewCatalogProviderWithTTL("", "", defaultCatalogTTL) + provider := NewCatalogProviderWithTTL(&logging.NoOpLogger{}, "", "", defaultCatalogTTL) assert.Equal(t, "https://grafana.com/api/plugins", provider.grafanaComAPIURL) }) } diff --git a/apps/plugins/pkg/app/meta/converter.go b/apps/plugins/pkg/app/meta/converter.go index 46d8326cde4..5c82a5118d5 100644 --- a/apps/plugins/pkg/app/meta/converter.go +++ b/apps/plugins/pkg/app/meta/converter.go @@ -1,7 +1,6 @@ package meta import ( - "context" "encoding/json" "fmt" "net/url" @@ -762,10 +761,10 @@ type grafanaComPluginManifest struct { // grafanaComChildPluginVersionToMetaSpec converts a child plugin version to a MetaSpec. // It inherits most information from the parent plugin. -func grafanaComChildPluginVersionToMetaSpec(ctx context.Context, child grafanaComChildPluginVersion, parent grafanaComPluginVersionMeta) pluginsv0alpha1.MetaSpec { +func grafanaComChildPluginVersionToMetaSpec(logger logging.Logger, child grafanaComChildPluginVersion, parent grafanaComPluginVersionMeta) (pluginsv0alpha1.MetaSpec, error) { cdnURL, err := url.JoinPath(parent.CDNURL, child.Path) if err != nil { - logging.FromContext(ctx).Error("Error getting cdn URL for catalog child meta", "child", child.Slug, "parent", parent.PluginSlug, "err", err) + return pluginsv0alpha1.MetaSpec{}, fmt.Errorf("failed to build CDN URL for child plugin %s: %w", child.Slug, err) } // Create a synthetic meta with both parent and child info @@ -781,11 +780,11 @@ func grafanaComChildPluginVersionToMetaSpec(ctx context.Context, child grafanaCo CDNURL: cdnURL, CreatePluginVersion: parent.CreatePluginVersion, } - return grafanaComPluginVersionMetaToMetaSpec(ctx, childMeta, child.Path) + return grafanaComPluginVersionMetaToMetaSpec(logger, childMeta, child.Path) } // grafanaComPluginVersionMetaToMetaSpec converts a grafanaComPluginVersionMeta to a pluginsv0alpha1.MetaSpec. -func grafanaComPluginVersionMetaToMetaSpec(ctx context.Context, gcomMeta grafanaComPluginVersionMeta, pluginRelBasePath string) pluginsv0alpha1.MetaSpec { +func grafanaComPluginVersionMetaToMetaSpec(logger logging.Logger, gcomMeta grafanaComPluginVersionMeta, pluginRelBasePath string) (pluginsv0alpha1.MetaSpec, error) { metaSpec := pluginsv0alpha1.MetaSpec{ PluginJson: gcomMeta.JSON, Class: pluginsv0alpha1.MetaSpecClassExternal, @@ -823,21 +822,27 @@ func grafanaComPluginVersionMetaToMetaSpec(ctx context.Context, gcomMeta grafana moduleURL, err := url.JoinPath(gcomMeta.CDNURL, "module.js") if err != nil { - logging.FromContext(ctx).Error("Error getting module.js URL for catalog meta", "pluginId", gcomMeta.PluginSlug, "version", gcomMeta.Version) + return pluginsv0alpha1.MetaSpec{}, fmt.Errorf("failed to build module.js URL for plugin %s: %w", gcomMeta.PluginSlug, err) } - modulePath := path.Join(pluginRelBasePath, "module.js") + modulePath := "module.js" + if pluginRelBasePath != "" { + modulePath = path.Join(pluginRelBasePath, modulePath) + } moduleHash, ok := gcomMeta.Manifest.Files[modulePath] if !ok { - logging.FromContext(ctx).Error("Error getting module hash for catalog meta", "pluginId", gcomMeta.PluginSlug, "version", gcomMeta.Version, "path", pluginRelBasePath) + logger.Warn("Module hash not found in manifest", "pluginId", gcomMeta.PluginSlug, "version", gcomMeta.Version, "path", modulePath) } loadingStrategy := calculateLoadingStrategyFromGcomMeta(gcomMeta.CreatePluginVersion) - metaSpec.Module = pluginsv0alpha1.MetaV0alpha1SpecModule{ + module := pluginsv0alpha1.MetaV0alpha1SpecModule{ Path: moduleURL, - Hash: &moduleHash, LoadingStrategy: loadingStrategy, } + if ok { + module.Hash = &moduleHash + } + metaSpec.Module = module metaSpec.BaseURL = gcomMeta.CDNURL children := make([]string, 0, len(gcomMeta.Children)) @@ -848,11 +853,11 @@ func grafanaComPluginVersionMetaToMetaSpec(ctx context.Context, gcomMeta grafana translations, err := translationsFromManifest(gcomMeta.CDNURL, gcomMeta.Manifest.Files, pluginRelBasePath, gcomMeta.JSON) if err != nil { - logging.FromContext(ctx).Warn("Error building translations", "pluginId", gcomMeta.PluginSlug, "version", gcomMeta.Version, "error", err) + logger.Warn("Error building translations", "pluginId", gcomMeta.PluginSlug, "version", gcomMeta.Version, "error", err) } metaSpec.Translations = translations - return metaSpec + return metaSpec, nil } func translationsFromManifest(cdnURL string, manifestFiles map[string]string, pluginRelBasePath string, jsonData pluginsv0alpha1.MetaJSONData) (map[string]string, error) { diff --git a/apps/plugins/pkg/app/meta/converter_test.go b/apps/plugins/pkg/app/meta/converter_test.go index 7ed305f6e8b..3f02163e4fa 100644 --- a/apps/plugins/pkg/app/meta/converter_test.go +++ b/apps/plugins/pkg/app/meta/converter_test.go @@ -1,10 +1,10 @@ package meta import ( - "context" "encoding/json" "testing" + "github.com/grafana/grafana-app-sdk/logging" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -527,8 +527,6 @@ func TestPluginToMetaSpec(t *testing.T) { } func TestGrafanaComChildPluginVersionToMetaSpec(t *testing.T) { - ctx := context.Background() - t.Run("converts child plugin version", func(t *testing.T) { parent := grafanaComPluginVersionMeta{ PluginSlug: "parent-plugin", @@ -549,7 +547,8 @@ func TestGrafanaComChildPluginVersionToMetaSpec(t *testing.T) { JSON: pluginsv0alpha1.MetaJSONData{Id: "child-plugin", Name: "Child Plugin"}, } - meta := grafanaComChildPluginVersionToMetaSpec(ctx, child, parent) + meta, err := grafanaComChildPluginVersionToMetaSpec(&logging.NoOpLogger{}, child, parent) + require.NoError(t, err) assert.Equal(t, "child-plugin", meta.PluginJson.Id) assert.Equal(t, pluginsv0alpha1.MetaSpecClassExternal, meta.Class) @@ -558,8 +557,6 @@ func TestGrafanaComChildPluginVersionToMetaSpec(t *testing.T) { } func TestGrafanaComPluginVersionMetaToMetaSpec(t *testing.T) { - ctx := context.Background() - t.Run("converts plugin version meta with all fields", func(t *testing.T) { gcomMeta := grafanaComPluginVersionMeta{ PluginSlug: "test-plugin", @@ -578,7 +575,8 @@ func TestGrafanaComPluginVersionMetaToMetaSpec(t *testing.T) { }, } - meta := grafanaComPluginVersionMetaToMetaSpec(ctx, gcomMeta, "test-plugin") + meta, err := grafanaComPluginVersionMetaToMetaSpec(&logging.NoOpLogger{}, gcomMeta, "test-plugin") + require.NoError(t, err) assert.Equal(t, "test-plugin", meta.PluginJson.Id) assert.Equal(t, pluginsv0alpha1.MetaSpecClassExternal, meta.Class) assert.Equal(t, pluginsv0alpha1.MetaV0alpha1SpecSignatureStatusValid, meta.Signature.Status) @@ -616,14 +614,15 @@ func TestGrafanaComPluginVersionMetaToMetaSpec(t *testing.T) { }, } - meta := grafanaComPluginVersionMetaToMetaSpec(ctx, gcomMeta, "test-plugin") + meta, err := grafanaComPluginVersionMetaToMetaSpec(&logging.NoOpLogger{}, gcomMeta, "test-plugin") + require.NoError(t, err) require.NotNil(t, meta.Signature.Type) assert.Equal(t, tc.expected, *meta.Signature.Type) }) } }) - t.Run("handles missing module hash", func(t *testing.T) { + t.Run("handles missing module hash gracefully", func(t *testing.T) { gcomMeta := grafanaComPluginVersionMeta{ PluginSlug: "test-plugin", Version: "1.0.0", @@ -635,8 +634,10 @@ func TestGrafanaComPluginVersionMetaToMetaSpec(t *testing.T) { }, } - meta := grafanaComPluginVersionMetaToMetaSpec(ctx, gcomMeta, "test-plugin") - assert.NotNil(t, meta.Module.Hash) + meta, err := grafanaComPluginVersionMetaToMetaSpec(&logging.NoOpLogger{}, gcomMeta, "test-plugin") + require.NoError(t, err) + require.NotNil(t, meta.Module) + assert.Nil(t, meta.Module.Hash) }) } diff --git a/apps/plugins/pkg/app/meta/core.go b/apps/plugins/pkg/app/meta/core.go index 80e2111881e..580714502e8 100644 --- a/apps/plugins/pkg/app/meta/core.go +++ b/apps/plugins/pkg/app/meta/core.go @@ -33,15 +33,16 @@ type CoreProvider struct { ttl time.Duration loader pluginsLoader.Service pluginsPathFunc func() (string, error) + logger logging.Logger } // NewCoreProvider creates a new CoreProvider for core plugins. -func NewCoreProvider(pluginsPath func() (string, error)) *CoreProvider { - return NewCoreProviderWithTTL(pluginsPath, defaultCoreTTL) +func NewCoreProvider(logger logging.Logger, pluginsPath func() (string, error)) *CoreProvider { + return NewCoreProviderWithTTL(logger, pluginsPath, defaultCoreTTL) } // NewCoreProviderWithTTL creates a new CoreProvider with a custom TTL. -func NewCoreProviderWithTTL(pluginsPathFunc func() (string, error), ttl time.Duration) *CoreProvider { +func NewCoreProviderWithTTL(logger logging.Logger, pluginsPathFunc func() (string, error), ttl time.Duration) *CoreProvider { cfg := &config.PluginManagementCfg{ Features: config.Features{}, } @@ -50,6 +51,7 @@ func NewCoreProviderWithTTL(pluginsPathFunc func() (string, error), ttl time.Dur ttl: ttl, loader: createLoader(cfg), pluginsPathFunc: pluginsPathFunc, + logger: logger, } } @@ -80,7 +82,7 @@ func (p *CoreProvider) GetMeta(ctx context.Context, ref PluginRef) (*Result, err if !p.initialized { if err := p.loadPlugins(ctx); err != nil { - logging.FromContext(ctx).Warn("CoreProvider: could not load core plugins, will return ErrMetaNotFound for all lookups", "error", err) + p.logger.WithContext(ctx).Error("Could not load core plugins", "error", err) // Mark as initialized even on failure so we don't keep trying p.initialized = true return nil, ErrMetaNotFound @@ -105,7 +107,7 @@ func (p *CoreProvider) GetMeta(ctx context.Context, ref PluginRef) (*Result, err func (p *CoreProvider) loadPlugins(ctx context.Context) error { pluginsPath, err := p.pluginsPathFunc() if err != nil { - logging.FromContext(ctx).Warn("CoreProvider: could not get plugins path", "error", err) + p.logger.WithContext(ctx).Warn("Could not get core plugins path", "error", err) return err } @@ -116,7 +118,7 @@ func (p *CoreProvider) loadPlugins(ctx context.Context) error { } if len(loadedPlugins) == 0 { - logging.FromContext(ctx).Warn("CoreProvider: no core plugins found during loading") + p.logger.WithContext(ctx).Warn("No core plugins found during loading") return nil } diff --git a/apps/plugins/pkg/app/meta/core_test.go b/apps/plugins/pkg/app/meta/core_test.go index c8e4dc73950..25121cbee5c 100644 --- a/apps/plugins/pkg/app/meta/core_test.go +++ b/apps/plugins/pkg/app/meta/core_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/grafana/grafana-app-sdk/logging" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -20,7 +21,7 @@ func TestCoreProvider_GetMeta(t *testing.T) { ctx := context.Background() t.Run("returns cached plugin when available", func(t *testing.T) { - provider := NewCoreProvider(pluginsPathFunc("")) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc("")) expectedMeta := pluginsv0alpha1.MetaSpec{ PluginJson: pluginsv0alpha1.MetaJSONData{ @@ -44,7 +45,7 @@ func TestCoreProvider_GetMeta(t *testing.T) { }) t.Run("returns ErrMetaNotFound for non-existent plugin", func(t *testing.T) { - provider := NewCoreProvider(pluginsPathFunc("")) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc("")) provider.mu.Lock() provider.initialized = true @@ -58,7 +59,7 @@ func TestCoreProvider_GetMeta(t *testing.T) { }) t.Run("ignores version parameter", func(t *testing.T) { - provider := NewCoreProvider(pluginsPathFunc("")) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc("")) expectedMeta := pluginsv0alpha1.MetaSpec{ PluginJson: pluginsv0alpha1.MetaJSONData{ @@ -83,7 +84,7 @@ func TestCoreProvider_GetMeta(t *testing.T) { t.Run("uses custom TTL when provided", func(t *testing.T) { customTTL := 2 * time.Hour - provider := NewCoreProviderWithTTL(pluginsPathFunc(""), customTTL) + provider := NewCoreProviderWithTTL(&logging.NoOpLogger{}, pluginsPathFunc(""), customTTL) expectedMeta := pluginsv0alpha1.MetaSpec{ PluginJson: pluginsv0alpha1.MetaJSONData{ @@ -116,7 +117,7 @@ func TestCoreProvider_GetMeta(t *testing.T) { require.NoError(t, os.Chdir(tempDir)) - provider := NewCoreProvider(pluginsPathFunc("")) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc("")) result, err := provider.GetMeta(ctx, PluginRef{ID: "any-plugin", Version: "1.0.0"}) assert.Error(t, err) assert.True(t, errors.Is(err, ErrMetaNotFound)) @@ -145,7 +146,7 @@ func TestCoreProvider_loadPlugins(t *testing.T) { require.NoError(t, os.Chdir(grafanaRoot)) pluginsPath := filepath.Join(grafanaRoot, "public", "app", "plugins") - provider := NewCoreProvider(pluginsPathFunc(pluginsPath)) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc(pluginsPath)) err = provider.loadPlugins(ctx) require.NoError(t, err) assert.Len(t, provider.loadedPlugins, 53) @@ -162,7 +163,7 @@ func TestCoreProvider_loadPlugins(t *testing.T) { require.NoError(t, os.Chdir(tempDir)) - provider := NewCoreProvider(func() (string, error) { return "", errors.New("not found") }) + provider := NewCoreProvider(&logging.NoOpLogger{}, func() (string, error) { return "", errors.New("not found") }) err = provider.loadPlugins(ctx) assert.Error(t, err) @@ -182,7 +183,7 @@ func TestCoreProvider_loadPlugins(t *testing.T) { require.NoError(t, os.Chdir(tempDir)) pluginsPath := filepath.Join(tempDir, "public", "app", "plugins") - provider := NewCoreProvider(pluginsPathFunc(pluginsPath)) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc(pluginsPath)) err = provider.loadPlugins(ctx) assert.NoError(t, err) }) @@ -220,7 +221,7 @@ func TestCoreProvider_loadPlugins(t *testing.T) { require.NoError(t, os.Chdir(tempDir)) pluginsPath := filepath.Join(tempDir, "public", "app", "plugins") - provider := NewCoreProvider(pluginsPathFunc(pluginsPath)) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc(pluginsPath)) err = provider.loadPlugins(ctx) if err != nil { @@ -242,7 +243,7 @@ func TestCoreProvider_loadPlugins(t *testing.T) { func TestNewCoreProvider(t *testing.T) { t.Run("creates provider with default TTL", func(t *testing.T) { - provider := NewCoreProvider(pluginsPathFunc("/test/path")) + provider := NewCoreProvider(&logging.NoOpLogger{}, pluginsPathFunc("/test/path")) assert.Equal(t, defaultCoreTTL, provider.ttl) assert.NotNil(t, provider.loadedPlugins) assert.False(t, provider.initialized) @@ -253,18 +254,18 @@ func TestNewCoreProvider(t *testing.T) { func TestNewCoreProviderWithTTL(t *testing.T) { t.Run("creates provider with custom TTL", func(t *testing.T) { customTTL := 2 * time.Hour - provider := NewCoreProviderWithTTL(pluginsPathFunc("/test/path"), customTTL) + provider := NewCoreProviderWithTTL(&logging.NoOpLogger{}, pluginsPathFunc("/test/path"), customTTL) assert.Equal(t, customTTL, provider.ttl) }) t.Run("accepts zero TTL", func(t *testing.T) { - provider := NewCoreProviderWithTTL(pluginsPathFunc("/test/path"), 0) + provider := NewCoreProviderWithTTL(&logging.NoOpLogger{}, pluginsPathFunc("/test/path"), 0) assert.Equal(t, time.Duration(0), provider.ttl) }) t.Run("stores plugins path function", func(t *testing.T) { expectedPath := "/usr/share/grafana/public/app/plugins" - provider := NewCoreProviderWithTTL(pluginsPathFunc(expectedPath), defaultCoreTTL) + provider := NewCoreProviderWithTTL(&logging.NoOpLogger{}, pluginsPathFunc(expectedPath), defaultCoreTTL) assert.NotNil(t, provider.pluginsPathFunc) path, err := provider.pluginsPathFunc() require.NoError(t, err) diff --git a/apps/plugins/pkg/app/metrics/metrics.go b/apps/plugins/pkg/app/metrics/metrics.go new file mode 100644 index 00000000000..bc1a059ffeb --- /dev/null +++ b/apps/plugins/pkg/app/metrics/metrics.go @@ -0,0 +1,82 @@ +package metrics + +import ( + "errors" + + "github.com/prometheus/client_golang/prometheus" +) + +const ( + namespace = "plugins_app" +) + +var ( + // API request metrics + APIRequestsTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: namespace, + Name: "api_requests_total", + Help: "Total number of API requests", + }, + []string{"operation", "status"}, + ) + + // Reconciliation metrics + ChildReconciliationTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: namespace, + Name: "child_reconciliation_total", + Help: "Total number of child plugin reconciliation operations", + }, + []string{"status"}, + ) + + ChildReconciliationDurationSeconds = prometheus.NewHistogram( + prometheus.HistogramOpts{ + Namespace: namespace, + Name: "child_reconciliation_duration_seconds", + Help: "Duration of child plugin reconciliation operations in seconds", + Buckets: prometheus.DefBuckets, + }, + ) + + // Registration metrics + RegistrationOperationsTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: namespace, + Name: "registration_operations_total", + Help: "Total number of registration operations", + }, + []string{"operation", "status"}, + ) + + RegistrationDurationSeconds = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: namespace, + Name: "registration_duration_seconds", + Help: "Duration of registration operations in seconds", + Buckets: prometheus.DefBuckets, + }, + []string{"operation"}, + ) +) + +func MustRegister(registerer prometheus.Registerer) { + metricsToRegister := []prometheus.Collector{ + APIRequestsTotal, + ChildReconciliationTotal, + ChildReconciliationDurationSeconds, + RegistrationOperationsTotal, + RegistrationDurationSeconds, + } + + for _, metric := range metricsToRegister { + if err := registerer.Register(metric); err != nil { + var alreadyRegistered prometheus.AlreadyRegisteredError + if errors.As(err, &alreadyRegistered) { + continue + } + panic(err) + } + } +} diff --git a/apps/plugins/pkg/app/storage.go b/apps/plugins/pkg/app/storage.go index 28ebadf8b0e..1565d257047 100644 --- a/apps/plugins/pkg/app/storage.go +++ b/apps/plugins/pkg/app/storage.go @@ -18,6 +18,7 @@ import ( pluginsv0alpha1 "github.com/grafana/grafana/apps/plugins/pkg/apis/plugins/v0alpha1" "github.com/grafana/grafana/apps/plugins/pkg/app/meta" + "github.com/grafana/grafana/apps/plugins/pkg/app/metrics" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" ) @@ -36,12 +37,14 @@ type MetaStorage struct { clientFactory func(context.Context) (*pluginsv0alpha1.PluginClient, error) clientErr error clientOnce sync.Once + logger logging.Logger gr schema.GroupResource tableConverter rest.TableConvertor } func NewMetaStorage( + logger logging.Logger, metaManager *meta.ProviderManager, clientFactory func(context.Context) (*pluginsv0alpha1.PluginClient, error), ) *MetaStorage { @@ -53,6 +56,7 @@ func NewMetaStorage( return &MetaStorage{ metaManager: metaManager, clientFactory: clientFactory, + logger: logger, gr: gr, tableConverter: rest.NewDefaultTableConvertor(gr), } @@ -100,14 +104,18 @@ func (s *MetaStorage) List(ctx context.Context, options *internalversion.ListOpt return nil, err } + logger := s.logger.WithContext(ctx).With("requestNamespace", ns.Value) + pluginClient, err := s.getClient(ctx) if err != nil { + metrics.APIRequestsTotal.WithLabelValues("list", "error").Inc() return nil, apierrors.NewInternalError(fmt.Errorf("failed to get plugin client: %w", err)) } plugins, err := pluginClient.ListAll(ctx, ns.Value, resource.ListOptions{}) if err != nil { - logging.FromContext(ctx).Error("Failed to list plugins", "namespace", ns.Value, "error", err) + logger.Error("Failed to list plugins", "error", err) + metrics.APIRequestsTotal.WithLabelValues("list", "error").Inc() return nil, apierrors.NewInternalError(fmt.Errorf("failed to list plugins: %w", err)) } @@ -121,7 +129,7 @@ func (s *MetaStorage) List(ctx context.Context, options *internalversion.ListOpt }) if err != nil { // Log error but continue with other plugins - logging.FromContext(ctx).Warn("Failed to fetch metadata for plugin", "pluginId", plugin.Spec.Id, "version", plugin.Spec.Version, "error", err) + logger.Warn("Failed to fetch metadata for plugin", "pluginId", plugin.Spec.Id, "version", plugin.Spec.Version, "error", err) continue } @@ -148,6 +156,7 @@ func (s *MetaStorage) List(ctx context.Context, options *internalversion.ListOpt Items: metaItems, } + metrics.APIRequestsTotal.WithLabelValues("list", "success").Inc() return list, nil } @@ -157,8 +166,11 @@ func (s *MetaStorage) Get(ctx context.Context, name string, options *metav1.GetO return nil, err } + logger := s.logger.WithContext(ctx).With("requestNamespace", ns.Value) + pluginClient, err := s.getClient(ctx) if err != nil { + metrics.APIRequestsTotal.WithLabelValues("get", "error").Inc() return nil, apierrors.NewInternalError(fmt.Errorf("failed to get plugin client: %w", err)) } @@ -167,6 +179,7 @@ func (s *MetaStorage) Get(ctx context.Context, name string, options *metav1.GetO Name: name, }) if err != nil { + metrics.APIRequestsTotal.WithLabelValues("get", "error").Inc() return nil, err } @@ -181,10 +194,12 @@ func (s *MetaStorage) Get(ctx context.Context, name string, options *metav1.GetO Group: pluginsv0alpha1.APIGroup, Resource: name, } + metrics.APIRequestsTotal.WithLabelValues("get", "error").Inc() return nil, apierrors.NewNotFound(gr, plugin.Spec.Id) } - logging.FromContext(ctx).Error("Failed to fetch plugin metadata", "pluginId", plugin.Spec.Id, "version", plugin.Spec.Version, "error", err) + logger.Error("Failed to fetch plugin metadata", "pluginId", plugin.Spec.Id, "version", plugin.Spec.Version, "error", err) + metrics.APIRequestsTotal.WithLabelValues("get", "error").Inc() return nil, apierrors.NewInternalError(fmt.Errorf("failed to fetch plugin metadata: %w", err)) } @@ -201,5 +216,6 @@ func (s *MetaStorage) Get(ctx context.Context, name string, options *metav1.GetO Kind: pluginsv0alpha1.MetaKind().Kind(), }) + metrics.APIRequestsTotal.WithLabelValues("get", "success").Inc() return pluginMeta, nil } diff --git a/pkg/registry/apps/plugins/register.go b/pkg/registry/apps/plugins/register.go index ffacf83eb79..c456001715d 100644 --- a/pkg/registry/apps/plugins/register.go +++ b/pkg/registry/apps/plugins/register.go @@ -9,9 +9,12 @@ import ( authlib "github.com/grafana/authlib/types" appsdkapiserver "github.com/grafana/grafana-app-sdk/k8s/apiserver" + "github.com/grafana/grafana-app-sdk/logging" + "github.com/prometheus/client_golang/prometheus" pluginsapp "github.com/grafana/grafana/apps/plugins/pkg/app" "github.com/grafana/grafana/apps/plugins/pkg/app/meta" + "github.com/grafana/grafana/apps/plugins/pkg/app/metrics" "github.com/grafana/grafana/pkg/configprovider" "github.com/grafana/grafana/pkg/plugins/pluginassets/modulehash" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -40,8 +43,10 @@ func ProvideAppInstaller( restConfigProvider apiserver.RestConfigProvider, pluginStore pluginstore.Store, moduleHashCalc *modulehash.Calculator, accessControlService accesscontrol.Service, accessClient authlib.AccessClient, - features featuremgmt.FeatureToggles, + features featuremgmt.FeatureToggles, registerer prometheus.Registerer, ) (*AppInstaller, error) { + metrics.MustRegister(registerer) + //nolint:staticcheck // not yet migrated to OpenFeature if features.IsEnabledGlobally(featuremgmt.FlagPluginStoreServiceLoading) { if err := registerAccessControlRoles(accessControlService); err != nil { @@ -49,13 +54,15 @@ func ProvideAppInstaller( } } + logger := logging.DefaultLogger.With("app", "plugins.app") + localProvider := meta.NewLocalProvider(pluginStore, moduleHashCalc) - coreProvider := meta.NewCoreProvider(func() (string, error) { + coreProvider := meta.NewCoreProvider(logger, func() (string, error) { return getPluginsPath(cfgProvider) }) metaProviderManager := meta.NewProviderManager(coreProvider, localProvider) authorizer := grafanaauthorizer.NewResourceAuthorizer(accessClient) - i, err := pluginsapp.ProvideAppInstaller(authorizer, metaProviderManager, false) + i, err := pluginsapp.NewPluginsAppInstaller(logger, authorizer, metaProviderManager, false) if err != nil { return nil, err } diff --git a/pkg/server/wire_gen.go b/pkg/server/wire_gen.go index 8ae4145ac70..d98c41f587f 100644 --- a/pkg/server/wire_gen.go +++ b/pkg/server/wire_gen.go @@ -793,7 +793,7 @@ func Initialize(ctx context.Context, cfg *setting.Cfg, opts Options, apiOpts api if err != nil { return nil, err } - pluginsAppInstaller, err := plugins.ProvideAppInstaller(configProvider, eventualRestConfigProvider, pluginstoreService, calculator, acimplService, accessClient, featureToggles) + pluginsAppInstaller, err := plugins.ProvideAppInstaller(configProvider, eventualRestConfigProvider, pluginstoreService, calculator, acimplService, accessClient, featureToggles, registerer) if err != nil { return nil, err } @@ -1479,7 +1479,7 @@ func InitializeForTest(ctx context.Context, t sqlutil.ITestDB, testingT interfac if err != nil { return nil, err } - pluginsAppInstaller, err := plugins.ProvideAppInstaller(configProvider, eventualRestConfigProvider, pluginstoreService, calculator, acimplService, accessClient, featureToggles) + pluginsAppInstaller, err := plugins.ProvideAppInstaller(configProvider, eventualRestConfigProvider, pluginstoreService, calculator, acimplService, accessClient, featureToggles, registerer) if err != nil { return nil, err } diff --git a/pkg/services/pluginsintegration/installsync/syncer.go b/pkg/services/pluginsintegration/installsync/syncer.go index b8bc1dc47a0..41d3503e916 100644 --- a/pkg/services/pluginsintegration/installsync/syncer.go +++ b/pkg/services/pluginsintegration/installsync/syncer.go @@ -98,7 +98,7 @@ func ProvideSyncer( if err != nil { return nil, err } - installRegistrar := install.NewInstallRegistrar(clientGenerator) + installRegistrar := install.NewInstallRegistrar(logging.DefaultLogger, clientGenerator) namespaceMapper := request.GetNamespaceMapper(cfg) return newSyncer( diff --git a/pkg/services/pluginsintegration/installsync/syncer_test.go b/pkg/services/pluginsintegration/installsync/syncer_test.go index 8eedc744ca5..603bc7a99e3 100644 --- a/pkg/services/pluginsintegration/installsync/syncer_test.go +++ b/pkg/services/pluginsintegration/installsync/syncer_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana-app-sdk/resource" "github.com/stretchr/testify/require" errorsK8s "k8s.io/apimachinery/pkg/api/errors" @@ -147,7 +148,7 @@ func TestSyncer_Sync(t *testing.T) { }, } clientGen := &fakeClientGenerator{client: fakeClient} - registrar := install.NewInstallRegistrar(clientGen) + registrar := install.NewInstallRegistrar(&logging.NoOpLogger{}, clientGen) // Create syncer s := newSyncer( @@ -320,7 +321,7 @@ func TestSyncer_syncNamespace(t *testing.T) { } clientGen := &fakeClientGenerator{client: fakeClient} - registrar := install.NewInstallRegistrar(clientGen) + registrar := install.NewInstallRegistrar(&logging.NoOpLogger{}, clientGen) // Create syncer s := newSyncer( @@ -379,7 +380,7 @@ func TestInstallRegistrar_GetClient(t *testing.T) { s := newSyncer( featuremgmt.NewMockFeatureToggles(t), clientGen, - install.NewInstallRegistrar(clientGen), + install.NewInstallRegistrar(&logging.NoOpLogger{}, clientGen), orgtest.NewOrgServiceFake(), func(orgID int64) string { return "org-1" }, &fakeServerLock{},