Middleware and metric to track per-database HTTP response egress - #5611
Middleware and metric to track per-database HTTP response egress#5611gefjon wants to merge 2 commits into
Conversation
This commit adds a new Prometheus metric, `spacetime_http_response_size_bytes_total`, which tracks bytes sent as responses to HTTP requests related to the database. This includes the `sql` and `call` routes, guest-defined HTTP handlers, logs, plus some misc. management routes. Notably, the `subscribe` route, which initiates a long-lived WebSocket connection, is not counted by the new metric, as we already track its egress separately. The new metric is tracked by an Axum middleware.
gefjon
added
the
release-any
joshua-spacetime
left a comment
joshua-spacetime
left a comment
There was a problem hiding this comment.
Looks good. I just have one billing related question.
Also can you open a PR to incorporate this metric into billing? Even if you'd like to collect some data before flipping the switch.
| let Ok(Ok(database_identity)) = name_or_identity.try_resolve(&worker_ctx).await else { | ||
| // The provided name doesn't map to an `Identity`. | ||
| // Run the route unchanged (as opposed to returning an error from the middleware) | ||
| // to preserve the error-handling behavior of the route. |
There was a problem hiding this comment.
Should we pass the resolved identity along so that the route doesn't try to resolve it again?
|
|
||
| // Remove all gauges associated with a database. | ||
| // This is useful if a database is being deleted. | ||
| /// Removes metrics associated with a database when the database is deleted. |
There was a problem hiding this comment.
The new comment is misleading. These metrics are not removed only when a database is deleted. They are removed on HostController::exit_module_host.
| /// Despite the historical function name, this cleans up per-database metric | ||
| /// series even when they are not literally `Gauge`s or `IntGauge`s. |
There was a problem hiding this comment.
Do we have billing related tests that confirm this is safe to do? A gauge reset is different than a counter reset. If handled improperly, this could result in negative billing.
And on a related note, we should probably have an alert for negative billing if we don't have one already (this is more of a note for myself).
2 participants
Description of Changes
This commit adds a new Prometheus metric,
spacetime_http_response_size_bytes_total, which tracks bytes sent as responses to HTTP requests related to the database. This includes thesqlandcallroutes, guest-defined HTTP handlers, logs, plus some misc. management routes. Notably, thesubscriberoute, which initiates a long-lived WebSocket connection, is not counted by the new metric, as we already track its egress separately.The new metric is tracked by an Axum middleware.
API and ABI breaking changes
N/a
Expected complexity level and risk
1 or 2? I don't have a huge amount of confidence any time I touch our Axum stuff, but this is just metrics, and (at least currently) not billed metrics.
Testing