Framework: Add Prometheus observability metrics - #241
Open
razor950 wants to merge 4 commits into
Open
Conversation
Vendor prometheus-cpp-lite behind a framework-owned registry and expose a configurable metrics endpoint. Instrument server ticks, HTTP routes, networking, replication, jobs, scripting, resources, and masterlist updates. Add registry and job-system metric coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThe framework adds configurable Prometheus and no-op metrics backends, instruments HTTP, server, job, networking, replication, and scripting paths, exposes protected metrics, and adds backend, exposition, and job-scheduling tests. ChangesMetrics observability
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/networking/network_server.cpp (1)
175-197: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Shutdown()doesn't clear_peerIdentities.
Shutdown()bulk-clears_acceptedClients,_authenticatedClients, and_readyClientsand resets the associated gauges, but_peerIdentities(populated viaSetPeerIdentity) is left untouched. Per-connection disconnects go throughClearClientState, which does erase the identity entry — this bulk path is the one exception, leaving stale identity data around after shutdown.🧹 Proposed fix
_acceptedClients.clear(); _authenticatedClients.clear(); _readyClients.clear(); + _peerIdentities.clear(); if (_authenticatedConnections) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/networking/network_server.cpp` around lines 175 - 197, Update NetworkServer::Shutdown() to clear _peerIdentities alongside _acceptedClients, _authenticatedClients, and _readyClients before destroying the peer, ensuring no stale identity entries remain after bulk shutdown.
🧹 Nitpick comments (5)
vendors/prometheus-cpp-lite/include/prometheus/prometheus.h (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#pragma onceheader guard.This is the only header in the vendored set without a guard. As per coding guidelines,
**/*.{h,hh,hpp}files should "Use#pragma oncefor header guards". Also, Line 9 uses<prometheus/file_saver.h>while Lines 2-7 use quoted includes — align for consistency.♻️ Proposed change
+#pragma once `#include` "prometheus/counter.h" `#include` "prometheus/gauge.h" `#include` "prometheus/summary.h" `#include` "prometheus/histogram.h" `#include` "prometheus/benchmark.h" `#include` "prometheus/info.h" - -#include <prometheus/file_saver.h> +#include "prometheus/file_saver.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendors/prometheus-cpp-lite/include/prometheus/prometheus.h` around lines 1 - 14, Add `#pragma` once at the beginning of prometheus.h to prevent repeated inclusion, and change the file_saver.h include to use the same quoted-include style as the other local prometheus headers.Source: Coding guidelines
code/framework/src/jobs/job_system.cpp (1)
48-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a
ScheduledCounter(priority)helper to matchQueueGauge/WaitHistogram/ExecutionHistogram.Both
Scheduleoverloads repeat the samepriority == ftl::TaskPriority::High ? g_jobsSchedHigh : g_jobsSchedNormalternary twice (once to null-check, once to dereference), duplicated across two call sites. The file already has the right pattern (QueueGauge,WaitHistogram,ExecutionHistogram) for exactly this kind of priority-keyed lookup.♻️ Suggested helper + usage
Metrics::Gauge *QueueGauge(ftl::TaskPriority priority) { return priority == ftl::TaskPriority::High ? g_jobsQueueHigh : g_jobsQueueNormal; } + + Metrics::Counter *ScheduledCounter(ftl::TaskPriority priority) { + return priority == ftl::TaskPriority::High ? g_jobsSchedHigh : g_jobsSchedNormal; + }- if (priority == ftl::TaskPriority::High ? g_jobsSchedHigh : g_jobsSchedNormal) { - (priority == ftl::TaskPriority::High ? g_jobsSchedHigh : g_jobsSchedNormal)->Inc(); - } + if (auto *scheduled = ScheduledCounter(priority)) { + scheduled->Inc(); + }(apply the same replacement in the
WaitGroup*overload)Also applies to: 152-167, 175-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/jobs/job_system.cpp` around lines 48 - 58, Extract a `ScheduledCounter(ftl::TaskPriority priority)` helper alongside `QueueGauge`, `WaitHistogram`, and `ExecutionHistogram`, returning the high- or normal-priority scheduled counter. Update both `Schedule` overloads, including the `WaitGroup*` overload, to use this helper for null-checking and dereferencing instead of repeating the priority ternary.code/framework/src/jobs/job_system.h (1)
320-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate
private:access specifier.A second
private:block was added for the new metric members instead of folding them into the existing one two lines below. Purely cosmetic but easy to clean up.🧹 Merge the two private sections
private: - Metrics::Counter *_blockingCalls = nullptr; - Metrics::Gauge *_callbackQueueDepth = nullptr; - void RecordBlockingCall(); - - private: std::unique_ptr<ftl::TaskScheduler> _scheduler; + Metrics::Counter *_blockingCalls = nullptr; + Metrics::Gauge *_callbackQueueDepth = nullptr; + void RecordBlockingCall();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/jobs/job_system.h` around lines 320 - 326, Remove the redundant first private: access specifier around the Metrics::Counter, Metrics::Gauge, and RecordBlockingCall declarations, folding those members into the existing private section that contains _scheduler.code/framework/src/networking/replication/replication_manager.h (1)
141-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
RecordStaleEpochDrop/RecordRejectedNonOwnerUpdate/RecordSerializationare public.These three are declared with no access specifier between them and the preceding public members (
OnClosedConnection,AllocConnection), making them callable by any code holding aReplicationManager*— which is reachable via the existing publicNetworkPeer::GetReplicationManager()escape hatch. They exist solely to letNetworkEntity::Serialize/Deserializereport instrumentation events; exposing them publicly lets any caller (including scripting/game code) fabricatefw_repl_*metric values with no corresponding replication event.Move them under
private:and grantNetworkEntityfriend access instead.♻️ Proposed fix
- void RecordStaleEpochDrop(int channel); - void RecordRejectedNonOwnerUpdate(); - void RecordSerialization(double durationSeconds, uint64_t bytes); - private: + friend class NetworkEntity; + void RecordStaleEpochDrop(int channel); + void RecordRejectedNonOwnerUpdate(); + void RecordSerialization(double durationSeconds, uint64_t bytes); + bool _isServer = false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/networking/replication/replication_manager.h` around lines 141 - 149, Restrict RecordStaleEpochDrop, RecordRejectedNonOwnerUpdate, and RecordSerialization to a private section in ReplicationManager, and add NetworkEntity as a friend so its Serialize/Deserialize instrumentation calls remain valid. Keep the existing public networking callbacks and connection allocation methods unchanged.code/framework/src/networking/network_peer.cpp (1)
52-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-tick scratch containers are allocated fresh every call.
SampleTransportStats()runs everyUpdate()tick and constructs threeDataStructures::Listobjects plus anstd::unordered_set<uint64_t>(live) on every invocation. Elsewhere in this same PR,InterestGridexplicitly documents reusing scratch containers as members so its per-tick hot path "performs no steady-state allocations." Consider applying the same pattern here — hoistaddresses,guids,stats, andliveto reusable members (clear/reserve instead of reconstruct) to avoid per-tick heap churn.Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/framework/src/networking/network_peer.cpp` around lines 52 - 56, Update NetworkPeer::SampleTransportStats to reuse persistent scratch members for addresses, guids, stats, and live instead of constructing them on each tick. Clear the containers at the start of each sample, reserve appropriate capacity where needed, and preserve the existing statistics-sampling behavior without steady-state per-tick allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/framework/src/http/webserver.cpp`:
- Around line 19-44: Update RouteStatusCounter and MakeRouteMetrics to accept an
HTTP method and include it as a metric label alongside route for all counters,
histogram, and gauge registrations. Thread the method through every
MakeRouteMetrics caller, including RegisterRequest and RegisterPostRequest, so
identical paths use distinct metrics; avoid resetting an already-registered
in-flight gauge during re-registration.
- Around line 53-58: Update EffectiveStatus so an unset response status defaults
to 200 unconditionally; remove the req.ranges-based 206 inference, while
preserving explicitly assigned response statuses.
In `@code/framework/src/integrations/server/instance.cpp`:
- Around line 318-336: Update MetricsConfig so metrics exporting is disabled by
default, while preserving explicit opt-in behavior in the _opts.metrics.enabled
mounting condition. Also ensure the unauthenticated token-empty case is
prominently surfaced when the exporter is enabled, using warning-level logging
rather than the existing info-only message; do not alter the existing
authenticated request flow.
- Around line 1017-1031: Update the `_nextTick` assignment to preserve the
fractional tick interval instead of converting
`Utils::Time::SecondsToMs(_opts.worldConfig.tickInterval)` to integral
`std::chrono::milliseconds`. Use a duration representation with sub-millisecond
precision while retaining the existing steady-clock scheduling behavior.
In `@code/framework/src/scripting/builtins/events.cpp`:
- Around line 722-726: Update EmitLocalCallback to measure synchronous handler
duration around its InvokeHandlersToPromiseArray call, using the same
steady_clock timing and conditional g_scriptEventDur->Observe pattern as the
existing dispatch path. Preserve the current handler invocation and promise
handling behavior.
- Around line 669-677: Clarify the outbound metric as tracking network-bound
events: remove its increment from the local/native path in EmitInternal and
increment g_scriptEventsOut within BroadcastLuaEvent for emitServer and
emitAllClients. Preserve g_scriptEventsIn for client-scoped incoming events and
ensure local Events.emit or emitLocal are not counted as outbound.
In `@code/framework/src/scripting/resource/resource_manager.cpp`:
- Around line 475-477: Update both restart-counter sites in
code/framework/src/scripting/resource/resource_manager.cpp at lines 475-477 and
557-559 to call g_scriptRestarts->Inc with affected.size() instead of the
default increment, so each affected resource is counted.
In `@code/tests/modules/metrics_ut.h`:
- Around line 141-144: Update the global fw_jobs_* metric assertions in the
affected test to avoid exact count matching against process-wide registry
values. Replace the hardcoded 0 and 1 expectations with assertions that tolerate
prior JobSystem activity, such as parsing values and verifying the relevant
counts are at least one where appropriate, while preserving the existing
metric-name and priority-label checks.
In `@vendors/prometheus-cpp-lite/include/prometheus/core.h`:
- Around line 144-164: Add the standard headers <charconv> and <limits> to the
includes used by WriteValue so both the std::to_chars path and the
std::numeric_limits fallback compile reliably.
In `@vendors/prometheus-cpp-lite/include/prometheus/summary.h`:
- Around line 341-367: Update collect() so excess observations are removed from
the underlying live_data storage, not only from the local observations snapshot.
Apply the max_observations trim through the live_data API or state used by
Snapshot, while preserving the sorted snapshot for quantile computation and
existing snapshot_count/snapshot_sum behavior.
---
Outside diff comments:
In `@code/framework/src/networking/network_server.cpp`:
- Around line 175-197: Update NetworkServer::Shutdown() to clear _peerIdentities
alongside _acceptedClients, _authenticatedClients, and _readyClients before
destroying the peer, ensuring no stale identity entries remain after bulk
shutdown.
---
Nitpick comments:
In `@code/framework/src/jobs/job_system.cpp`:
- Around line 48-58: Extract a `ScheduledCounter(ftl::TaskPriority priority)`
helper alongside `QueueGauge`, `WaitHistogram`, and `ExecutionHistogram`,
returning the high- or normal-priority scheduled counter. Update both `Schedule`
overloads, including the `WaitGroup*` overload, to use this helper for
null-checking and dereferencing instead of repeating the priority ternary.
In `@code/framework/src/jobs/job_system.h`:
- Around line 320-326: Remove the redundant first private: access specifier
around the Metrics::Counter, Metrics::Gauge, and RecordBlockingCall
declarations, folding those members into the existing private section that
contains _scheduler.
In `@code/framework/src/networking/network_peer.cpp`:
- Around line 52-56: Update NetworkPeer::SampleTransportStats to reuse
persistent scratch members for addresses, guids, stats, and live instead of
constructing them on each tick. Clear the containers at the start of each
sample, reserve appropriate capacity where needed, and preserve the existing
statistics-sampling behavior without steady-state per-tick allocations.
In `@code/framework/src/networking/replication/replication_manager.h`:
- Around line 141-149: Restrict RecordStaleEpochDrop,
RecordRejectedNonOwnerUpdate, and RecordSerialization to a private section in
ReplicationManager, and add NetworkEntity as a friend so its
Serialize/Deserialize instrumentation calls remain valid. Keep the existing
public networking callbacks and connection allocation methods unchanged.
In `@vendors/prometheus-cpp-lite/include/prometheus/prometheus.h`:
- Around line 1-14: Add `#pragma` once at the beginning of prometheus.h to prevent
repeated inclusion, and change the file_saver.h include to use the same
quoted-include style as the other local prometheus headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a574143-a4e4-4892-977e-faaef78040a9
📒 Files selected for processing (38)
code/framework/CMakeLists.txtcode/framework/src/http/webserver.cppcode/framework/src/integrations/server/instance.cppcode/framework/src/integrations/server/instance.hcode/framework/src/jobs/job_system.cppcode/framework/src/jobs/job_system.hcode/framework/src/metrics/registry.cppcode/framework/src/metrics/registry.hcode/framework/src/networking/network_peer.cppcode/framework/src/networking/network_peer.hcode/framework/src/networking/network_server.cppcode/framework/src/networking/network_server.hcode/framework/src/networking/replication/interest_grid.cppcode/framework/src/networking/replication/interest_grid.hcode/framework/src/networking/replication/network_entity.cppcode/framework/src/networking/replication/replication_manager.cppcode/framework/src/networking/replication/replication_manager.hcode/framework/src/scripting/builtins/events.cppcode/framework/src/scripting/resource/resource_manager.cppcode/tests/framework_ut.cppcode/tests/modules/metrics_ut.hvendors/CMakeLists.txtvendors/prometheus-cpp-lite/.gitignorevendors/prometheus-cpp-lite/CMakeLists.txtvendors/prometheus-cpp-lite/LICENSEvendors/prometheus-cpp-lite/README.mdvendors/prometheus-cpp-lite/include/prometheus/atomic_floating.hvendors/prometheus-cpp-lite/include/prometheus/benchmark.hvendors/prometheus-cpp-lite/include/prometheus/core.hvendors/prometheus-cpp-lite/include/prometheus/counter.hvendors/prometheus-cpp-lite/include/prometheus/file_saver.hvendors/prometheus-cpp-lite/include/prometheus/gauge.hvendors/prometheus-cpp-lite/include/prometheus/histogram.hvendors/prometheus-cpp-lite/include/prometheus/info.hvendors/prometheus-cpp-lite/include/prometheus/prometheus.hvendors/prometheus-cpp-lite/include/prometheus/simpleapi.hvendors/prometheus-cpp-lite/include/prometheus/summary.hvendors/prometheus-cpp-lite/src/simpleapi.cpp
Comment on lines
+318
to
+336
| if (_opts.metrics.enabled && _opts.webServerEnabled && _webServer) { | ||
| const std::string token = _opts.metrics.token; | ||
| const std::string path = _opts.metrics.path.empty() ? std::string("/metrics") : _opts.metrics.path; | ||
| _webServer->RegisterRequest(path, [token](const httplib::Request &req, httplib::Response &res) { | ||
| if (!token.empty()) { | ||
| const std::string auth = req.get_header_value("Authorization"); | ||
| if (auth != "Bearer " + token) { | ||
| res.status = 401; | ||
| res.set_content("unauthorized\n", "text/plain; version=0.0.4"); | ||
| return; | ||
| } | ||
| } | ||
| thread_local std::string buf; | ||
| Metrics::Registry::Get().Render(buf); | ||
| res.set_content(buf, "text/plain; version=0.0.4"); | ||
| res.status = 200; | ||
| }); | ||
| Logging::GetLogger(FRAMEWORK_INNER_HTTP)->info("Prometheus /metrics exporter mounted at {}", path); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
/metrics is enabled and unauthenticated by default.
MetricsConfig::enabled defaults to true and token defaults to empty (see instance.h MetricsConfig), so unless an operator explicitly sets a token, this mounts /metrics on the same public webserver with no auth check at all (if (!token.empty()) skips the check entirely). That exposes uptime, connection/auth-failure counts, tick health, and build info to anyone reaching the HTTP port. Consider defaulting enabled to false, or logging a loud warning (not just info) when the exporter is mounted without a token, so insecure defaults are visible to operators.
Separately, the token check itself (auth != "Bearer " + token) is a plain (non-constant-time) string compare — a minor hardening opportunity if this is ever exposed beyond a trusted network.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/framework/src/integrations/server/instance.cpp` around lines 318 - 336,
Update MetricsConfig so metrics exporting is disabled by default, while
preserving explicit opt-in behavior in the _opts.metrics.enabled mounting
condition. Also ensure the unauthenticated token-empty case is prominently
surfaced when the exporter is enabled, using warning-level logging rather than
the existing info-only message; do not alter the existing authenticated request
flow.
zpl-zak
requested changes
Jul 21, 2026
zpl-zak
left a comment
zpl-zak
left a comment
Member
There was a problem hiding this comment.
Thanks for the contribution. Here are some things that I would propose to improve:
- It is good that the metrics are backend-agnostic on the surface but it would also be nice to have the backend implementation independent from Prometheus. There should be an ability to change, using CMake, what kind of backend you would like to build with. We can use Prometheus as a default for now. That means that the registry system should be decoupled from the actual internal implementation that includes Prometheus together with framework metrics implementation.
- Please strip the AI comments. They are excessive and don't really help much with the understanding of the implementation.
- Could you provide some detail on the changes to the JobSystem::ScheduleBatch(...) function please?
- I would also suggest providing the compile-time ability to disable metrics altogether. Such an option should only provide stubs. However, there should be no code pulled from Prometheus and such.
Thank you!
- add CMake-selectable Prometheus and no-op backends - keep Prometheus as the default backend - preserve job metrics with allocation-efficient batch scheduling - add backend and ScheduleBatch test coverage
zpl-zak
approved these changes
Jul 22, 2026
applied the coderabbit suggested changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vendor prometheus-cpp-lite behind a framework-owned registry and expose a
configurable metrics endpoint.
Instrument server ticks, HTTP routes, networking, replication, jobs,
scripting, resources, and masterlist updates. Add registry and job-system
metric coverage.
Summary by CodeRabbit
New Features
/metrics) with optional bearer-token access.Bug Fixes
Tests