-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
add 4 uptime metrics in prometheus #5506
base: master
Are you sure you want to change the base?
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.
Hi, thanks for the PR.
Please remember to link and issues that this is fixing so that they get autoclosed
let data24h = await uptimeCalculator.get24Hour(); | ||
let data30d = await uptimeCalculator.get30Day(); | ||
let data1y = await uptimeCalculator.get1Year(); |
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.
What is the performance impact of this?
This makes doing a ping substancially more exspensive...
Unsure if this is an isssue.
Have you tested this?
@@ -80,6 +105,76 @@ class Prometheus { | |||
} | |||
} | |||
|
|||
if (uptime) { | |||
if (typeof uptime.avgPing !== "undefined") { |
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.
I have no clue what war crimes we did commit here.
(Looking at the surrounding code, this fits in..)
I have no clue why the whole typeof
verifcation exists in this class.
Seems strange.
We know what the type we pass into this function is..
@@ -54,7 +79,7 @@ class Prometheus { | |||
* @param {object} tlsInfo TLS details | |||
* @returns {void} | |||
*/ | |||
update(heartbeat, tlsInfo) { | |||
update(heartbeat, tlsInfo, uptime) { |
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.
please adress the linting error
labelNames: commonLabels, | ||
}); | ||
|
||
const monitorAverageResponseTime = new PrometheusClient.Gauge({ |
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.
please make sure that users + developers know that this is 24h ping times by adding the 24h
suffix as above.
@@ -20,6 +20,31 @@ const monitorCertIsValid = new PrometheusClient.Gauge({ | |||
help: "Is the certificate still valid? (1 = Yes, 0= No)", | |||
labelNames: commonLabels | |||
}); | |||
|
|||
const monitorUptime1y = new PrometheusClient.Gauge({ |
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.
please go through https://prometheus.io/docs/practices/naming/ and make sure that the new metrics are better "alligned" to what prometheus wants metrics to be like ^^
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
Add 4 new values in prometheus metrics. Values are present in website for each monitor : AverageReponseTime, Uptime 24 hours, Uptime 30 days and Uptime 1 year.
I've copy the existing code and adapt it ;-)
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots (if any)
Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.