Skip to content

feat(profiling): Go 1.26 + pprof, runtime metrics and continuous profiler on all Go deployables #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anuragthakur16 wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

feat(profiling): Go 1.26 + pprof, runtime metrics and continuous profiler on all Go deployables #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e870153
feat(profiling): Go 1.26 + pprof, runtime metrics and continuous prof…
anuragthakur16 Aug 10, 2026
ede5de0
fix(profiling): parity with the fleet reference, and unblock CI on Go…
anuragthakur16 Aug 12, 2026
59bb575
ci: raise the Go toolchain to 1.26 for the five modules this PR touches
anuragthakur16 Aug 12, 2026
ee75967
fix(onfs): drop the deprecated golang.org/x/net/context import
anuragthakur16 Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/horizon.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.24'
go-version: '1.26'
cache: false

- name: Cache Go modules
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/inferflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.24'
go-version: '1.26'
cache: false

- name: Cache Go modules
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/interaction-store.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.24'
go-version: '1.26'
cache: false

- name: Cache Go modules
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/online-feature-store.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.24'
go-version: '1.26'
cache: false

- name: Cache Go modules
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/skye.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: '1.24'
go-version: '1.26'
cache: false

- name: Cache Go modules
Expand Down
4 changes: 4 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ repos:
entry: "pre-commit-scripts/runner.sh"
language: script
stages: ["pre-commit", "pre-push"]
always_run: true
pass_filenames: false
- repo: local
hooks:
- id: post-commit
Expand All @@ -16,3 +18,5 @@ repos:
language: script
stages: ["post-commit"]
always_run: true
pass_filenames: false
verbose: true
10 changes: 10 additions & 0 deletions helm-charts/horizon/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8082
name: http
protocol: TCP
- containerPort: 14271

@m-agentic-review m-agentic-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ WARNING — Expose the new metrics port through the chart's Service as well as the container port, or add the repo's expected scrape configuration.

Declaring only a containerPort does not make :14271 reachable via the Kubernetes Service, so service-based Prometheus scraping or operators using ServiceMonitor will not discover the new runtime metrics endpoint.

Also flagged on this line:

  • ⚠️ WARNING — Do not expose the pprof listener on the metrics port without authentication or strict NetworkPolicy restrictions.

name: metrics
protocol: TCP
probes:
liveness:
path: /health
Expand Down Expand Up @@ -245,6 +248,13 @@ deployment:
value: "true"
- name: SKYE_TRIGGER_URL
value: "http://skye-trigger:8080"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""

@m-agentic-review m-agentic-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ WARNING — The chart hard-codes CICD_VERSION_ID to an empty value, which makes the continuous profiler skip itself in a default deployment.

The newly added profiler code treats an empty CICD_VERSION_ID as an error. Rendering this chart as-is injects an empty environment variable, so the new continuous profiler path logs/fails instead of starting unless another deployment step rewrites this value.

Same issue also flagged at:

  • helm-charts/inferflow/values.yaml:148
  • helm-charts/onfs-api-server/values.yaml:179
  • helm-charts/onfs-consumer/values.yaml:178
  • helm-charts/skye-admin/values.yaml:126
  • helm-charts/skye-consumers/values.yaml:211
  • … and 1 more location.

volumes:
- name: configs
configMap:
Expand Down
10 changes: 10 additions & 0 deletions helm-charts/inferflow/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8085
name: http
protocol: TCP
- containerPort: 14271
name: metrics
protocol: TCP
probes:
liveness:
path: /health/self
Expand Down Expand Up @@ -136,6 +139,13 @@ deployment:
value: "broker:29092"
- name: KAFKA_LOGGING_TOPIC
value: "inferflow_inference_logs"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""
serviceAccount:
enabled: false
annotations: {}
Expand Down
10 changes: 10 additions & 0 deletions helm-charts/onfs-api-server/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8089
name: grpc
protocol: TCP
- containerPort: 14271
name: metrics
protocol: TCP
probes:
liveness:
path: /health/self
Expand Down Expand Up @@ -167,6 +170,13 @@ deployment:
value: "8088"
- name: P2P_CACHE_ACTIVE_CONFIG_IDS
value: "5"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""
serviceAccount:
enabled: false
annotations: {}
Expand Down
14 changes: 12 additions & 2 deletions helm-charts/onfs-consumer/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8090
name: http
protocol: TCP
- containerPort: 14271
name: metrics
protocol: TCP
probes:
liveness:
path: /health/self
Expand Down Expand Up @@ -73,7 +76,7 @@ deployment:
- name: APP_METRIC_SAMPLING_RATE
value: "1"
- name: APP_NAME
value: "onfs"
value: "onfs-consumer"

@m-agentic-review m-agentic-review Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

- name: APP_PORT
value: "8090"
# Kafka consumer
Expand Down Expand Up @@ -118,7 +121,7 @@ deployment:
- name: STORAGE_SCYLLA_1_CONTACT_POINTS
value: "scylla"
- name: STORAGE_SCYLLA_1_KEYSPACE
value: "onfs"
value: "onfs-consumer"

@m-agentic-review m-agentic-review Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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.

- name: STORAGE_SCYLLA_1_PORT
value: "9042"
- name: STORAGE_SCYLLA_1_NUM_CONNS
Expand Down Expand Up @@ -166,6 +169,13 @@ deployment:
value: "30"
- name: STORAGE_REDIS_STANDALONE_ACTIVE_CONFIG_IDS
value: "2"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""
serviceAccount:
enabled: false
annotations: {}
Expand Down
10 changes: 10 additions & 0 deletions helm-charts/skye-admin/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8092
name: http
protocol: TCP
- containerPort: 14271
name: metrics
protocol: TCP
probes:
liveness:
path: /health
Expand Down Expand Up @@ -115,6 +118,13 @@ deployment:
value: "10"
- name: KAFKA_1_POLL_TIMEOUT
value: "1000"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""
serviceAccount:
enabled: false
annotations: {}
Expand Down
10 changes: 10 additions & 0 deletions helm-charts/skye-consumers/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8093
name: http
protocol: TCP
- containerPort: 14271
name: metrics
protocol: TCP
probes:
liveness:
path: /health
Expand Down Expand Up @@ -199,6 +202,13 @@ deployment:
value: "10"
- name: KAFKA_7_POLL_TIMEOUT
value: "1000"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""
serviceAccount:
enabled: false
annotations: {}
Expand Down
10 changes: 10 additions & 0 deletions helm-charts/skye-serving/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ deployment:
- containerPort: 8094
name: http
protocol: TCP
- containerPort: 14271
name: metrics
protocol: TCP
probes:
liveness:
path: /health/self
Expand Down Expand Up @@ -100,6 +103,13 @@ deployment:
# Auth
- name: AUTH_TOKENS
value: "test"
# Profiling. metric.Init binds the Prometheus registry + pprof on :14271
# (the `metrics` containerPort above). CICD_VERSION_ID tags captured profiles
# with the running build and is injected by CI/CD; when it is empty the
# continuous profiler is skipped with a warning and pprof + Go runtime
# metrics still work.
- name: CICD_VERSION_ID
value: ""
serviceAccount:
enabled: false
annotations: {}
Expand Down
9 changes: 9 additions & 0 deletions horizon/cmd/horizon/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ import (
"github.com/Meesho/BharatMLStack/horizon/pkg/infra"
"github.com/Meesho/BharatMLStack/horizon/pkg/logger"
"github.com/Meesho/BharatMLStack/horizon/pkg/metric"
"github.com/Meesho/BharatMLStack/horizon/pkg/profiling"
"github.com/Meesho/BharatMLStack/horizon/pkg/scheduler"
"github.com/rs/zerolog/log"
)

type AppConfig struct {
Expand Down Expand Up @@ -63,6 +65,13 @@ func main() {
horizonConfig.InitAll(appConfig.Configs)
logger.Init(appConfig.Configs)
metric.Init(appConfig.Configs)

@m-agentic-review m-agentic-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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

if err := profiling.InitWithOptions(
profiling.WithPprof(profiling.PprofAll...),

@m-agentic-review m-agentic-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ WARNING — Do not enable all pprof endpoints in production without authentication or strict network access controls.

Go pprof endpoints can expose heap contents, goroutine stacks, command-line arguments, and operational details that may include secrets or sensitive data. This initialization enables all pprof handlers with no visible authentication or authorization guard.

Same issue also flagged at:

  • inferflow/cmd/inferflow/main.go:34
  • online-feature-store/cmd/api-server/main.go:37
  • online-feature-store/cmd/consumer/main.go:34
  • skye/cmd/serving/main.go:34

profiling.WithRuntimeMetrics(profiling.RuntimeMetricAll),
profiling.WithContinuousProfiler(),

@m-agentic-review m-agentic-review Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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.

); err != nil {
log.Error().Err(err).Msg("profiling init failed")
}
httpframework.Init(middleware.NewMiddleware().GetMiddleWares()...)
workflowHandler.InitV1WorkflowHandler()
deployableRouter.Init(appConfig.Configs)
Expand Down
Loading
Loading