-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[NodePing] support for NodePing uptime monitoring service #2910
Conversation
|
The PR author isn't associated with NodePing (other than a happy customer) but we're willing to support this effort however we can. If the project would find a free ongoing NodePing account useful, we can provide that. |
Hi! Thanks for the awesome contribution @fileformat and for thanks for chiming in @NodePing 😄 Our testing strategy is primarily based on live tests. That way we find out about upstream API changes that break the badges. So one thing this PR will need is a couple live examples that hit the real API. The more longevity those examples have, the less work it is to keep the tests working. Re a free account, I'd be happy to give it a try! We've been using UptimeRobot which works okay, though I wouldn't mind notifications and I'm open to experiment. |
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.
Thanks for this! It's looking pretty good, and I left a few inline comments
@paulmelnikow - please sign up over at nodeping.com and then drop me a note at support@nodeping.com with the email address you used to sign up and I'll covert it to a free account. Happy to support the project. |
@paulmelnikow I added live checks, but it is rather difficult to guarantee a specific uptime percentage. I tried making a 0% test, but it seems like there is some floating point glitching going on, so it occasionally fails with "0.001%" or "0.003%" or such like. @NodePing Can you make a checkUuid that always returns a specific uptime value? |
@fileformat, sorry, we don't have the ability to create a 'fake' check like that. |
We don't want a fake check, we want a real check :) Take a look in |
@paulmelnikow, if you need a check that "always returns a specific uptime value", that won't be a real check. Uptime will change for a real check. |
Hi folks, just to clarify, we don't need to have a test that checks for a specific uptime value. This is covered in more detail in our tutorial for writing service tests, but in short the purpose of these tests is to validate that Shields can still integrate with a service and generate the respective badges correctly. For NodePing this would be a test that used a real checkUuid, and then verified the label and message (that the message is a percentage). For example:
Whenever an upstream service makes a change (api path, query param, response schema, etc.) these tests will catch it and give us a head start on updating the Shields code accordingly. The uptimerobot tests may be a useful reference as well |
Thanks for the updates! The changes are looking good, but the Edit: The same test is failing in CI as well https://circleci.com/gh/badges/shields/38972?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link |
My bad: I didn't realize that I used the 'down' checkUuid in multiple tests & deleted it. It should be working now. |
No worries! It does indeed look fixed now. I'll give this a final look over tonight but I think it's just about ready to be merged |
The "up" test is on @NodePing, so if it is down, every other NodePing test will also be down. They have a pretty good track record: if you look at the human-readable version for this checkUuid, you can see that they've only had 10 minutes of downtime in the past 2 years. The "down" report is pinging a server that doesn't exist. I can't imagine how it could start working. It is in my personal account, but I'm a very satisfied NodePing customer (they are great, especially if you are cost-sensitive and have a zillion microservices and domains). I could just delete this test, since the mocked results should still get us 100% code coverage. Let me know if you want me to. |
I also noticed that if I supply a checkUuid that's invalid, the resulting badge renders It looks like NodePing returns an HTTP 200 response with an html page that includes a message Ideally, we want the badge to display a message that is meaningful for the user (as often times it happens due to user error, such as a typo in the badge parameters). For example with HTTP 404 scenarios our badge messages are typically something like There may not be anything else we can do, but worth thinking through those scenarios |
Changed the tests per your instructions. Still 100% test coverage. I don't want to try to parse the HTML that they return when there is an error: seems iffy, and even if I got it to work, there are no stability guarantees for it. The documented API will have to do. |
Not proposing parsing html, we never do that 😄 Ideally, the API would return a different status code in that scenario but as I said there's not much we can do there. I was however wondering if there are any other error scenarios/status code we could receive, or whether the API will always return with a 200 |
I have only seen 200 responses. The docs don't say anything about status codes. Note: this is NodePing's public API. They have a more detailed private API, but that requires a security token and has write-access for the whole the account: not usable for shields. |
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.
Thanks for this!
No description provided.