-
Notifications
You must be signed in to change notification settings - Fork 336
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 Prometheus metrics endpoint #10864
base: master
Are you sure you want to change the base?
Conversation
@@ -53,6 +60,9 @@ process.on('SIGTERM', async (signal) => { | |||
}) | |||
|
|||
const PORT = Number(__PRODUCTION__ ? process.env.PORT : process.env.SOCKET_PORT) | |||
const METRICS_PORT = Number(process.env.METRICS_PORT || 9090) // Default to 9090 |
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.
+1 Default to 9090
is obvious from the code, please remove the comment.
Useful would be why we've chosen 9090, for example 9090 is the standard prometheus port
, if that's true
}) | ||
.any('/*', createSSR) | ||
.listen(PORT, listenHandler) | ||
|
||
// Metrics App (only start if ENABLE_METRICS is 'true') |
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.
+1 (only start if ENABLE_METRICS is 'true')
is obvious, please remove
incrementWebSocketConnections() | ||
trackWebSocketOpen(ws) |
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.
-1 Why have a setting ENABLE_METRICS
if we're collecting metrics unconditionally? Instead we should conditionally bind the function here. I suggest something along the lines of
// at the top of the file
const metricsEnabled = process.env.ENABLE_METRICS === 'true'
// here
open: !metricsEnabled ? handleOpen : (ws) => {
trackOpen(ws)
handleOpen(ws)
}
// metrics.ts
export const trackOpen = ((ws: WebSocket) => {
incrementWebSocketConnections()
trackWebSocketOpen(ws)
}
This way we don't need to know how to track things here and we don't spend any effort if tracking is disabled.
Description
Fixes/Partially Fixes (https://github.com/ParabolInc/action-devops/issues/262)
Demo
https://www.loom.com/share/9aa4f099360d46479f4ab3129041c261
Testing scenarios
Final checklist