-
Couldn't load subscription status.
- Fork 130
feat: add pb usage metrics, server state #2503
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.
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
feat: add pb usage metrics, server state #2503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR introduces server state tracking and usage metrics functionality across the Rivet platform. The changes span both core and edge services, with significant modifications to server management and metrics collection.
- Added new
ServerStateenum with 7 states (Provisioning through Destroyed) incluster/src/types.rsfor tracking server lifecycle - Introduced new Prometheus metrics in pegboard service for tracking CPU/memory usage per environment and client flavor
- Created new standalone
pegboard-usage-metrics-publishservice to handle periodic collection and publishing of resource usage metrics - Moved client usage metrics from
usage_get.rsto the new standalone service for better separation of concerns - Critical: Integration tests are missing for the new functionality, with only a TODO placeholder in
integration.rs
13 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
| CASE | ||
| WHEN s.cloud_destroy_ts IS NOT NULL THEN 6 -- Destroyed | ||
| WHEN s.taint_ts IS NOT NULL AND s.drain_ts IS NOT NULL THEN 5 -- TaintedDraining | ||
| WHEN s.drain_ts IS NOT NULL THEN 4 -- Draining | ||
| WHEN s.taint_ts IS NOT NULL THEN 3 -- Tainted | ||
| WHEN s.install_complete_ts IS NOT NULL THEN 2 -- Running | ||
| WHEN s.provision_complete_ts IS NOT NULL THEN 1 -- Installing | ||
| ELSE 0 -- Provisioning | ||
| END AS state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: Missing table alias 's' in FROM clause but used in CASE statement. Add 's AS s' to the FROM clause.
| CASE | |
| WHEN s.cloud_destroy_ts IS NOT NULL THEN 6 -- Destroyed | |
| WHEN s.taint_ts IS NOT NULL AND s.drain_ts IS NOT NULL THEN 5 -- TaintedDraining | |
| WHEN s.drain_ts IS NOT NULL THEN 4 -- Draining | |
| WHEN s.taint_ts IS NOT NULL THEN 3 -- Tainted | |
| WHEN s.install_complete_ts IS NOT NULL THEN 2 -- Running | |
| WHEN s.provision_complete_ts IS NOT NULL THEN 1 -- Installing | |
| ELSE 0 -- Provisioning | |
| END AS state | |
| FROM db_cluster.servers AS s |
| CASE | ||
| WHEN s.cloud_destroy_ts IS NOT NULL THEN 6 -- Destroyed | ||
| WHEN s.taint_ts IS NOT NULL AND s.drain_ts IS NOT NULL THEN 5 -- TaintedDraining | ||
| WHEN s.drain_ts IS NOT NULL THEN 4 -- Draining | ||
| WHEN s.taint_ts IS NOT NULL THEN 3 -- Tainted | ||
| WHEN s.install_complete_ts IS NOT NULL THEN 2 -- Running | ||
| WHEN s.provision_complete_ts IS NOT NULL THEN 1 -- Installing | ||
| ELSE 0 -- Provisioning | ||
| END AS state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: State values should be defined as constants in the codebase to avoid magic numbers and ensure consistency across queries
| CASE | ||
| WHEN s.cloud_destroy_ts IS NOT NULL THEN 6 -- Destroyed | ||
| WHEN s.taint_ts IS NOT NULL AND s.drain_ts IS NOT NULL THEN 5 -- TaintedDraining | ||
| WHEN s.drain_ts IS NOT NULL THEN 4 -- Draining | ||
| WHEN s.taint_ts IS NOT NULL THEN 3 -- Tainted | ||
| WHEN s.install_complete_ts IS NOT NULL THEN 2 -- Running | ||
| WHEN s.provision_complete_ts IS NOT NULL THEN 1 -- Installing | ||
| ELSE 0 -- Provisioning | ||
| END AS state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: State transitions appear to be mutually exclusive, but consider adding explicit priority with AND NOT conditions to prevent edge cases where multiple timestamps are set.
| pub lan_ip: Option<IpAddr>, | ||
| pub wan_ip: Option<IpAddr>, | ||
| pub cloud_destroy_ts: Option<i64>, | ||
| pub state: ServerState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The Server struct doesn't derive Serialize/Deserialize traits but contains a serializable state field. This may cause issues if Server needs to be serialized
| Running = 2, | ||
| Tainted = 3, | ||
| Draining = 4, | ||
| TaintedDraining = 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: TaintedDraining combines two states - consider if this should be a separate flag instead of a combined state for better state management
| pub static ref ENV_CPU_USAGE: IntGaugeVec = register_int_gauge_vec_with_registry!( | ||
| "pegboard_env_cpu_usage", | ||
| "Total percent of CPU (per core) used by an environment.", | ||
| &["env_id", "flavor"], | ||
| *REGISTRY | ||
| ).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Missing trailing comma after *REGISTRY to maintain consistency with other metric definitions
| pub static ref ENV_CPU_USAGE: IntGaugeVec = register_int_gauge_vec_with_registry!( | |
| "pegboard_env_cpu_usage", | |
| "Total percent of CPU (per core) used by an environment.", | |
| &["env_id", "flavor"], | |
| *REGISTRY | |
| ).unwrap(); | |
| pub static ref ENV_CPU_USAGE: IntGaugeVec = register_int_gauge_vec_with_registry!( | |
| "pegboard_env_cpu_usage", | |
| "Total percent of CPU (per core) used by an environment.", | |
| &["env_id", "flavor"], | |
| *REGISTRY, | |
| ).unwrap(); |
| pub static ref ENV_MEMORY_USAGE: IntGaugeVec = register_int_gauge_vec_with_registry!( | ||
| "pegboard_env_memory_usage", | ||
| "Total MiB of memory used by an environment.", | ||
| &["env_id", "flavor"], | ||
| *REGISTRY | ||
| ).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Missing trailing comma after *REGISTRY to maintain consistency with other metric definitions
| pub static ref ENV_MEMORY_USAGE: IntGaugeVec = register_int_gauge_vec_with_registry!( | |
| "pegboard_env_memory_usage", | |
| "Total MiB of memory used by an environment.", | |
| &["env_id", "flavor"], | |
| *REGISTRY | |
| ).unwrap(); | |
| pub static ref ENV_MEMORY_USAGE: IntGaugeVec = register_int_gauge_vec_with_registry!( | |
| "pegboard_env_memory_usage", | |
| "Total MiB of memory used by an environment.", | |
| &["env_id", "flavor"], | |
| *REGISTRY, | |
| ).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Complete removal of usage metrics tracking code without a clear replacement could break existing monitoring. Ensure equivalent functionality exists elsewhere before removing this file.
| tracing = "0.1" | ||
| tracing-subscriber = { version = "0.3", default-features = false, features = ["fmt", "json", "ansi"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using workspace version for tracing and tracing-subscriber to maintain consistency with other dependencies
| if actor.start_ts.is_none() || actor.destroy_ts.is_some() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider logging skipped actors with debug level to help with monitoring and debugging
Deploying rivet with
|
| Latest commit: |
39fc637
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eefe696a.rivet.pages.dev |
| Branch Preview URL: | https://05-30-feat-add-pb-usage-metr.rivet.pages.dev |
3de4613 to
464160e
Compare
d69ce87 to
ecbaa48
Compare
464160e to
1eac431
Compare
ecbaa48 to
47e00c0
Compare
1eac431 to
2f17dc8
Compare
Deploying rivet-studio with
|
| Latest commit: |
39fc637
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6d9ce416.rivet-studio.pages.dev |
| Branch Preview URL: | https://05-30-feat-add-pb-usage-metr.rivet-studio.pages.dev |
2f17dc8 to
1eac431
Compare
47e00c0 to
ecbaa48
Compare
1eac431 to
2f17dc8
Compare
ecbaa48 to
47e00c0
Compare
Deploying rivet-hub with
|
| Latest commit: |
39fc637
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://639f7abc.rivet-hub-7jb.pages.dev |
| Branch Preview URL: | https://05-30-feat-add-pb-usage-metr.rivet-hub-7jb.pages.dev |
47e00c0 to
ecbaa48
Compare
2f17dc8 to
1eac431
Compare
ecbaa48 to
47e00c0
Compare
1eac431 to
2f17dc8
Compare
47e00c0 to
90e20fd
Compare
2f17dc8 to
39fc637
Compare

Changes