Skip to content

perf: Decouple metrics HTTP server into a dedicated child process - #6855

Open
ntkathole wants to merge 1 commit into
feast-dev:masterfrom
ntkathole:perf/decouple-metrics-server
Open

ntkathole wants to merge 1 commit into
feast-dev:masterfrom
ntkathole:perf/decouple-metrics-server

Conversation

@ntkathole

@ntkathole ntkathole commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

Summary

Replace the daemon thread that serves the Prometheus /metrics endpoint with a multiprocessing.Process so that scrape-time aggregation (MultiProcessCollector reads + text serialization) runs with its own GIL, fully isolated from request-serving workers and the Gunicorn master.

Problem

When Prometheus scrapes the /metrics endpoint, the current daemon thread performs:

  1. File listing of all .db files in PROMETHEUS_MULTIPROCESS_DIR
  2. Reading mmap-backed metric files from every Gunicorn worker
  3. Aggregating/merging metrics across workers
  4. Serializing to Prometheus text format

All of this runs under the same GIL as the Gunicorn master process. With high metric cardinality (many label combinations), this scrape overhead can cause GIL contention that indirectly affects request latency.

Solution

Move the WSGI HTTP metrics server into a dedicated child process via multiprocessing.Process(daemon=True):

  • _run_metrics_server() — new top-level function that serves as the child process entry point. It re-imports prometheus_client for macOS spawn safety and registers SIGTERM/SIGINT handlers for graceful shutdown.
  • start_metrics_server() — now spawns a Process instead of a Thread. The function signature and call sites are unchanged.
  • Background monitoring threads (resource, freshness) remain in-process because they only write to mmap-backed Gauges — an operation that is fast and does not benefit from process isolation.

Why this is safe

Aspect Before (thread) After (process)
Metrics port :8000 in master process :8000 in child process
GIL isolation Shared with master Fully isolated
daemon=True Thread auto-joins on exit Process auto-terminates on parent exit
Prometheus scrape Same metrics, same format Identical — MultiProcessCollector reads same mmap files
Kubernetes/HPA Transparent Transparent — same port, same pod network namespace
spawn safety N/A _run_metrics_server is top-level, re-imports deps

@ntkathole
ntkathole requested a review from a team as a code owner September 22, 2026 05:17
@ntkathole
ntkathole force-pushed the perf/decouple-metrics-server branch from a0c1437 to 755e967 Compare September 22, 2026 05:19
@codecov-commenter

codecov-commenter commented Sep 22, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.09%. Comparing base (dd9f099) to head (b4afb2d).

Files with missing lines Patch % Lines
sdk/python/feast/metrics.py 93.33% 1 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6855      +/-   ##
==========================================
+ Coverage   48.04%   48.09%   +0.05%     
==========================================
  Files         427      427              
  Lines       53591    53612      +21     
  Branches     7800     7803       +3     
==========================================
+ Hits        25749    25786      +37     
+ Misses      25986    25969      -17     
- Partials     1856     1857       +1     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 49.43% <93.33%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/metrics.py 85.25% <93.33%> (+9.74%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd9f099...b4afb2d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jyejare jyejare left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The PR cleanly isolates Prometheus scrape aggregation in a dedicated process and preserves the existing in-process monitoring behavior. The documentation and unit tests cover the process construction and monitoring flags well, but the child shutdown path can deadlock because BaseServer.shutdown() is called from the same thread running serve_forever(). Startup failures and lifecycle management are also not surfaced to the parent, which can leave the service reporting metrics as started when the endpoint is unavailable.

Comment thread sdk/python/feast/metrics.py Outdated
Comment thread sdk/python/feast/metrics.py
Comment thread sdk/python/feast/metrics.py Outdated
Comment thread sdk/python/feast/metrics.py
Comment thread sdk/python/tests/unit/test_metrics.py
@ntkathole
ntkathole force-pushed the perf/decouple-metrics-server branch 2 times, most recently from ca670d8 to b1dff9b Compare September 26, 2026 15:17
@ntkathole
ntkathole requested a review from jyejare September 26, 2026 15:17
@ntkathole
ntkathole force-pushed the perf/decouple-metrics-server branch 2 times, most recently from c3a6ca7 to 596d1ac Compare September 30, 2026 16:42
Signed-off-by: ntkathole <nikhilkathole2683@gmail.com>
@ntkathole
ntkathole force-pushed the perf/decouple-metrics-server branch from 596d1ac to b4afb2d Compare September 30, 2026 16:47

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants