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 shields for Uptime Robot #947

Merged
merged 8 commits into from
Apr 20, 2017
Merged

Add shields for Uptime Robot #947

merged 8 commits into from
Apr 20, 2017

Conversation

crazy-max
Copy link
Contributor

Linked to issue #824

@crazy-max crazy-max mentioned this pull request Apr 18, 2017
@paulmelnikow
Copy link
Member

Thanks for opening this! I'm excited.

server.js Outdated
try {
var percent = parseFloat(buffer.monitors[0].custom_uptime_ratio);
badgeData.text[1] = percent + "%";
badgeData.colorscheme = floorCountColor(percent, 25, 50, 75);
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, these could be a bit higher. e.g. 90, 99, 99.9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best practice for a color level indicator with 4 color schemes seems to be :

image

10%, 30%, 50%, 70%

So maybe i can put these values instead :

//badgeData.colorscheme = floorCountColor(percent, 10, 30, 50);
if (percent <= 10) {
    badgeData.colorscheme = 'red';
} else if (percent <= 30) {
    badgeData.colorscheme = 'yellow';
} else if (percent <= 50) {
    badgeData.colorscheme = 'yellowgreen';
} else if (percent <= 70) {
    badgeData.colorscheme = 'green';
} else {
    badgeData.colorscheme = 'brightgreen';
}

"api_key": monitorApiKey,
"format": "json"
},
uri: 'https://api.uptimerobot.com/v2/getMonitors'
Copy link
Member

Choose a reason for hiding this comment

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

Could you include a link to the API docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

badgeData.text[1] = 'inaccessible';
sendBadge(format, badgeData);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a "monitor not found" branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an incorrect api key is entered for a monitor the following json is returned :

{
	"stat": "fail",
	"error": {
		"type": "invalid_parameter",
		"parameter_name": "api_key",
		"passed_value": "u956-afus321g565fghr519",
		"message": "api_key not found."
	}
}

I can add this case if this is what you want with something like that :

...
if (json.stat === 'fail') {
    badgeData.text[1] = 'unknown error';
    if (json.error) {
        badgeData.text[1] = json.error.message;
    }
    badgeData.colorscheme = 'lightgrey';
    sendBadge(format, badgeData);
    return;
}
var status = json.monitors[0].status;
if (status === 0) {

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding that. Looks great. Though, I think it would be better to check that json.error.message is a string. Otherwise if error is an empty object you could end up with undefined on the badge.

if (json.error && typeof json.error.message === 'string') {
  badgeData.text[1] = json.error.message;
} else {
  badgeData.text[1] = 'vendor error';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 91f69ae

@paulmelnikow paulmelnikow added the service-badge New or updated service badge label Apr 18, 2017
@crazy-max
Copy link
Contributor Author

Just made a new commit about your review

index.html Outdated
@@ -931,6 +931,14 @@ <h3 id="miscellaneous"> Miscellaneous </h3>
<td><img src='https://img.shields.io/swagger/valid/2.0/https/bitbucket.org/api/swagger.json.svg?maxAge=2592000' alt=''/></td>
<td><code>https://img.shields.io/swagger/valid/2.0/https/bitbucket.org/api/swagger.json.svg</code></td>
</tr>
<tr><th> Uptime Robot status: </th>
<td><img src='https://img.shields.io/uptimerobot/status/u956-afus321g565fghr519.svg?maxAge=2592000' alt=''/></td>
Copy link
Member

Choose a reason for hiding this comment

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

This example looks like the format used for Uptime Robot "full API keys". You should instead use a "monitor-specific API key" that looks like m775661887-da53aab04d8b7fe0ed74e653 (mxxxxx rather than uxxxxx at the start) as monitor-specific API keys only allow read-only access.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Let's go a step further and reject the user keys, to reduce the chance that people share them by accident.

Copy link
Contributor Author

@crazy-max crazy-max Apr 19, 2017

Choose a reason for hiding this comment

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

Yes indeed on the API documentation they don't provide a monitor api key example.
Btw i asked a question about the API rate limit and they said :

We don't apply a hard limit but monitor "bad usage" and warn if that happens. That almost never happened and no worries for that for such a legitimate usage.

So i will include an example with one of my monitor

server.js Outdated
@@ -6108,6 +6114,12 @@ cache(function(data, match, sendBadge, request) {
},
uri: 'https://api.uptimerobot.com/v2/getMonitors'
};
// A monitor API key must start with "m"
if (monitorApiKey.substring(0, "m".length) !== "m") {
badgeData.text[1] = 'api_key invalid';
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this change.

Would 'must use a monitor key' be more helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done a0e1afb

server.js Outdated

// Uptime Robot ratio integration.
// API documentation : https://uptimerobot.com/api
camp.route(/^\/uptimerobot\/ratio\/(.*)\/(.*)\.(svg|png|gif|jpg|json)$/,
Copy link
Member

Choose a reason for hiding this comment

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

In testing this out, I found it was easy to miss the numberOfDays parameter. What do you think about moving it before the key, so it's less easily missed, and making it optional?

http://img.shields.io/uptimerobot/ratio/m778918918-3e92c097147760ee39d02d36.svg
http://img.shields.io/uptimerobot/ratio/30/m778918918-3e92c097147760ee39d02d36.svg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right 1bd8766

Copy link
Member

Choose a reason for hiding this comment

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

It's still required.

Copy link
Contributor Author

@crazy-max crazy-max Apr 19, 2017

Choose a reason for hiding this comment

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

Oups forgot the optional part...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 3ed5543

server.js Outdated
@@ -6034,7 +6034,7 @@ cache(function(data, match, sendBadge, request) {
// API documentation : https://uptimerobot.com/api
camp.route(/^\/uptimerobot\/status\/(.*)\.(svg|png|gif|jpg|json)$/,
cache(function(data, match, sendBadge, request) {
var monitorApiKey = match[1]; // eg, u956-afus321g565fghr519
var monitorApiKey = match[1]; // eg, m778918918-3e92c097147760ee39d02d36
Copy link
Member

Choose a reason for hiding this comment

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

👍

@paulmelnikow
Copy link
Member

This is great! Thanks for all the tweaks!

@paulmelnikow paulmelnikow merged commit a079b68 into badges:master Apr 20, 2017
@crazy-max
Copy link
Contributor Author

Great! Do you have a roadmap for a release date on production ?

@paulmelnikow
Copy link
Member

It'll happen when @espadrine deploys. One of us can ping this thread when it does.

@paulmelnikow
Copy link
Member

This has been deployed. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants