feat(profiling): Go 1.26 + pprof, runtime metrics and continuous profiler on all Go deployables by anuragthakur16 · Pull Request #384 · Meesho/BharatMLStack

anuragthakur16 · 2026-08-11T05:39:22Z

Context:

The Go modules here expose application metrics via the telegraf/statsd path, but nothing exports Go runtime metrics (go_*) or serves pprof, so there is no way to answer "where is this service's CPU going?" without redeploying with ad-hoc instrumentation.

This adds that surface to every Go deployable, built from public dependencies only — prometheus/client_golang and cloud.google.com/go/profiler. No internal library dependency is introduced, so the stack keeps its independence while emitting the series the standard Go runtime dashboards select on.

Describe your changes:

Per module, two small packages:

pkg/metric/server.go (pkg/metrics/server.go in inferflow) — a Prometheus registry and http.ServeMux served on :14271, with Registerer(), Mux() and Addr() accessors. Returns an error rather than panicking when the port is already bound.
pkg/profiling — InitWithOptions with WithPprof, WithRuntimeMetrics and WithContinuousProfiler. It reads metric.Registerer()/metric.Mux(), so metric.Init() must run first; it returns an error rather than half-initialising if they are nil.

Wired into all 9 entrypoints across the 5 Go modules:

Module	Entrypoints
`skye`	serving, consumers, admin
`online-feature-store`	api-server, consumer
`interaction-store`	server, consumer
`horizon`	horizon
`inferflow`	inferflow

Two things worth calling out for reviewers:

interaction-store was already calling the deprecated profiling.Init(), which serves pprof on a separate PROFILING_PORT and exports no runtime metrics — so it appeared instrumented while producing none of the go_* series. Both mains now use InitWithOptions.
inferflow needed care its siblings did not. Its metric package is pkg/metrics (plural), so a parameter named metrics shadowed the import and metrics.Registerer() resolved to a []RuntimeMetric rather than the package. Parameters renamed, with a comment — the resulting compile error is misleading rather than obvious.

go directive moved to 1.26.0 in all five modules.

Helm charts: every Go service gains a metrics containerPort 14271 and an explicit empty CICD_VERSION_ID. None set it previously, so the continuous profiler would have degraded to a warning on every service while appearing configured. Empty is the honest default — CI/CD injects the real value, and pprof plus runtime metrics work without it.

Deliberately not included: a pure-gofmt reformatting of eight unrelated horizon files (argocd, gcp, templates, github) that an earlier pass had picked up. A profiling change should not reformat unrelated code.

Testing:

Automated. skye/pkg/profiling/profiling_integration_test.go starts the metrics server and asserts against the live endpoint. It checks the wire format rather than our own helpers, because a dashboard goes blank silently if a metric name or label drifts:

go_gc_heap_allocs_bytes_total{env="stg",service="skye-test"}  1.0557264e+07
go_sched_latencies_seconds_count{env="stg",service="skye-test"}  43

Same series names and the same env/service label pair the existing Go runtime dashboards already select on.

Per module — all five clean on Go 1.26:

skye                   build OK · vet OK · tests OK
online-feature-store   build OK · vet OK · tests OK
interaction-store      build OK · vet OK · tests OK
horizon                build OK · vet OK · tests OK
inferflow              build OK · vet OK · tests OK

gofmt clean across every touched file; all seven modified values.yaml re-parsed as valid YAML with both changes present.

Monitoring:

Existing Go runtime dashboards work unchanged once :14271 is scraped — that is the point of asserting on the wire format above. pprof is available on the same port for on-demand CPU/heap/trace capture. Continuous profiles are tagged by service and build version via APP_NAME and CICD_VERSION_ID.

One open item for a reviewer who knows the platform: how :14271 gets scraped is not visible in this repo. The container port is now declared, but no scrape annotation or ServiceMonitor convention exists in these charts today, so I have not invented one — a chart that looks configured but is not scraped would be worse than an obviously incomplete one. Happy to add whatever the platform expects.

Also: interaction-store has no helm chart, so its two wired entrypoints have nowhere to declare the port yet.

Rollback plan

Revert the commit. The change is additive and inert by construction:

metric.Init() returning an error on a bound port means a port clash degrades rather than crashes.
InitWithOptions returns an error that every call site logs and continues past — no entrypoint treats profiling failure as fatal.
A missing CICD_VERSION_ID skips only the continuous profiler; pprof and runtime metrics are unaffected.

No request path, serialization format or existing metric is touched.

Checklist before requesting a review

I have reviewed my own changes?
Relevant or critical functionality is covered by tests?
Monitoring needs have been evaluated?
Any necessary documentation updates have been considered?

📂 Modules Affected

horizon (Real-time systems / networking)
online-feature-store (Feature serving infra)
trufflebox-ui (Admin panel / UI)
infra (Docker, CI/CD, GCP/AWS setup)
docs (Documentation updates)
Other: skye, interaction-store, inferflow

✅ Type of Change

📊 Benchmark / Metrics (if applicable)

This change adds the means to measure rather than a measurement itself, so there is no throughput or latency delta to report.

Overhead, for reference: the Prometheus Go collector is a scrape-time read of runtime/metrics. The continuous profiler samples for ~10s every ~10min, which is well under 1% average CPU. pprof handlers cost nothing until requested — with the exception of /debug/pprof/trace, which is deliberately on-demand only.

🤖 Generated with Claude Code

…iler on all deployables Ports the go-core profiling surface into BharatMLStack without taking a go-core dependency — the metric and profiling packages are built from public deps only (prometheus/client_golang, cloud.google.com/go/profiler), so the stack keeps its independence while emitting exactly what the platform's Go dashboards select on. Verified as byte-compatible, not assumed: the skye integration test asserts against a live endpoint and gets `go_gc_heap_allocs_bytes_total{env="stg",service="skye-test"}` on :14271 — the same series name and the same env/service label pair go-core produces. Those dashboards go blank silently if either drifts, so the assertion is on the wire format rather than on our own helpers. Per module: a Prometheus registry + mux served on :14271 (metric/server.go, returning an error rather than panicking when the port is taken), and a profiling package offering WithPprof / WithRuntimeMetrics / WithContinuousProfiler through InitWithOptions. All 9 entrypoints across the 5 Go modules are wired: skye serving, consumers, admin online-feature-store api-server, consumer interaction-store server, consumer horizon horizon inferflow inferflow interaction-store was already calling the deprecated profiling.Init(), which serves pprof on a separate PROFILING_PORT and exports NO runtime metrics — so it looked wired while producing none of the go_* series the dashboards need. Both its mains now use InitWithOptions. inferflow needed care its siblings did not: its metric package is `pkg/metrics` (plural), so a parameter named `metrics` shadowed the import and `metrics.Registerer()` resolved to a []RuntimeMetric instead of the package. Renamed the parameters and left a comment, because the failure mode is a confusing type error rather than an obvious one. Charts: every Go service gains a `metrics` containerPort 14271 and an explicit empty CICD_VERSION_ID. None of them set it before, so the continuous profiler would have degraded to a warning on every service while looking configured. Empty is the honest default — CI/CD injects the real value, and pprof plus runtime metrics work without it. go directive moved to 1.26.0 in all five modules. NOT included: the pure-gofmt reformatting of eight unrelated horizon files (argocd, gcp, templates, github) that an earlier pass had picked up. A profiling change has no business reformatting other people's code. Still open: interaction-store has no helm chart, so its two wired entrypoints have nowhere to declare the port; and how :14271 is actually scraped is a platform concern not visible in this repo — no go-core service declares it in config.yaml either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

meesho-drs · 2026-08-11T05:40:59Z

🔴 NON_LOW — needs a normal review · confidence 0.92

This ships a Go 1.26 upgrade, new third-party libraries, and unauthenticated pprof on port 14271 across every Go service — any of those can change runtime behavior or leak internals. The diff also retargets the onfs-consumer Scylla keyspace from onfs to onfs-consumer in helm-charts/onfs-consumer/values.yaml, which is unrelated profiling work. Cleared: no change to customer-facing API contracts.

Top risks

skye/pkg/metric/server.go — binds :14271 with /debug/pprof/* and /metrics on a plain http.Server with no auth middleware
horizon/go.mod (and 4 sibling go.mod files) — Go 1.24→1.26 plus new prometheus/client_golang and cloud.google.com/go/profiler with many transitive bumps
helm-charts/onfs-consumer/values.yaml — STORAGE_SCYLLA_1_KEYSPACE and APP_NAME changed from onfs to onfs-consumer, altering which keyspace the consumer uses
horizon/pkg/profiling/continuous_profiler.go — GCP continuous profiler starts on every entrypoint at boot with no rollout flag

Check before approving

Confirm whether onfs-consumer Scylla keyspace rename is intentional for production or only quick-start values
Verify network policy restricts :14271 pprof/metrics to internal scrapers only
Review Go 1.26 + grpc/protobuf transitive upgrades in staging before prod
Wait for pending CI analyze/build jobs to finish green

5 tool(s) ran · DRS agentic v2 · #384

m-agentic-review

🤖 Code Review — PR #384

Language: Go | Files reviewed: 62

Severity		Count
SUGGESTION	💡	22
WARNING	⚠️	98

m-agentic-review · 2026-08-11T05:48:51Z

+	go func() {
+		addr := fmt.Sprintf(":%d", port)
+		log.Info().Msgf("Starting profiling server on %v", addr)
+		if err := http.ListenAndServe(addr, nil); err != nil {


⚠️ WARNING — Do not run the pprof server with http.ListenAndServe(addr, nil) on :port; bind to localhost or use an authenticated mux.

Using nil serves http.DefaultServeMux, where net/http/pprof registers unauthenticated /debug/pprof endpoints, and addr is constructed as :port so it listens on all interfaces. This can expose sensitive runtime profiles and enable resource exhaustion when profiling is enabled.

Same issue also flagged at:

inferflow/pkg/profiling/profiling.go:106

interaction-store/cmd/consumer/main.go:45

interaction-store/cmd/server/main.go:40

online-feature-store/cmd/api-server/main.go:36

online-feature-store/cmd/consumer/main.go:33

… and 1 more location.

m-agentic-review · 2026-08-11T05:48:51Z

+
+	// pprof must be on the same port as /metrics, not a second one.
+	for _, path := range []string{"/debug/pprof/heap", "/debug/pprof/goroutine", "/debug/pprof/cmdline"} {
+		resp, err := http.Get(base + path)


⚠️ WARNING — Use an http.Client with a timeout or a request context for the pprof GETs instead of http.Get.

http.Get uses the default client without a timeout, so the test can hang indefinitely if the metrics server accepts a connection but does not respond. Bounded requests make failures deterministic in CI.

Same issue also flagged at:

skye/pkg/profiling/profiling_integration_test.go:80

m-agentic-review · 2026-08-11T05:48:51Z

@@ -63,6 +65,13 @@ func main() {
 	horizonConfig.InitAll(appConfig.Configs)
 	logger.Init(appConfig.Configs)
 	metric.Init(appConfig.Configs)


⚠️ WARNING — Handle the error returned by metric.Init before calling profiling.InitWithOptions.

The PR describes metric.Init as returning an error when the metrics port cannot be bound, but this call discards that error. If the metrics server fails to start, profiling initialization will only fail indirectly and the service will silently lose the new runtime metrics surface.

Same issue also flagged at:

inferflow/cmd/inferflow/main.go:32

online-feature-store/cmd/api-server/main.go:35

online-feature-store/cmd/consumer/main.go:32

m-agentic-review · 2026-08-11T05:48:51Z

+
+		// Prometheus endpoint for runtime metrics and pprof (see pkg/profiling).
+		// Additive to StatsD: a failure here is logged, not fatal.
+		if err := initMetricsServer(defaultMetricsServerPort, config.AppName, config.AppEnv); err != nil {


⚠️ WARNING — Thread a context.Context into the metrics-server startup instead of calling initMetricsServer without one from Init.

The added path starts a Prometheus/pprof HTTP listener, which is I/O and should be context-controlled with context.Context as the first argument. Without it there is no standard way to cancel startup or shut the server down gracefully.

Also flagged on this line:

⚠️ WARNING — Ensure initMetricsServer exposes pprof only on a private, authenticated listener instead of a broadly reachable metrics port.

m-agentic-review · 2026-08-11T05:48:51Z

+	for _, h := range handlers {
+		switch h {
+		case PprofHeap:
+			mux.Handle("/debug/pprof/heap", pprof.Handler("heap"))


⚠️ WARNING — Deduplicate selected pprof handlers or return an error instead of using ServeMux.Handle directly on possibly duplicate patterns.

http.ServeMux.Handle panics when the same pattern is registered more than once. Passing duplicate handlers, or combining WithPprof() with an explicit handler, can crash the process during profiling initialization instead of returning an error as this feature promises.

Also flagged on this line:

⚠️ WARNING — Wrap the mounted pprof handlers with authentication or restrict them to a trusted listener before exposing them on the metrics server.

m-agentic-review · 2026-08-11T05:48:52Z

+		return fmt.Errorf("APP_NAME is required for continuous profiler")
+	}
+	version := viper.GetString("CICD_VERSION_ID")
+	if version == "" {


💡 SUGGESTION — The continuous profiler is disabled whenever CICD_VERSION_ID is absent or empty, including the explicit empty Helm default described for new deployments.

This function treats an empty version as a hard initialization error before calling profiler.Start. If a production deployment uses the chart default or a CI path that does not inject CICD_VERSION_ID, the service will appear configured for continuous profiling but will never start the profiler.

Same issue also flagged at:

interaction-store/pkg/profiling/continuous_profiler.go:20

m-agentic-review · 2026-08-11T05:48:52Z

+	mux := metric.Mux()
+	for _, h := range handlers {
+		switch h {
+		case PprofHeap:


💡 SUGGESTION — The pprof index endpoint /debug/pprof/ is never mounted, so standard pprof discovery and browser navigation return 404.

The standard net/http/pprof registration includes the index handler at /debug/pprof/ in addition to named profiles. This code only registers individual profile paths, so users and tools expecting the canonical index cannot discover available profiles on the advertised pprof surface.

Same issue also flagged at:

horizon/pkg/profiling/pprof.go:17

inferflow/pkg/profiling/pprof.go:16

inferflow/pkg/profiling/pprof.go:17

interaction-store/pkg/profiling/pprof.go:12

online-feature-store/pkg/profiling/pprof.go:12

… and 1 more location.

m-agentic-review · 2026-08-11T05:48:52Z

+	if err := profiling.InitWithOptions(
+		profiling.WithPprof(profiling.PprofAll...),
+		profiling.WithRuntimeMetrics(profiling.RuntimeMetricAll),
+		profiling.WithContinuousProfiler(),


💡 SUGGESTION — Consider gating WithContinuousProfiler() for horizon instead of enabling background profiling on every instance.

Continuous profiling adds periodic background sampling and upload work to the process. Horizon may be less CPU-critical than serving paths, but enabling it on every replica still creates recurring fleet CPU/network cost; a runtime flag or sampled deployment gives the same observability with lower steady-state overhead.

Same issue also flagged at:

horizon/pkg/profiling/continuous_profiler.go:23

inferflow/cmd/inferflow/main.go:36

interaction-store/cmd/consumer/main.go:47

interaction-store/cmd/server/main.go:42

online-feature-store/cmd/api-server/main.go:39

… and 5 more locations.

m-agentic-review · 2026-08-11T05:48:52Z

+
+// PprofAll is every handler above. Snapshot handlers are cheap; Profile and
+// Trace only cost while a request is in flight.
+var PprofAll = []PprofHandler{


💡 SUGGESTION — PprofAll is an exported mutable slice, so any package can accidentally alter the global handler set used by later initializers.

Because PprofAll is a package variable rather than an immutable value, callers can modify its elements or length through normal slice operations. Later calls using profiling.PprofAll... may mount the wrong endpoints or duplicate endpoints and panic in ServeMux.Handle.

Same issue also flagged at:

interaction-store/pkg/profiling/options.go:21

online-feature-store/pkg/profiling/options.go:21

skye/pkg/profiling/options.go:21

m-agentic-review · 2026-08-11T05:48:52Z

+)
+
+// RuntimeMetricAllSet is the full set, matching the go-runtime-metrics dashboard.
+var RuntimeMetricAllSet = []RuntimeMetric{RuntimeMetricAll}


💡 SUGGESTION — RuntimeMetricAllSet is an exported mutable slice, so callers can corrupt the default runtime metric selection globally.

Any importing package can overwrite elements in RuntimeMetricAllSet before another initializer uses it. If it is changed to an invalid RuntimeMetric, InitWithOptions will fail runtime metric registration and consume optsOnce, leaving metrics disabled until restart.

Same issue also flagged at:

interaction-store/pkg/profiling/options.go:37

online-feature-store/pkg/profiling/options.go:37

skye/pkg/profiling/options.go:37

… 1.26 Three classes of fix, from CI and from review on #384. CI was red for one reason that masked another. Every package failed `[setup failed]` on a missing go.sum entry for prometheus/procfs, which is Linux-only and therefore never compiled on the darwin machine this was built on. `go mod download` is not sufficient here -- Go requires procfs as an explicit indirect require on linux -- so the manifests are tidied. Behind that sat a real Go 1.26 regression. pkg/api/http.BuildHttpUrl formatted "http://%s:%d:%s", producing "http://host:8080:/path" -- an extra colon. Go's URL parser accepted that leniently before 1.26 and rejects it from 1.26 on, and BuildContentTypeJson never checked the error from NewRequestWithContext, so it dereferenced a nil *http.Request and panicked. Confirmed by changing ONLY the go directive on an untouched develop checkout: the panic reproduces there, so the upgrade surfaced a long-standing bug rather than introducing one. Both halves are fixed -- the format string and the unchecked error. Parity was checked against the actual fleet reference rather than assumed, and it cut both ways. Removed, because it is NOT what go-core does: a blank `_ "net/http/pprof"` import in every profiling.go. That registers all pprof handlers on DefaultServeMux, and both online-feature-store entrypoints run `http.ListenAndServe(":8080", nil)` -- so it silently published profile and trace on the application port. go-core imports the package normally and mounts handlers explicitly on the metrics mux; now so does this. Kept, because it IS what go-core does, despite review flagging both: the 3-minute WriteTimeout (go-core's own comment: "pprof CPU profiles run up to 120s; 3m gives buffer") and the InitWithOptions ordering, where pprof and the continuous profiler start before fallible runtime-metric registration. Diverging would make this stack behave differently from every other Go service. The doc comment overclaimed on that ordering and now describes what the code actually does. Continuous profiling now follows offer-platform-go rather than go-core v2: when CICD_VERSION_ID is absent, fall back to the VCS revision stamped into the binary instead of refusing to start. go-core requires the env var, which meant a default deployment of this stack -- with no Meesho pipeline injecting it -- got no continuous profiles at all while appearing configured. onfs-consumer's APP_NAME was "onfs", identical to onfs-api-server, so Cloud Profiler would have merged two different workloads into one service. Now "onfs-consumer". Excluded deliberately: gofmt-only reformatting of eight unrelated horizon files, and a pre-commit-config rewrite the hook installer made locally. pkg/grpc still fails locally, on this branch and on an untouched develop alike; develop's CI is green, so it is environment-specific and out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

turbo-turtle-github · 2026-08-12T22:29:53Z

CTXM-NO-SCAN-NOTICE

🔎 Context Maintainer hasn't scanned this branch yet.

Nothing broke — but this PR won't get a green Context Maintainer check until it gets scanned. Two ways to unblock:

Run /m-ctxm:fix-my-drift in Claude Code — foreground reconcile against HEAD, opens the drift-fix PR
Push another commit — auto-fires the post-commit hook (works now that this branch is on origin)

Stuck? Ping #ucl-support with this PR link.

CI failed staticcheck with "package requires newer Go version go1.26 (application built with go1.25)". The workflows pin setup-go to 1.24 and then `go install staticcheck@latest`, so staticcheck is built with the pinned toolchain and cannot analyse a module whose go directive is higher. Raising the pin is not a scope choice — the dependencies this PR exists to add require it: cloud.google.com/go/profiler v0.6.0 go 1.25.0 github.com/prometheus/client_golang v1.24.1 go 1.25.0 `go mod tidy` raises each module's go directive to 1.25.0 on its own to satisfy them. Verified by reverting the directive by hand: the modules still build, but staticcheck then reports "module requires at least go1.25.0", so the CI pin fails either way. Continuous profiling on Google Profiler and go_* runtime metrics are simply not reachable on Go 1.24. Since the pin has to move past 1.24 regardless, it goes to 1.26 — the version the fleet is standardising on, and the directive already in these modules. Scoped to the five workflows whose modules this PR changes. go-sdk.yml and release-go-sdk.yml keep 1.24: that module is untouched here and raising it would be an unrelated change to another team's build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

m-agentic-review

🤖 Code Review — PR #384

Language: Go | Files reviewed: 64

Severity		Count
BLOCKER	🚨	1
WARNING	⚠️	23

m-agentic-review · 2026-08-12T22:38:24Z

      value: "scylla"
    - name: STORAGE_SCYLLA_1_KEYSPACE
-      value: "onfs"
+      value: "onfs-consumer"


🚨 BLOCKER — Do not change STORAGE_SCYLLA_1_KEYSPACE from the existing onfs keyspace to onfs-consumer as part of profiling wiring.

This changes the datastore namespace the consumer connects to, not just profiling configuration. If the production Scylla schema/data lives in the existing onfs keyspace, the pod will either fail to start due to a missing keyspace/table or read/write an empty/wrong keyspace.

Also flagged on this line:

⚠️ WARNING — Revert the STORAGE_SCYLLA_1_KEYSPACE change unless the backing Scylla keyspace has actually been renamed.

m-agentic-review · 2026-08-12T22:38:24Z

+
+// Deprecated: serves pprof on its own PROFILING_PORT and exports no runtime
+// metrics. Use InitWithOptions.
 func Init() {


⚠️ WARNING — The deprecated Init path can no longer serve pprof because the side-effect net/http/pprof import was removed while Init still uses the default mux.

initProfilingTool still starts http.ListenAndServe with a nil handler, which relies on net/http/pprof registering handlers on http.DefaultServeMux. This file now imports only net/http, so any remaining caller of profiling.Init gets a live profiling port that returns 404 for /debug/pprof endpoints.

Same issue also flagged at:

skye/pkg/profiling/profiling.go:73

m-agentic-review · 2026-08-12T22:38:24Z

 	etcd.Init(configManagerVersion, &featureConfig.FeatureRegistry{})
 	metric.Init()
+	if err := profiling.InitWithOptions(
+		profiling.WithPprof(profiling.PprofAll...),


⚠️ WARNING — Remove the legacy net/http/pprof default-mux server or avoid enabling WithPprof here so pprof is not exposed on both :8080 and :14271.

This file still imports net/http/pprof and starts http.ListenAndServe(":8080", nil), so adding WithPprof leaves an unadvertised second pprof surface outside the new metrics port.

Same issue also flagged at:

online-feature-store/cmd/consumer/main.go:34

m-agentic-review · 2026-08-12T22:38:24Z

+	// mirrors offer-platform-go, whose cloud profiler derives its version the
+	// same way, and is why continuous profiling works here with no config.
+	version := viper.GetString("CICD_VERSION_ID")
+	if version == "" {


⚠️ WARNING — Skip or return when CICD_VERSION_ID is empty instead of falling back to buildVersion.

The PR description and Helm defaults rely on an empty CICD_VERSION_ID disabling only the continuous profiler; this fallback still starts Cloud Profiler with a VCS revision or "unknown", so deployments without CI injection behave differently than documented.

Same issue also flagged at:

inferflow/pkg/profiling/continuous_profiler.go:26

interaction-store/pkg/profiling/continuous_profiler.go:26

online-feature-store/pkg/profiling/continuous_profiler.go:26

skye/pkg/profiling/continuous_profiler.go:26

m-agentic-review · 2026-08-12T22:38:24Z

+	log.Info().Msg("Profiling environment initialized!")
+}
+
+func initProfilingTool(port int) {


⚠️ WARNING — Add a context.Context as the first parameter to initProfilingTool or replace it with a context-aware server startup API.

This helper starts an HTTP listener, which is network I/O, but it has no context parameter despite the repository rule requiring context.Context first for I/O functions. A context-aware API would also make shutdown and cancellation possible instead of launching an unmanaged goroutine.

Same issue also flagged at:

inferflow/pkg/profiling/profiling.go:105

online-feature-store/pkg/profiling/profiling.go:105

m-agentic-review · 2026-08-12T22:38:24Z

+// as a parse error, because the caller below did not check err.
 func BuildHttpUrl(host string, port int, path string) string {
-	return fmt.Sprintf("http://%s:%d:%s", host, port, path)
+	return fmt.Sprintf("http://%s:%d%s", host, port, path)


⚠️ WARNING — BuildHttpUrl now assumes every caller supplies a leading slash, so a path like "v1/foo" produces an invalid URL such as "http://host:8080v1/foo".

The helper only checks/receives an arbitrary path string and does not normalize or validate that it starts with '/'. Removing the separator makes the port and path run together whenever a caller passes a relative path without a leading slash, causing http.NewRequestWithContext to fail under the newly added error handling.

Also flagged on this line:

⚠️ WARNING — Validate or normalize that path starts with '/' before concatenating it into the URL.

m-agentic-review · 2026-08-12T22:38:25Z

+	// Prometheus endpoint for runtime metrics and pprof (see pkg/profiling).
+	// Started before the StatsD client so it comes up even when Telegraf is
+	// absent and the block below returns early.
+	if err := initMetricsServer(defaultMetricsServerPort, configs.Configs.ApplicationName, configs.Configs.ApplicationEnv); err != nil {


⚠️ WARNING — InitMetrics starts the fixed-port metrics server on every call, so reinitializing metrics can collide with the listener it already created.

Unlike the sibling metric packages shown here, this function is not guarded by sync.Once. If configuration reload or repeated initialization calls InitMetrics again in the same process, this added line tries to bind :14271 again and fails, and depending on initMetricsServer internals may also leave global mux/registerer state inconsistent with the running listener.

m-agentic-review · 2026-08-12T22:38:25Z

 	metric.Init()
 	etcd.InitFromAppName(&config.Skye{}, appConfig.Configs.AppName, appConfig.Configs)
-	profiling.Init()
+	if err := profiling.InitWithOptions(


⚠️ WARNING — Replacing profiling.Init() changes the pprof listener from the legacy profiling port to the new metrics port, breaking callers of the old endpoint.

This entrypoint used to invoke the package's legacy profiling initialization directly. The new options-based initialization depends on the metric server and serves pprof from that mux instead, so existing operational tooling that targets the old profiling endpoint will silently lose access.

m-agentic-review · 2026-08-12T22:38:25Z

      value: "1"
    - name: APP_NAME
-      value: "onfs"
+      value: "onfs-consumer"


⚠️ WARNING — Changing APP_NAME from onfs to onfs-consumer can alter the application's runtime identity beyond profiler labels.

APP_NAME is commonly consumed by application config, logging, metrics, tracing, service discovery, or external integrations, while this PR only needs profiler tagging. Renaming it in the deployment can make the consumer look up different config or emit under a different service identity than existing dashboards/alerts expect.

m-agentic-review · 2026-08-12T22:38:25Z

+	if len(opts) == 0 {
+		return fmt.Errorf("profiling: InitWithOptions requires at least one option")
+	}
+	optsOnce.Do(func() {


⚠️ WARNING — A first successful call with only a subset of options permanently suppresses later profiling options.

The sync.Once guards the entire option application, not each profiling feature. If any package initializes with only continuous profiling or only pprof, a later call that adds runtime metrics is ignored and returns the previous nil error, leaving the service silently missing the expected go_* series.

Same issue also flagged at:

inferflow/pkg/profiling/profiling.go:38

online-feature-store/pkg/profiling/profiling.go:38

staticcheck SA1019 on internal/data/repositories/stores/redis.go:16 — "golang.org/x/net/context" is deprecated: Use the standard library context package instead. Pre-existing, and previously invisible for two compounding reasons: on develop staticcheck ran against an older golang.org/x/net that did not carry the deprecation notice, and on this branch it could not analyse the module at all until the toolchain pin was raised, so it failed at compile before reaching any check. Raising the pin is what surfaced it. x/net/context has been a type alias for the standard library context since Go 1.7, so this is a drop-in swap with no behaviour change. It was the only occurrence across all five modules. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

m-agentic-review

🤖 Code Review — PR #384

Language: Go | Files reviewed: 69

Severity		Count
BLOCKER	🚨	1
WARNING	⚠️	6

m-agentic-review · 2026-08-12T22:45:21Z

-go 1.24.4
-
-toolchain go1.24.10
+go 1.26.0


🚨 BLOCKER — Raising the required Go version to 1.26.0 will make builds fail anywhere the deploy toolchain is still pinned to the previous Go 1.24 image.

The deleted toolchain line showed this module was previously built with Go 1.24.x, while this line now requires Go 1.26.0. If any CI, release, or emergency rollback builder uses GOTOOLCHAIN=local or cannot auto-download 1.26, the service cannot be rebuilt or deployed.

Also flagged on this line:

⚠️ WARNING — Changing the module to Go 1.26 enables the newer container-aware GOMAXPROCS defaults, which can silently reduce runtime parallelism in CPU-limited pods.

m-agentic-review · 2026-08-12T22:45:21Z

+// mountPprof attaches the selected pprof endpoints to the metrics server's mux,
+// so they are served on the same port as /metrics rather than a second port.
+func mountPprof(handlers []PprofHandler) {
+	mux := metric.Mux()


⚠️ WARNING — mountPprof dereferences the metrics mux without guarding against metric.Init failing or not having run.

metric.Mux() is assumed to return a non-nil *http.ServeMux, and the next registrations will panic if it is nil. The PR states metric.Init may fail non-fatally when the port is already bound, so any path that still invokes profiling after that failure turns a degraded metrics startup into a process crash.

Same issue also flagged at:

inferflow/pkg/profiling/pprof.go:13

interaction-store/pkg/profiling/pprof.go:13

online-feature-store/pkg/profiling/pprof.go:13

skye/pkg/profiling/pprof.go:13

m-agentic-review

🤖 Code Review — PR #384

Language: Go | Files reviewed: 70

Severity		Count
SUGGESTION	💡	4
WARNING	⚠️	8

m-agentic-review · 2026-08-12T22:48:50Z

+
+		// Prometheus endpoint for runtime metrics and pprof (see pkg/profiling).
+		// Additive to StatsD: a failure here is logged, not fatal.
+		if err := initMetricsServer(defaultMetricsServerPort, config.AppName, config.AppEnv); err != nil {


⚠️ WARNING — Binding the fixed metrics port from metric.Init can break horizon instances whose normal service port is 14271.

This added listener is not coordinated with the service's configured application listener. Because metric.Init runs during startup before the main server is brought up, a config that uses 14271 for the application will now fail to start the real server.

Same issue also flagged at:

inferflow/pkg/metrics/metrics.go:34

interaction-store/pkg/metric/metric.go:56

online-feature-store/pkg/metric/metric.go:56

skye/pkg/metric/metric.go:81

m-agentic-review · 2026-08-12T22:48:50Z

+
+	go func() {
+		log.Info().Str("addr", ln.Addr().String()).Msg("Starting metrics server")
+		if err := metricsServer.Serve(ln); err != nil && err != http.ErrServerClosed {


⚠️ WARNING — Capture the newly created http.Server in the goroutine instead of reading the package-level metricsServer variable.

The goroutine calls Serve through the mutable global, so a second init on another port can overwrite metricsServer before the first goroutine runs. The first listener would then be served by the wrong server/handler, and the original server object is leaked or never used.

Same issue also flagged at:

skye/pkg/metric/server.go:83

m-agentic-review · 2026-08-12T22:48:50Z

+		rules = append(rules, rule)
+	}
+	collector := collectors.NewGoCollector(collectors.WithGoCollectorRuntimeMetrics(rules...))
+	return metric.Registerer().Register(collector)


⚠️ WARNING — registerRuntimeMetrics calls metric.Registerer().Register without guarding against a nil registerer, so a failed or skipped metric.Init can still panic.

The PR states metric.Init may return an error rather than crashing when :14271 is already bound, but this helper assumes the global registerer is always initialized. If InitWithOptions reaches this function after metric.Init failed or was not called, the method call on a nil prometheus.Registerer will panic instead of returning the advertised initialization error.

m-agentic-review · 2026-08-12T22:48:50Z

+func WithPprof(handlers ...PprofHandler) Option {
+	return func(c *config) {
+		if len(handlers) == 0 {
+			handlers = []PprofHandler{


💡 SUGGESTION — WithPprof()'s default handler set omits cmdline and symbol even though the contract says it mounts everything except profile and trace.

The no-argument path includes heap, allocs, goroutine, threadcreate, block and mutex only. Cmdline and symbol are also non-blocking snapshot handlers, so callers using the documented default will get unexpected 404s for those pprof endpoints.

Same issue also flagged at:

inferflow/pkg/profiling/options.go:54

interaction-store/pkg/profiling/options.go:54

online-feature-store/pkg/profiling/options.go:54

m-agentic-review Bot reviewed Aug 11, 2026

View reviewed changes

m-agentic-review Bot reviewed Aug 12, 2026

View reviewed changes

Uh oh!

Conversation

anuragthakur16 commented Aug 11, 2026

Context:

Describe your changes:

Testing:

Monitoring:

Rollback plan

Checklist before requesting a review

📂 Modules Affected

✅ Type of Change

📊 Benchmark / Metrics (if applicable)

Uh oh!

meesho-drs Bot commented Aug 11, 2026 • edited by m-agentic-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔴 NON_LOW — needs a normal review · confidence 0.92

Uh oh!

m-agentic-review Bot left a comment

Choose a reason for hiding this comment

🤖 Code Review — PR #384

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

turbo-turtle-github Bot commented Aug 12, 2026

CTXM-NO-SCAN-NOTICE

Uh oh!

m-agentic-review Bot left a comment

Choose a reason for hiding this comment

🤖 Code Review — PR #384

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

m-agentic-review Bot Aug 12, 2026

meesho-drs Bot commented Aug 11, 2026 •

edited by m-agentic-review Bot

Loading