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.

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

Conversation

RubenNL
Copy link
Contributor

@RubenNL RubenNL commented Dec 28, 2022

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
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]:

  • I have read and understand the pull request rules.

Description

Fixes part of #1474.

Currently, the push metrics are only updated when they "expire". This is fixed with this small change.

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

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.

@@ -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

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

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);

@louislam louislam added the help wanted May need your help to test or answer label Jan 4, 2023
@chakflying
Copy link
Collaborator

You can also fix the response time display by changing line 40:

        let ping = parseInt(request.query.ping) || null;

@RubenNL
Copy link
Contributor Author

RubenNL commented Jan 11, 2023

You can also fix the response time display by changing line 40:

        let ping = parseInt(request.query.ping) || null;

I'm trying to understand what this would do, because I don't have any errors now. The ping is reported as -1 in /metrics, which seems to be as expected?

@chakflying
Copy link
Collaborator

When I tried to send the ping with push parameters ?ping=100 it didn't update in the metrics. I guess if you don't use it you can ignore this.

@RubenNL
Copy link
Contributor Author

RubenNL commented Jan 11, 2023

Good catch, I will try that later this week and add it to the pull request.

spali added a commit to spali/uptime-kuma that referenced this pull request Jan 24, 2023
spali added a commit to spali/uptime-kuma that referenced this pull request Jan 24, 2023
spali added a commit to spali/uptime-kuma that referenced this pull request Jan 24, 2023
@louislam louislam added this to the 1.22.0 milestone Mar 19, 2023
@louislam louislam removed the help wanted May need your help to test or answer label Apr 1, 2023
@louislam louislam changed the base branch from master to 1.22.X April 1, 2023 18:04
@louislam louislam merged commit 32f84b5 into louislam:1.22.X Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants