From 63e1d52663812f4663c50d65dd81252a7d939c98 Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Mon, 22 Sep 2025 15:53:50 -0600 Subject: [PATCH] Provisioning: Add metrics around webhooks (#111453) --- .../apis/provisioning/webhooks/metrics.go | 30 ++++++ .../webhooks/pullrequest/changes.go | 19 +++- .../webhooks/pullrequest/changes_test.go | 6 +- .../webhooks/pullrequest/metrics.go | 97 +++++++++++++++++++ .../webhooks/pullrequest/worker.go | 32 +++++- .../webhooks/pullrequest/worker_test.go | 12 ++- .../apis/provisioning/webhooks/register.go | 4 +- .../apis/provisioning/webhooks/webhook.go | 10 ++ pkg/server/wire_gen.go | 4 +- 9 files changed, 196 insertions(+), 18 deletions(-) create mode 100644 pkg/registry/apis/provisioning/webhooks/metrics.go create mode 100644 pkg/registry/apis/provisioning/webhooks/pullrequest/metrics.go diff --git a/pkg/registry/apis/provisioning/webhooks/metrics.go b/pkg/registry/apis/provisioning/webhooks/metrics.go new file mode 100644 index 00000000000..1323c12233d --- /dev/null +++ b/pkg/registry/apis/provisioning/webhooks/metrics.go @@ -0,0 +1,30 @@ +package webhooks + +import ( + "github.com/prometheus/client_golang/prometheus" +) + +type webhookMetrics struct { + registry prometheus.Registerer + eventsProcessed *prometheus.CounterVec +} + +func registerWebhookMetrics(registry prometheus.Registerer) webhookMetrics { + eventsProcessed := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "grafana_provisioning_webhook_events_processed_total", + Help: "Total number of webhook events processed and what job type was queued", + }, + []string{"job"}, + ) + registry.MustRegister(eventsProcessed) + + return webhookMetrics{ + registry: registry, + eventsProcessed: eventsProcessed, + } +} + +func (m *webhookMetrics) recordEventProcessed(jobQueued string) { + m.eventsProcessed.WithLabelValues(jobQueued).Inc() +} diff --git a/pkg/registry/apis/provisioning/webhooks/pullrequest/changes.go b/pkg/registry/apis/provisioning/webhooks/pullrequest/changes.go index 6f50fbfba31..4c9a49a7db4 100644 --- a/pkg/registry/apis/provisioning/webhooks/pullrequest/changes.go +++ b/pkg/registry/apis/provisioning/webhooks/pullrequest/changes.go @@ -6,6 +6,7 @@ import ( "net/url" "path" "strings" + "time" "github.com/grafana/grafana-app-sdk/logging" dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1beta1" @@ -15,6 +16,8 @@ import ( "github.com/grafana/grafana/pkg/infra/slugify" "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs" "github.com/grafana/grafana/pkg/registry/apis/provisioning/resources" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/utils" + "github.com/prometheus/client_golang/prometheus" ) type changeInfo struct { @@ -53,13 +56,16 @@ type evaluator struct { render ScreenshotRenderer parsers resources.ParserFactory urlProvider func(namespace string) string + metrics screenshotMetrics } -func NewEvaluator(render ScreenshotRenderer, parsers resources.ParserFactory, urlProvider func(namespace string) string) Evaluator { +func NewEvaluator(render ScreenshotRenderer, parsers resources.ParserFactory, urlProvider func(namespace string) string, registry prometheus.Registerer) Evaluator { + metrics := registerScreenshotMetrics(registry) return &evaluator{ render: render, parsers: parsers, urlProvider: urlProvider, + metrics: metrics, } } @@ -152,14 +158,14 @@ func (e *evaluator) evaluateFile(ctx context.Context, repo repository.Reader, ba info.PreviewURL += "?" + query.Encode() if shouldRender { if info.GrafanaURL != "" { - info.GrafanaScreenshotURL, err = renderScreenshotFromGrafanaURL(ctx, baseURL, e.render, info.Parsed.Repo, info.GrafanaURL) + info.GrafanaScreenshotURL, err = renderScreenshotFromGrafanaURL(ctx, baseURL, e.render, info.Parsed.Repo, info.GrafanaURL, e.metrics) if err != nil { info.Error = err.Error() } } if info.PreviewURL != "" { - info.PreviewScreenshotURL, err = renderScreenshotFromGrafanaURL(ctx, baseURL, e.render, info.Parsed.Repo, info.PreviewURL) + info.PreviewScreenshotURL, err = renderScreenshotFromGrafanaURL(ctx, baseURL, e.render, info.Parsed.Repo, info.PreviewURL, e.metrics) if err != nil { info.Error = err.Error() } @@ -175,7 +181,13 @@ func renderScreenshotFromGrafanaURL(ctx context.Context, renderer ScreenshotRenderer, repo provisioning.ResourceRepositoryInfo, grafanaURL string, + metrics screenshotMetrics, ) (string, error) { + outcome := utils.ErrorOutcome + duration := time.Now() + defer func() { + metrics.recordScreenshotDuration(outcome, time.Since(duration)) + }() parsed, err := url.Parse(grafanaURL) if err != nil { logging.FromContext(ctx).Warn("invalid", "url", grafanaURL, "err", err) @@ -194,5 +206,6 @@ func renderScreenshotFromGrafanaURL(ctx context.Context, logger.Warn("invalid base", "url", baseURL, "err", err) return "", err } + outcome = utils.SuccessOutcome return base.JoinPath(snap).String(), nil } diff --git a/pkg/registry/apis/provisioning/webhooks/pullrequest/changes_test.go b/pkg/registry/apis/provisioning/webhooks/pullrequest/changes_test.go index fef0cebfc16..1d647256a2c 100644 --- a/pkg/registry/apis/provisioning/webhooks/pullrequest/changes_test.go +++ b/pkg/registry/apis/provisioning/webhooks/pullrequest/changes_test.go @@ -7,6 +7,7 @@ import ( "fmt" "testing" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -759,7 +760,7 @@ func TestCalculateChanges(t *testing.T) { } return "http://host/" - }) + }, prometheus.NewPedanticRegistry()) pullRequest := provisioning.PullRequestJobOptions{ Ref: "ref", @@ -890,6 +891,7 @@ func TestRenderScreenshotFromGrafanaURL(t *testing.T) { }, } + metrics := registerScreenshotMetrics(prometheus.NewPedanticRegistry()) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { renderer := NewMockScreenshotRenderer(t) @@ -900,7 +902,7 @@ func TestRenderScreenshotFromGrafanaURL(t *testing.T) { Name: "repo", } - got, err := renderScreenshotFromGrafanaURL(context.Background(), tt.baseURL, renderer, repo, tt.grafanaURL) + got, err := renderScreenshotFromGrafanaURL(context.Background(), tt.baseURL, renderer, repo, tt.grafanaURL, metrics) if tt.wantErr != "" { require.Error(t, err) require.Contains(t, err.Error(), tt.wantErr) diff --git a/pkg/registry/apis/provisioning/webhooks/pullrequest/metrics.go b/pkg/registry/apis/provisioning/webhooks/pullrequest/metrics.go new file mode 100644 index 00000000000..651f07795bb --- /dev/null +++ b/pkg/registry/apis/provisioning/webhooks/pullrequest/metrics.go @@ -0,0 +1,97 @@ +package pullrequest + +import ( + "sync" + "time" + + "github.com/prometheus/client_golang/prometheus" +) + +var ( + pullRequestMetricsOnce sync.Once + pullRequestMetricsInstance pullRequestMetrics +) + +type pullRequestMetrics struct { + registry prometheus.Registerer + processingDuration *prometheus.HistogramVec + commentsPosted *prometheus.CounterVec +} + +func registerPullRequestMetrics(registry prometheus.Registerer) pullRequestMetrics { + // called by both ProvidePullRequestWorker and ProvideWebhooksWithImages + // this ensures we only register the metric once + pullRequestMetricsOnce.Do(func() { + processingDuration := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "grafana_provisioning_pullrequest_processing_duration_seconds", + Help: "Duration of pull request processing", + Buckets: []float64{0.5, 1.0, 2.0, 5.0, 10.0, 30.0}, + }, + []string{"outcome"}, + ) + registry.MustRegister(processingDuration) + + commentsPosted := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "grafana_provisioning_pullrequest_comments_posted_total", + Help: "Total number of comments posted to pull requests", + }, + []string{"outcome"}, + ) + registry.MustRegister(commentsPosted) + + pullRequestMetricsInstance = pullRequestMetrics{ + registry: registry, + processingDuration: processingDuration, + commentsPosted: commentsPosted, + } + }) + + return pullRequestMetricsInstance +} + +func (m *pullRequestMetrics) recordProcessed(outcome string, duration time.Duration) { + m.processingDuration.WithLabelValues(outcome).Observe(duration.Seconds()) +} + +func (m *pullRequestMetrics) recordCommentPosted(outcome string) { + m.commentsPosted.WithLabelValues(outcome).Inc() +} + +type screenshotMetrics struct { + registry prometheus.Registerer + screenshotDuration *prometheus.HistogramVec +} + +var ( + screenshotMetricsOnce sync.Once + screenshotMetricsInstance screenshotMetrics +) + +func registerScreenshotMetrics(registry prometheus.Registerer) screenshotMetrics { + // called by both ProvidePullRequestWorker and ProvideWebhooksWithImages + // this ensures we only register the metric once + screenshotMetricsOnce.Do(func() { + screenshotDuration := prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "grafana_provisioning_pullrequest_screenshot_duration_seconds", + Help: "Duration of screenshot generation", + Buckets: []float64{1.0, 2.0, 5.0, 10.0, 30.0, 60.0}, + }, + []string{"outcome"}, + ) + registry.MustRegister(screenshotDuration) + + screenshotMetricsInstance = screenshotMetrics{ + registry: registry, + screenshotDuration: screenshotDuration, + } + }) + + return screenshotMetricsInstance +} + +func (m *screenshotMetrics) recordScreenshotDuration(outcome string, duration time.Duration) { + m.screenshotDuration.WithLabelValues(outcome).Observe(duration.Seconds()) +} diff --git a/pkg/registry/apis/provisioning/webhooks/pullrequest/worker.go b/pkg/registry/apis/provisioning/webhooks/pullrequest/worker.go index 1291c590148..299febe9786 100644 --- a/pkg/registry/apis/provisioning/webhooks/pullrequest/worker.go +++ b/pkg/registry/apis/provisioning/webhooks/pullrequest/worker.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "time" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -12,10 +13,12 @@ import ( "github.com/grafana/grafana/apps/provisioning/pkg/repository" "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs" "github.com/grafana/grafana/pkg/registry/apis/provisioning/resources" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/utils" "github.com/grafana/grafana/pkg/services/apiserver" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/storage/unified/resource" + "github.com/prometheus/client_golang/prometheus" ) func ProvidePullRequestWorker( @@ -23,6 +26,7 @@ func ProvidePullRequestWorker( renderer rendering.Service, blobstore resource.ResourceClient, configProvider apiserver.RestConfigProvider, + registry prometheus.Registerer, ) *PullRequestWorker { urlProvider := func(_ string) string { return cfg.AppURL @@ -33,10 +37,10 @@ func ProvidePullRequestWorker( clients := resources.NewClientFactory(configProvider) parsers := resources.NewParserFactory(clients) screenshotRenderer := NewScreenshotRenderer(renderer, blobstore) - evaluator := NewEvaluator(screenshotRenderer, parsers, urlProvider) + evaluator := NewEvaluator(screenshotRenderer, parsers, urlProvider, registry) commenter := NewCommenter() - return NewPullRequestWorker(evaluator, commenter) + return NewPullRequestWorker(evaluator, commenter, registry) } //go:generate mockery --name=PullRequestRepo --structname=MockPullRequestRepo --inpackage --filename=mock_pullrequest_repo.go --with-expecter @@ -60,12 +64,15 @@ type Commenter interface { type PullRequestWorker struct { evaluator Evaluator commenter Commenter + metrics pullRequestMetrics } -func NewPullRequestWorker(evaluator Evaluator, commenter Commenter) *PullRequestWorker { +func NewPullRequestWorker(evaluator Evaluator, commenter Commenter, registry prometheus.Registerer) *PullRequestWorker { + metrics := registerPullRequestMetrics(registry) return &PullRequestWorker{ evaluator: evaluator, commenter: commenter, + metrics: metrics, } } @@ -80,30 +87,42 @@ func (c *PullRequestWorker) Process(ctx context.Context, ) error { cfg := repo.Config().Spec opts := job.Spec.PullRequest + startTime := time.Now() + outcome := utils.ErrorOutcome + defer func() { + duration := time.Since(startTime) + c.metrics.recordProcessed(outcome, duration) + }() + if opts == nil { return apierrors.NewBadRequest("missing spec.pr") } + logger := logging.FromContext(ctx).With("pr", opts.PR, "repo", repo.Config().GetName(), "namespace", job.GetNamespace()) + if opts.Ref == "" { + logger.Debug("missing spec.ref") return apierrors.NewBadRequest("missing spec.ref") } // FIXME: this is leaky because it's supposed to be already a PullRequestRepo if cfg.GitHub == nil { + logger.Debug("expecting github configuration") return apierrors.NewBadRequest("expecting github configuration") } reader, ok := repo.(repository.Reader) if !ok { + logger.Debug("pull request job submitted targeting repository that is not a Reader") return errors.New("pull request job submitted targeting repository that is not a Reader") } prRepo, ok := repo.(PullRequestRepo) if !ok { + logger.Debug("pull request job submitted targeting repository that is not a PullRequestRepo") return fmt.Errorf("repository is not a pull request repository") } - logger := logging.FromContext(ctx).With("pr", opts.PR) logger.Info("process pull request") defer logger.Info("pull request processed") @@ -112,6 +131,7 @@ func (c *PullRequestWorker) Process(ctx context.Context, base := cfg.GitHub.Branch files, err := prRepo.CompareFiles(ctx, base, opts.Ref) if err != nil { + logger.Error("failed to list pull request files", "error", err) return fmt.Errorf("failed to list pull request files: %w", err) } @@ -123,12 +143,16 @@ func (c *PullRequestWorker) Process(ctx context.Context, changeInfo, err := c.evaluator.Evaluate(ctx, reader, *opts, files, progress) if err != nil { + logger.Error("failed to calculate changes", "error", err) return fmt.Errorf("calculate changes: %w", err) } if err := c.commenter.Comment(ctx, prRepo, opts.PR, changeInfo); err != nil { + c.metrics.recordCommentPosted(utils.ErrorOutcome) return fmt.Errorf("comment pull request: %w", err) } + outcome = utils.SuccessOutcome + c.metrics.recordCommentPosted(utils.SuccessOutcome) logger.Info("preview comment added") return nil diff --git a/pkg/registry/apis/provisioning/webhooks/pullrequest/worker_test.go b/pkg/registry/apis/provisioning/webhooks/pullrequest/worker_test.go index 7f1cb4f14a0..3414703a5bf 100644 --- a/pkg/registry/apis/provisioning/webhooks/pullrequest/worker_test.go +++ b/pkg/registry/apis/provisioning/webhooks/pullrequest/worker_test.go @@ -5,10 +5,12 @@ import ( "errors" "testing" + "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/grafana/grafana-app-sdk/logging" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/apps/provisioning/pkg/repository" "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs" @@ -44,7 +46,7 @@ func TestPullRequestWorker_IsSupported(t *testing.T) { t.Run(tt.name, func(t *testing.T) { evaluator := NewMockEvaluator(t) commenter := NewMockCommenter(t) - worker := NewPullRequestWorker(evaluator, commenter) + worker := NewPullRequestWorker(evaluator, commenter, prometheus.NewPedanticRegistry()) result := worker.IsSupported(context.Background(), tt.job) require.Equal(t, tt.expected, result) }) @@ -68,7 +70,7 @@ func TestPullRequestWorker_Process_NotPullRequestRepository(t *testing.T) { }, }) - worker := NewPullRequestWorker(evaluator, commenter) + worker := NewPullRequestWorker(evaluator, commenter, prometheus.NewPedanticRegistry()) job := provisioning.Job{ Spec: provisioning.JobSpec{ Action: provisioning.JobActionPullRequest, @@ -106,7 +108,7 @@ func TestPullRequestWorker_Process_NotReaderRepository(t *testing.T) { }, }) - worker := NewPullRequestWorker(evaluator, commenter) + worker := NewPullRequestWorker(evaluator, commenter, prometheus.NewPedanticRegistry()) job := provisioning.Job{ Spec: provisioning.JobSpec{ Action: provisioning.JobActionPullRequest, @@ -392,7 +394,7 @@ func TestPullRequestWorker_Process(t *testing.T) { progress := jobs.NewMockJobProgressRecorder(t) tt.setupMocks(evaluator, commenter, &repo, progress) - worker := NewPullRequestWorker(evaluator, commenter) + worker := NewPullRequestWorker(evaluator, commenter, prometheus.NewPedanticRegistry()) job := provisioning.Job{ Spec: provisioning.JobSpec{ Action: provisioning.JobActionPullRequest, @@ -400,7 +402,7 @@ func TestPullRequestWorker_Process(t *testing.T) { }, } - err := worker.Process(context.Background(), repo, job, progress) + err := worker.Process(logging.Context(context.Background(), logging.DefaultLogger), repo, job, progress) if tt.expectedError != "" { require.EqualError(t, err, tt.expectedError) } else { diff --git a/pkg/registry/apis/provisioning/webhooks/register.go b/pkg/registry/apis/provisioning/webhooks/register.go index 6dc55a5b0de..4fc02db9041 100644 --- a/pkg/registry/apis/provisioning/webhooks/register.go +++ b/pkg/registry/apis/provisioning/webhooks/register.go @@ -87,9 +87,9 @@ func ProvideWebhooksWithImages( registry, ) - evaluator := pullrequest.NewEvaluator(screenshotRenderer, parsers, urlProvider) + evaluator := pullrequest.NewEvaluator(screenshotRenderer, parsers, urlProvider, registry) commenter := pullrequest.NewCommenter() - pullRequestWorker := pullrequest.NewPullRequestWorker(evaluator, commenter) + pullRequestWorker := pullrequest.NewPullRequestWorker(evaluator, commenter, registry) return NewWebhookExtraWithImages( render, diff --git a/pkg/registry/apis/provisioning/webhooks/webhook.go b/pkg/registry/apis/provisioning/webhooks/webhook.go index 321f08fdafb..4e5bfffb28a 100644 --- a/pkg/registry/apis/provisioning/webhooks/webhook.go +++ b/pkg/registry/apis/provisioning/webhooks/webhook.go @@ -36,6 +36,7 @@ type webhookConnector struct { core *provisioningapis.APIBuilder renderer pullrequest.ScreenshotRenderer registry prometheus.Registerer + metrics webhookMetrics } func NewWebhookConnector( @@ -45,11 +46,13 @@ func NewWebhookConnector( renderer pullrequest.ScreenshotRenderer, registry prometheus.Registerer, ) *webhookConnector { + metrics := registerWebhookMetrics(registry) return &webhookConnector{ webhooksEnabled: webhooksEnabled, core: core, renderer: renderer, registry: registry, + metrics: metrics, } } @@ -151,10 +154,17 @@ func (s *webhookConnector) Connect(ctx context.Context, name string, opts runtim logger.Error("failed to update last event", "error", err) } + actionTaken := "none" + defer func() { + s.metrics.recordEventProcessed(actionTaken) + }() + if rsp.Job != nil { rsp.Job.Repository = name + actionTaken = string(rsp.Job.Action) job, err := s.core.GetJobQueue().Insert(ctx, namespace, *rsp.Job) if err != nil { + logger.Error("failed to insert job", "error", err) responder.Error(err) return } diff --git a/pkg/server/wire_gen.go b/pkg/server/wire_gen.go index fa09706c01c..4a33e8e41b9 100644 --- a/pkg/server/wire_gen.go +++ b/pkg/server/wire_gen.go @@ -829,7 +829,7 @@ func Initialize(ctx context.Context, cfg *setting.Cfg, opts Options, apiOpts api legacyMigrator := legacy.ProvideLegacyMigrator(sqlStore, provisioningServiceImpl, libraryPanelService, dashboardPermissionsService, accessControl, featureToggles) webhookExtraBuilder := webhooks.ProvideWebhooksWithImages(cfg, renderingService, resourceClient, eventualRestConfigProvider, registerer) v3 := extras.ProvideProvisioningExtraAPIs(webhookExtraBuilder) - pullRequestWorker := pullrequest.ProvidePullRequestWorker(cfg, renderingService, resourceClient, eventualRestConfigProvider) + pullRequestWorker := pullrequest.ProvidePullRequestWorker(cfg, renderingService, resourceClient, eventualRestConfigProvider, registerer) v4 := extras.ProvideExtraWorkers(pullRequestWorker) v5 := _wireValue decryptAuthorizer := decrypt.ProvideDecryptAuthorizer(tracer, v5) @@ -1434,7 +1434,7 @@ func InitializeForTest(ctx context.Context, t sqlutil.ITestDB, testingT interfac legacyMigrator := legacy.ProvideLegacyMigrator(sqlStore, provisioningServiceImpl, libraryPanelService, dashboardPermissionsService, accessControl, featureToggles) webhookExtraBuilder := webhooks.ProvideWebhooksWithImages(cfg, renderingService, resourceClient, eventualRestConfigProvider, registerer) v3 := extras.ProvideProvisioningExtraAPIs(webhookExtraBuilder) - pullRequestWorker := pullrequest.ProvidePullRequestWorker(cfg, renderingService, resourceClient, eventualRestConfigProvider) + pullRequestWorker := pullrequest.ProvidePullRequestWorker(cfg, renderingService, resourceClient, eventualRestConfigProvider, registerer) v4 := extras.ProvideExtraWorkers(pullRequestWorker) v5 := _wireValue decryptAuthorizer := decrypt.ProvideDecryptAuthorizer(tracer, v5)