From 74cf7aef501eb34990e635b456681cd7026d8dfc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Aug 2025 10:17:19 -0500 Subject: [PATCH 1/4] chore: add profiling labels for pprof analysis PProf labels segment the code into groups for determing the source of cpu/memory profiles. Since the web server and background jobs share a lot of the same code (eg wsbuilder), it helps to know if the load is user induced, or background job based. --- cli/server.go | 9 ++++---- coderd/autobuild/lifecycle_executor.go | 5 +++-- coderd/coderd.go | 1 + coderd/httpmw/pprof.go | 22 +++++++++++++++++++ coderd/pproflabel/pproflabel.go | 22 +++++++++++++++++++ .../insights/metricscollector.go | 5 +++-- enterprise/wsproxy/wsproxy.go | 1 + 7 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 coderd/httpmw/pprof.go create mode 100644 coderd/pproflabel/pproflabel.go diff --git a/cli/server.go b/cli/server.go index 26d0c8f110403..f9e744761b22e 100644 --- a/cli/server.go +++ b/cli/server.go @@ -55,6 +55,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" + "github.com/coder/coder/v2/coderd/pproflabel" "github.com/coder/pretty" "github.com/coder/quartz" "github.com/coder/retry" @@ -1459,14 +1460,14 @@ func newProvisionerDaemon( tracer := coderAPI.TracerProvider.Tracer(tracing.TracerName) terraformClient, terraformServer := drpcsdk.MemTransportPipe() wg.Add(1) - go func() { + pproflabel.Go(ctx, pproflabel.Service(pproflabel.ServiceTerraformProvisioner), func(ctx context.Context) { defer wg.Done() <-ctx.Done() _ = terraformClient.Close() _ = terraformServer.Close() - }() + }) wg.Add(1) - go func() { + pproflabel.Go(ctx, pproflabel.Service(pproflabel.ServiceTerraformProvisioner), func(ctx context.Context) { defer wg.Done() defer cancel() @@ -1485,7 +1486,7 @@ func newProvisionerDaemon( default: } } - }() + }) connector[string(database.ProvisionerTypeTerraform)] = sdkproto.NewDRPCProvisionerClient(terraformClient) default: diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index 234a72de04c50..a8ddc61104b91 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -20,6 +20,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/files" + "github.com/coder/coder/v2/coderd/pproflabel" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" @@ -107,7 +108,7 @@ func (e *Executor) WithStatsChannel(ch chan<- Stats) *Executor { // tick from its channel. It will stop when its context is Done, or when // its channel is closed. func (e *Executor) Run() { - go func() { + pproflabel.Go(e.ctx, pproflabel.Service(pproflabel.ServiceLifecycles), func(ctx context.Context) { for { select { case <-e.ctx.Done(): @@ -128,7 +129,7 @@ func (e *Executor) Run() { e.log.Debug(e.ctx, "run stats", slog.F("elapsed", stats.Elapsed), slog.F("transitions", stats.Transitions)) } } - }() + }) } func (e *Executor) runOnce(t time.Time) Stats { diff --git a/coderd/coderd.go b/coderd/coderd.go index 26bf4a7bf9b63..9b0e35b21032b 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -852,6 +852,7 @@ func New(options *Options) *API { r.Use( httpmw.Recover(api.Logger), + httpmw.WithProfilingLabels, tracing.StatusWriterMiddleware, tracing.Middleware(api.TracerProvider), httpmw.AttachRequestID, diff --git a/coderd/httpmw/pprof.go b/coderd/httpmw/pprof.go new file mode 100644 index 0000000000000..3d9ee38ce95e5 --- /dev/null +++ b/coderd/httpmw/pprof.go @@ -0,0 +1,22 @@ +package httpmw + +import ( + "context" + "net/http" + "runtime/pprof" + + "github.com/coder/coder/v2/coderd/pproflabel" +) + +// WithProfilingLabels adds a pprof label to all http request handlers. This is +// primarily used to determine if load is coming from background jobs, or from +// http traffic. +func WithProfilingLabels(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ctx := r.Context() + pprof.Do(ctx, pproflabel.Service(pproflabel.ServiceHTTPServer), func(ctx context.Context) { + r = r.WithContext(ctx) + next.ServeHTTP(rw, r) + }) + }) +} diff --git a/coderd/pproflabel/pproflabel.go b/coderd/pproflabel/pproflabel.go new file mode 100644 index 0000000000000..1ec58f7489d1c --- /dev/null +++ b/coderd/pproflabel/pproflabel.go @@ -0,0 +1,22 @@ +package pproflabel + +import ( + "context" + "runtime/pprof" +) + +// Go is just a convince wrapper to set off a labeled goroutine. +func Go(ctx context.Context, labels pprof.LabelSet, f func(context.Context)) { + go pprof.Do(ctx, labels, f) +} + +const ( + ServiceTerraformProvisioner = "terraform-provisioner" + ServiceMetricCollector = "metric-collector" + ServiceLifecycles = "lifecycles" + ServiceHTTPServer = "http-server" +) + +func Service(name string) pprof.LabelSet { + return pprof.Labels("service", name) +} diff --git a/coderd/prometheusmetrics/insights/metricscollector.go b/coderd/prometheusmetrics/insights/metricscollector.go index 41d3a0220f391..a095968526ca8 100644 --- a/coderd/prometheusmetrics/insights/metricscollector.go +++ b/coderd/prometheusmetrics/insights/metricscollector.go @@ -14,6 +14,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/pproflabel" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) @@ -158,7 +159,7 @@ func (mc *MetricsCollector) Run(ctx context.Context) (func(), error) { }) } - go func() { + pproflabel.Go(ctx, pproflabel.Service(pproflabel.ServiceMetricCollector), func(ctx context.Context) { defer close(done) defer ticker.Stop() for { @@ -170,7 +171,7 @@ func (mc *MetricsCollector) Run(ctx context.Context) (func(), error) { doTick() } } - }() + }) return func() { closeFunc() <-done diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index 69241d8aa1c17..c2ac1baf2db4e 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -333,6 +333,7 @@ func New(ctx context.Context, opts *Options) (*Server, error) { r.Use( // TODO: @emyrk Should we standardize these in some other package? httpmw.Recover(s.Logger), + httpmw.WithProfilingLabels, tracing.StatusWriterMiddleware, tracing.Middleware(s.TracerProvider), httpmw.AttachRequestID, From c92903c47375d8e450fc9620cb92dbe9827a5937 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Aug 2025 10:22:23 -0500 Subject: [PATCH 2/4] add prebuild reconciler --- coderd/pproflabel/pproflabel.go | 1 + enterprise/coderd/coderd.go | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/coderd/pproflabel/pproflabel.go b/coderd/pproflabel/pproflabel.go index 1ec58f7489d1c..c4aa61de27025 100644 --- a/coderd/pproflabel/pproflabel.go +++ b/coderd/pproflabel/pproflabel.go @@ -15,6 +15,7 @@ const ( ServiceMetricCollector = "metric-collector" ServiceLifecycles = "lifecycles" ServiceHTTPServer = "http-server" + ServicePrebuildReconciler = "prebuild-reconciler" ) func Service(name string) pprof.LabelSet { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 9583e14cd7fd3..211ebd16c86ff 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -12,20 +12,20 @@ import ( "sync" "time" - "github.com/coder/quartz" - "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/appearance" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/entitlements" "github.com/coder/coder/v2/coderd/idpsync" agplportsharing "github.com/coder/coder/v2/coderd/portsharing" + "github.com/coder/coder/v2/coderd/pproflabel" agplprebuilds "github.com/coder/coder/v2/coderd/prebuilds" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/wsbuilder" "github.com/coder/coder/v2/enterprise/coderd/connectionlog" "github.com/coder/coder/v2/enterprise/coderd/enidpsync" "github.com/coder/coder/v2/enterprise/coderd/portsharing" + "github.com/coder/quartz" "golang.org/x/xerrors" "tailscale.com/tailcfg" @@ -903,7 +903,8 @@ func (api *API) updateEntitlements(ctx context.Context) error { } api.AGPL.PrebuildsReconciler.Store(&reconciler) - go reconciler.Run(context.Background()) + // TODO: Should this context be the app context? + pproflabel.Go(context.Background(), pproflabel.Service(pproflabel.ServicePrebuildReconciler), reconciler.Run) api.AGPL.PrebuildsClaimer.Store(&claimer) } From cb1ec21f6155a566794766b151de4e39183d6822 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Aug 2025 10:34:44 -0500 Subject: [PATCH 3/4] linting --- coderd/autobuild/lifecycle_executor.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/autobuild/lifecycle_executor.go b/coderd/autobuild/lifecycle_executor.go index a8ddc61104b91..16072e6517125 100644 --- a/coderd/autobuild/lifecycle_executor.go +++ b/coderd/autobuild/lifecycle_executor.go @@ -111,7 +111,7 @@ func (e *Executor) Run() { pproflabel.Go(e.ctx, pproflabel.Service(pproflabel.ServiceLifecycles), func(ctx context.Context) { for { select { - case <-e.ctx.Done(): + case <-ctx.Done(): return case t, ok := <-e.tick: if !ok { @@ -121,12 +121,12 @@ func (e *Executor) Run() { e.metrics.autobuildExecutionDuration.Observe(stats.Elapsed.Seconds()) if e.statsCh != nil { select { - case <-e.ctx.Done(): + case <-ctx.Done(): return case e.statsCh <- stats: } } - e.log.Debug(e.ctx, "run stats", slog.F("elapsed", stats.Elapsed), slog.F("transitions", stats.Transitions)) + e.log.Debug(ctx, "run stats", slog.F("elapsed", stats.Elapsed), slog.F("transitions", stats.Transitions)) } } }) From 56f67a7cc7202e53ef080004ed1b27dc4b072afb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Aug 2025 11:11:00 -0500 Subject: [PATCH 4/4] add label for websocket vs http --- coderd/httpmw/pprof.go | 10 +++++++++- coderd/pproflabel/pproflabel.go | 14 ++++++++------ enterprise/coderd/coderd.go | 3 ++- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/coderd/httpmw/pprof.go b/coderd/httpmw/pprof.go index 3d9ee38ce95e5..eee3e9c9fdbe1 100644 --- a/coderd/httpmw/pprof.go +++ b/coderd/httpmw/pprof.go @@ -14,7 +14,15 @@ import ( func WithProfilingLabels(next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - pprof.Do(ctx, pproflabel.Service(pproflabel.ServiceHTTPServer), func(ctx context.Context) { + + // Label to differentiate between http and websocket requests. Websocket requests + // are assumed to be long-lived and more resource consuming. + requestType := "http" + if r.Header.Get("Upgrade") == "websocket" { + requestType = "websocket" + } + + pprof.Do(ctx, pproflabel.Service(pproflabel.ServiceHTTPServer, "request_type", requestType), func(ctx context.Context) { r = r.WithContext(ctx) next.ServeHTTP(rw, r) }) diff --git a/coderd/pproflabel/pproflabel.go b/coderd/pproflabel/pproflabel.go index c4aa61de27025..cd803b0f1baea 100644 --- a/coderd/pproflabel/pproflabel.go +++ b/coderd/pproflabel/pproflabel.go @@ -11,13 +11,15 @@ func Go(ctx context.Context, labels pprof.LabelSet, f func(context.Context)) { } const ( + ServiceTag = "service" + + ServiceHTTPServer = "http-api" + ServiceLifecycles = "lifecycle-executor" + ServiceMetricCollector = "metrics-collector" + ServicePrebuildReconciler = "prebuilds-reconciler" ServiceTerraformProvisioner = "terraform-provisioner" - ServiceMetricCollector = "metric-collector" - ServiceLifecycles = "lifecycles" - ServiceHTTPServer = "http-server" - ServicePrebuildReconciler = "prebuild-reconciler" ) -func Service(name string) pprof.LabelSet { - return pprof.Labels("service", name) +func Service(name string, pairs ...string) pprof.LabelSet { + return pprof.Labels(append([]string{ServiceTag, name}, pairs...)...) } diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 211ebd16c86ff..40569ead70658 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -903,7 +903,8 @@ func (api *API) updateEntitlements(ctx context.Context) error { } api.AGPL.PrebuildsReconciler.Store(&reconciler) - // TODO: Should this context be the app context? + // TODO: Should this context be the api.ctx context? To cancel when + // the API (and entire app) is closed via shutdown? pproflabel.Go(context.Background(), pproflabel.Service(pproflabel.ServicePrebuildReconciler), reconciler.Run) api.AGPL.PrebuildsClaimer.Store(&claimer)