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

Add 'zabbix trigger' monitor #3972

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Add 'zabbix trigger' monitor #3972

wants to merge 6 commits into from

Conversation

Hex4919
Copy link

@Hex4919 Hex4919 commented Nov 1, 2023

⚠️⚠️⚠️ 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

I want to add a 'Zabbix Trigger' Monitor, which will query the API of a Zabbix Server and get the status of a trigger.

Resolves #1960

Why? I use uptime-kuma as a simple 'customer-side' status page. In the background there is more complex monitoring using Zabbix.
It would be nice to use the existing data & triggers from zabbix and show them in uptime-kuma, to get the true overview into the status page and not only part of the monitoring.

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)

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 generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

@louislam
Copy link
Owner

louislam commented Nov 24, 2023

If I understand it correctly, you meant this monitor will query the Zabbix API in a fixed interval like another monitors do.

If yes, it is OK to be added.

You can add a new monitor type here:
https://github.com/louislam/uptime-kuma/tree/master/server/monitor-types

Here is the tailscale monitor for your reference:
#3178

I am still working on 2.0.0, this pr may be expected in 2.2.0.

@louislam louislam changed the title [empty commit] pull request for Add 'zabbix trigger' monitor Add 'zabbix trigger' monitor Nov 24, 2023
@DonovanDiamond
Copy link

Let me know if assistance is needed, this would be incredible for me as I use Zabbix for our admins, and uptime kuma for our customers status pages.

@Hex4919
Copy link
Author

Hex4919 commented Nov 27, 2023

Hi, thanks for your Feedback. I already started trying out a implementation, so it shouldn't take long to finish it.

If I understand it correctly, you meant this monitor will query the Zabbix API in a fixed interval like another monitors do.

Exactly.

@louislam louislam added this to the 2.2.0 milestone Dec 1, 2023
* Add zabbix-trigger monitor type
@chakflying

This comment was marked as resolved.

@Hex4919 Hex4919 marked this pull request as ready for review December 2, 2023 10:33
@chakflying chakflying added the area:monitor Everything related to monitors label Dec 2, 2023
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

First sorry for the long review time.
Your PR somewhat got off my radar/never entered it 😅

Comment on lines +127 to +129
table.text("zabbix_instance_url");
table.text("zabbix_auth_token");
table.integer("zabbix_trigger_id");
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 added below in the migration

Suggested change
table.text("zabbix_instance_url");
table.text("zabbix_auth_token");
table.integer("zabbix_trigger_id");

const axios = require("axios");

class ZabbixTriggerMonitorType extends MonitorType {

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change

Comment on lines +13 to +14

// Make Request
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: lets link to the here docs instead

Suggested change
// Make Request
// see https://www.zabbix.com/documentation/current/en/manual/api/reference/trigger/get

Comment on lines +15 to +39
const options = {
method: "post",
url: monitor.zabbixInstanceUrl,
data: {
"jsonrpc": "2.0",
"method": "trigger.get",
"params": {
"triggerids": monitor.zabbixTriggerId,
"output": "extend",
"selectFunctions": "extend",
},
// Will be deprecated, but Bearer Auth is currently not working
"auth": monitor.zabbixAuthToken,
"id": 1
},
headers: {
"Content-Type": "application/json-rpc",
// In the future Authentication will use Bearer Token
"Authorization": `Bearer ${monitor.zabbixAuthToken}`,
}
};

// Send Request & Convert Data
let res = await axios(options);
let data = res.data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Our codebase is mostly using the axios.post syntax.
Lets not change this here ^^

Suggested change
const options = {
method: "post",
url: monitor.zabbixInstanceUrl,
data: {
"jsonrpc": "2.0",
"method": "trigger.get",
"params": {
"triggerids": monitor.zabbixTriggerId,
"output": "extend",
"selectFunctions": "extend",
},
// Will be deprecated, but Bearer Auth is currently not working
"auth": monitor.zabbixAuthToken,
"id": 1
},
headers: {
"Content-Type": "application/json-rpc",
// In the future Authentication will use Bearer Token
"Authorization": `Bearer ${monitor.zabbixAuthToken}`,
}
};
// Send Request & Convert Data
let res = await axios(options);
let data = res.data;
const options = {
headers: {
"Content-Type": "application/json-rpc",
// In the future Authentication will use Bearer Token
"Authorization": `Bearer ${monitor.zabbixAuthToken}`,
}
};
const postData = {
"jsonrpc": "2.0",
"method": "trigger.get",
"params": {
"triggerids": monitor.zabbixTriggerId,
"output": "extend",
"selectFunctions": "extend",
},
// Will be deprecated, but Bearer Auth is currently not working
"auth": monitor.zabbixAuthToken,
"id": 1
};
const { data } = await axios.post(monitor.zabbixInstanceUrl, postData, options);

let data = res.data;

// convert data to object
if (typeof data === "string") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docs don't mention that this can be a string.
=> I think this check can be removed (I have not tried to set up zabbix => idk if the json-decoding is already done at this point)

let trigger = data.result[0];

/*
Zabbix Trigger Value Mapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could not find a reference to this in their docs.
Are there more states that might be interesting?

The value also might be empty (at least it is in one of their examples). Do we have to pay special attention to this case?

<template v-if="monitor.type === 'zabbix-trigger'">
<div class="my-3">
<label for="zabbix_instance_url" class="form-label">{{ $t("zabbixInstanceUrl") }}</label>
<input id="zabbix_instance_url" v-model="monitor.zabbixInstanceUrl" type="text" class="form-control" required />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think here a placeholder what the url we expect is might be helpfull.

Suggested change
<input id="zabbix_instance_url" v-model="monitor.zabbixInstanceUrl" type="text" class="form-control" required />
<input id="zabbix_instance_url" v-model="monitor.zabbixInstanceUrl" type="text" class="form-control" placeholder="https://example.com/zabbix/api_jsonrpc.php" required />


<div class="my-3">
<label for="zabbix_trigger_id" class="form-label">{{ $t("zabbixTriggerId") }}</label>
<input id="zabbix_trigger_id" v-model="monitor.zabbixTriggerId" type="text" class="form-control" required />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have never used zabbix.
Is it nessesary to add a helptext how to find the triggerid?


<div class="my-3">
<label for="zabbix_auth_token" class="form-label">{{ $t("zabbixAuthToken") }}</label>
<input id="zabbix_auth_token" v-model="monitor.zabbixAuthToken" type="password" class="form-control" required />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use HiddenInput for such fields instead to communicate to our users that this is a sensitive field ^^


<div class="my-3">
<label for="zabbix_auth_token" class="form-label">{{ $t("zabbixAuthToken") }}</label>
<input id="zabbix_auth_token" v-model="monitor.zabbixAuthToken" type="password" class="form-control" required />
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have never used zabbix.
Is it nessesary to add a helptext how to find the auth token?

@CommanderStorm CommanderStorm marked this pull request as draft April 2, 2024 01:49
@CommanderStorm CommanderStorm added the needs:work this PR needs work label May 19, 2024
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024
@github-actions github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again needs:work this PR needs work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Triggering abillity with Centreon & Zabbix tools
5 participants