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

Pingdom duplicate checks creation #293

Open
artemlive opened this issue Nov 2, 2020 · 14 comments
Open

Pingdom duplicate checks creation #293

artemlive opened this issue Nov 2, 2020 · 14 comments
Assignees
Labels
kind/bug Something isn't working kind/help wanted Extra attention is needed

Comments

@artemlive
Copy link
Contributor

artemlive commented Nov 2, 2020

Hello, the community!
I have found one major issue with Pingdom API integration. At some point, I discovered that my Pingdom monitoring slots have been significantly reduced. I checked my Pingdom account and found so many checks with the same name.
Here is the screenshot with a duplicated check example from Pingdom UI: https://prnt.sc/vbyhjm
During the investigation, I found such lines in the IMC log:

time="2020-11-01T23:40:07Z" level=info msg="Error received while listing checks: invalid character '<' looking for beginning of value"
time="2020-11-01T23:40:07Z" level=info msg="Creating Monitor: mycompany-software-master-xxx.com"

It means that the API call has been failed and IMC re-created existed monitor, because of some API error (it may be either an API service problem or transport problem, etc)
One more important thing that Pingdom allows us to create multiple checks with the same name.
Here is the method which returns all existing checks from Pingdom without error checking because of MonitorService interface implementation doesn't have an error in the returning parameters:

func (service *PingdomMonitorService) GetAll() []models.Monitor {
var monitors []models.Monitor
checks, err := service.client.Checks.List()
if err != nil {
log.Println("Error received while listing checks: ", err.Error())
return nil
}
for _, mon := range checks {
newMon := models.Monitor{
URL: mon.Hostname,
ID: fmt.Sprintf("%v", mon.ID),
Name: mon.Name,
}
monitors = append(monitors, newMon)
}
return monitors
}

Then during processing the response from the previous method, IMC generates the error if monitor wasn't found:
return match, fmt.Errorf("Unable to locate monitor with name %v", name)

But in the ReconcileEndpointMonitor we don't check the error itself, so we are able to create a duplicated monitor:

monitor := findMonitorByName(r.monitorServices[index], monitorName)

I decided to implement the method findMonitorByName without ignoring an error. It may help us to avoid duplicate checks creation. I've refactored error raising algorithm, so it will track only real errors with API calls. If monitor didn't exist, it won't be marked as an error.

As I mentioned before, there is one more big issue, which blocks me to correctly implement the GetAll with an error returning.
GetAll is the part of MonitorService that has the method prototype without the error in return parameters. Of course, I could change the interface, but then I won't be able to guarantee that all of my changes to other monitors would be fine, because I don't have enough expertise with other solutions than Pingdom and I found the same way avoiding of GetAll function usage in the uptimerobot monitor.
Here is my PR #294 (Which is still in work for that moment)

@ahmedwaleedmalik
Copy link
Contributor

As far as i understand the issue:

  • You need to create a compactor for pingdom to avoid creation of multiple monitors because i think in each reconcile loop it's creating a new monitor in your case probably.

    // TODO: Retrieve oldMonitor config and compare it here

  • findMonitorByName should not ignore the error and thank you for fixing that. But still in your case the method to retrieve a monitor should ensure that the correct monitor is being retrieved.

Let me know if we can further discuss this or if you have any more queries regarding this.

@artemlive
Copy link
Contributor Author

artemlive commented Nov 12, 2020

@ahmedwaleedmalik hello, thanks for your reply.

  • You need to create a compactor for pingdom to avoid creation of multiple monitors because i think in each reconcile loop it's creating a new monitor in your case probably.

It won't create new monitors every reconciles iteration if there weren't errors during an API call. With the current algorithm, reconcile doesn't check any errors during API calls, it just marks monitor as non-exist and doesn't handle API error itself.
Our main issue is if there was a problem with the API request (e.g. some service issue and it responds with 500 HTTP code), which checks whether a monitor exists or it has to be created:

monitor := findMonitorByName(r.monitorServices[index], monitorName)
if monitor != nil {
// Monitor already exists, update if required
err = r.handleUpdate(request, instance, *monitor, r.monitorServices[index])
} else {

So it would create a new monitor even if it does exist because Pingdom allows creating monitors with the same names.

findMonitorByName should not ignore the error and thank you for fixing that. But still in your case the method to retrieve a monitor should ensure that the correct monitor is being retrieved.

I think GetByName method is responsible for that If I got it right. We should check only if it's not null, if not we have to update exist monitor.
Сorrect me if I'm wrong.

@tomjohnburton
Copy link

I'm getting this error too. Any updates to be had?

@github-actions
Copy link

This issue is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Apr 13, 2023
@github-actions
Copy link

This issue was closed because it has been inactive for 30 days since being marked as stale.

@Dadavan
Copy link

Dadavan commented May 29, 2023

Hey guys, this is still happening, are you willing to accept a PR for this?

@devopstagon
Copy link

This is still broken :'(

@karl-johan-grahn
Copy link
Contributor

Reopening issues that inadvertently were closed as stale

@davidgibbons
Copy link

This is basically unusable as is for pingdom or statuscake. It just makes a giant mess.

@ThommyH
Copy link

ThommyH commented Jan 11, 2024

is this ever getting fixed?

@ShawnUCD
Copy link

I can confirm this is still happening with chart version 2.1.49. I can scale up the ingressmonitorcontroller deployment and watch the checks in Statuscake tick upward until we hit our limit (300).

@abhishek-annigeri-brainbits

@karl-johan-grahn @rasheedamir - Can someone please look into this? This is pending from long time.

@cwells
Copy link

cwells commented Jul 2, 2024

From my own experience writing a Puppet Pingdom module a few years ago, this is a flaw (one of many) in the Pingdom API. There's no supported way to uniquely identify a check as they allow duplicate names. IIRC I ended up tagging my checks with a unique hash so I could search for them.

Best bet: don't use Pingdom. I'm fairly confident their web UI doesn't even use their own API.

@sidick
Copy link

sidick commented Jul 15, 2024

https://developers.statuscake.com/guides/api/ratelimiting may be useful for StatusCake, as I'm seeing the duplicate checks when it gets an HTTP 429 error

@karl-johan-grahn karl-johan-grahn added kind/help wanted Extra attention is needed and removed workflow/in progress labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working kind/help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests