Skip to content
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.

Sign up for GitHub

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

Fixed the metrics for the push type. #2491

Merged
merged 3 commits into from
Apr 1, 2023
Merged

Fixed the metrics for the push type. #2491

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9404efd
Fixed the metrics for the push type.
RubenNL Dec 28, 2022
71f00b3
Parse push ping parameter with parseInt.
RubenNL Jan 12, 2023
896e338
Merge branch 'louislam:master' into fix-metrics-push
RubenNL Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/routers/api-router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const StatusPage = require("../model/status_page");
const { UptimeKumaServer } = require("../uptime-kuma-server");
const { makeBadge } = require("badge-maker");
const { badgeConstants } = require("../config");
const { Prometheus } = require("../prometheus");

let router = express.Router();

Expand Down Expand Up @@ -87,6 +88,7 @@ router.get("/api/push/:pushToken", async (request, response) => {

io.to(monitor.user_id).emit("heartbeat", bean.toJSON());
Monitor.sendStats(io, monitor.id, monitor.user_id);
new Prometheus(monitor).update(bean, undefined);
Copy link
Owner

@louislam louislam Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I don't use Prometheus, may need someone to cross check.

As here new Prometheus(monitor) is created a new Object for each request, I am not sure whether it is really ok.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the correct way to use the current design. Since we don't have a list of existing metrics, only way to update is to construct the object with the same parameters. It's a very simple object with some identifying fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I mark this as resolved?

Copy link
Contributor

@spali spali Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to jump in.
But this would be solved with #898, where I'm currently working on and my current suggested commit (054b3ec) for the PR would allow access to the existing Prometheus instance of the monitor on monitor.prometheus.update(...).

BTW: For the notes of @louislam 😉 We could think about to put stuff like monitor.prometheus.update(...) in onAfterUpdate on the bean... I don't know redbean-node good enough, but it sounds like the monitor could implement everything that has to be done on update there. So every store would automatically update prometheus and other stuff if required? This would hide monitor internal stuff from api-router or other callers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can now replace this line with monitor.getPrometheus().update(bean, undefined);


response.json({
ok: true,
Expand Down