Feature/advanced error handling #294

artemlive · 2020-11-02T21:28:15Z

Here is the issue with the detailed description of this improvement: #293

merge latest

…stem and pingdom monitor with method that prevents duplicated check creation

stakater-user · 2020-11-02T21:32:09Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 1 has Failed!

stakater-user · 2020-11-02T21:43:27Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 2 has Failed!

stakater-user · 2020-11-02T21:45:24Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 3 has Failed!

stakater-user · 2020-11-02T22:48:06Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 4 has Failed!

…sn't exist

stakater-user · 2020-11-03T16:31:16Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 5 has Failed!

stakater-user · 2020-11-03T16:37:18Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 6 has Failed!

stakater-user · 2020-11-03T17:19:59Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 7 has Failed!

artemlive · 2020-11-03T17:20:15Z

Strange thing, because all tests have been passed locally.

time make test &> /dev/null; echo $?

real 0m42.501s
user 0m35.419s
sys 0m8.937s
0

This reverts commit 4684028.

stakater-user · 2020-11-03T17:46:24Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 8 has Failed!

stakater-user · 2020-11-03T17:52:07Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 9 has Failed!

artemlive · 2020-11-03T18:03:26Z

Linter, tests, and builds are fine
I see the mistake in Makefile in the verify section. At least in my local env, it doesn't work because the project doesn't have "test" directory. All other stuff is the same and it works locally. Could someone tell me what exact problem with the last build and why Jenkins build has been failed. Thank you!

stakater-user · 2020-11-12T06:03:47Z

@artemlive Yikes! You better fix it before anyone else finds out! Build 10 has Failed!

ahmedwaleedmalik · 2020-11-12T07:10:02Z

Hi @artemlive, everything was fine but the test cases were failing against a different provider. I am looking into it and hopefully will resolve that today.
In future, we will move to github actions so that anyone can view the pipeline and there is no inconvenience of such sorts. I'll try to get this prioritized !

ahmedwaleedmalik

Hi, please merge the latest master in your branch. Also, there are a fw issues with the PR:

You have replaced errors with logs at a lot of places which i don't get it that why is it required
For test cases, remove the provider == nil check since that is something i have already handled and we don't want an error in there.

ahmedwaleedmalik · 2020-11-12T09:20:45Z

pkg/controller/endpointmonitor/endpointmonitor_controller.go

+		monitor, err := findMonitorByName(r.monitorServices[index], monitorName)
+		// if there was some error while getting a monitor, re-queue it to try on the next reconcile iteration
+		if err != nil {
+			return reconcile.Result{RequeueAfter: RequeueTime}, err
+		}


If it's unable to create a monitor this will be an endless re-conciliation loop !

But if it was only the API call error? E.g. API has returned 500 code, and we would be able to add this monitor on the next iteration?

ahmedwaleedmalik · 2020-11-12T09:24:09Z

pkg/controller/endpointmonitor/endpointmonitor_controller.go

+			if err != nil {
+				log.Errorf("Error while handling update: %s", err)
+				return reconcile.Result{RequeueAfter: RequeueTime}, err
+			}


This is redundant since it already requeue's after create or update action:
https://github.com/stakater/IngressMonitorController/blob/master/pkg/controller/endpointmonitor/endpointmonitor_controller.go#L135

ahmedwaleedmalik · 2020-11-12T09:24:15Z

pkg/controller/endpointmonitor/endpointmonitor_controller.go

+			if err != nil {
+				log.Errorf("Error while handling create: %s", err)
+				return reconcile.Result{RequeueAfter: RequeueTime}, err
+			}


This is redundant since it already requeue's after create or update action:
https://github.com/stakater/IngressMonitorController/blob/master/pkg/controller/endpointmonitor/endpointmonitor_controller.go#L135

Thanks, I'll fix that.

ahmedwaleedmalik · 2020-11-12T09:26:52Z

pkg/monitors/appinsights/appinsights-monitor.go

+			log.Printf("Application Insights WebTest %s was not found in Resource Group %s", monitorName, aiService.resourceGroup)
+			return nil, nil


Why are we removing this error from here ?

ahmedwaleedmalik · 2020-11-12T09:27:00Z

pkg/monitors/gcloud/gcloud-monitor.go

+	log.Printf("There is no gcloud MonitorService with name %s found.", name)
+	return nil, nil


Why are we removing this error from here ?

ahmedwaleedmalik · 2020-11-12T09:27:24Z

pkg/monitors/pingdom/pingdom-monitor.go

+	log.Printf("There is no PingdomMonitorService with name %s found.", name)
+	return nil, nil


Why are we removing this error from here ?

ahmedwaleedmalik · 2020-11-12T09:27:57Z

pkg/monitors/statuscake/statuscake-monitor.go

+	log.Printf("There is no StatusCakeMonitorService with name %s found.", name)
+	return nil, nil


Why are we removing this error from here ?

…ntroller into feature/advanced-error-handling

stakater-user · 2020-11-12T12:33:20Z

@artemlive Image is available for testing. docker pull stakater/ingressmonitorcontroller:SNAPSHOT-PR-294-12

rasheedamir · 2021-03-14T08:38:21Z

@artemlive can you plz resolve the conflicts and lets get this merged?

rasheedamir · 2021-05-11T06:43:27Z

@artemlive any plans to update this PR?

tomjohnburton · 2021-08-23T15:51:08Z

Any update on this?? Would be great to have

namm2 · 2021-08-24T12:12:34Z

pkg/monitors/pingdom/pingdom-monitor.go

@@ -82,6 +84,26 @@ func (service *PingdomMonitorService) GetAll() []models.Monitor {
 	return monitors
 }

+// Function that gets all monitors with an error handling to avoid duplicate checks creation
+func (service *PingdomMonitorService) GetAllWithAPIErrorHandler() ([]models.Monitor, error) {


I don't know how this function can avoid duplication compared with the existing GetAll() one?

mmiller1 · 2022-01-21T18:32:13Z

I'm glad I found this PR, I was about to start working on something similar. Any chance this will make progress soon?

bilby91 · 2022-03-25T21:50:35Z

Is there any ETA for merging this PR and releasing a new version?

Thanks!

julien-michaud · 2023-03-21T17:06:25Z

Hello, we also would like this PR to be merged, thanks for your help !

artemlive · 2023-03-28T14:35:28Z

Hello, folks! I'm sorry for being gone so long. I'll review all the code and fix the conflicts as soon as possible.

karl-johan-grahn · 2023-07-26T08:21:21Z

@artemlive have you had time to look at this again?

davidgibbons · 2023-10-18T17:38:49Z

@artemlive Any chance you'll get time to actually look at this soon?

andreas-mueller-bb · 2024-02-26T08:41:02Z

@artemlive I would also be very curious if there is any update to this matter? :)

artemlive added 2 commits November 2, 2020 17:36

Merge pull request #1 from stakater/master

df47b2e

merge latest

feat: findMonitorByName with error handling, better error handling sy…

fb9a94d

…stem and pingdom monitor with method that prevents duplicated check creation

artemlive mentioned this pull request Nov 2, 2020

Pingdom duplicate checks creation #293

Open

artemlive added 2 commits November 2, 2020 23:41

fix: tests issues

b1e3a10

fix: fomatting

39ec778

fix: test condition

8f0930e

artemlive marked this pull request as draft November 3, 2020 07:54

fix: pingdom tests fixed and updownmonitor tests skip if provider doe…

7cee7e2

…sn't exist

artemlive marked this pull request as ready for review November 3, 2020 16:28

fix: comment

093e16f

fix: deletion delay on pingdom

f6cc7e0

artemlive added 3 commits November 3, 2020 19:41

fix: try to fix linter errors

4684028

Revert "fix: try to fix linter errors"

6584bce

This reverts commit 4684028.

fix: linter errors fix

2d89e3f

fix: errorf instead error

d539a7a

ahmedwaleedmalik suggested changes Nov 12, 2020

View reviewed changes

fix: remove redundant return

0f5d73a

Merge branch 'master' of https://github.com/stakater/IngressMonitorCo…

03df3e1

…ntroller into feature/advanced-error-handling

ahmedwaleedmalik added 2 commits December 4, 2020 15:05

Merge branch 'master' into feature/advanced-error-handling

702e632

Merge branch 'master' into feature/advanced-error-handling

2467994

namm2 reviewed Aug 24, 2021

View reviewed changes

ehdieunguyen approved these changes Jan 7, 2022

View reviewed changes

mmiller1 mentioned this pull request Jan 22, 2022

add rate limit backoffs & retries for uptime.com #375

Closed

		log.Printf("Application Insights WebTest %s was not found in Resource Group %s", monitorName, aiService.resourceGroup)
		return nil, nil

		log.Printf("There is no gcloud MonitorService with name %s found.", name)
		return nil, nil

		log.Printf("There is no PingdomMonitorService with name %s found.", name)
		return nil, nil

		log.Printf("There is no StatusCakeMonitorService with name %s found.", name)
		return nil, nil

Feature/advanced error handling #294

Are you sure you want to change the base?

Feature/advanced error handling #294

Conversation

artemlive commented Nov 2, 2020 • edited Loading

stakater-user commented Nov 2, 2020

stakater-user commented Nov 2, 2020

stakater-user commented Nov 2, 2020

stakater-user commented Nov 2, 2020

stakater-user commented Nov 3, 2020

stakater-user commented Nov 3, 2020

stakater-user commented Nov 3, 2020

artemlive commented Nov 3, 2020 • edited Loading

stakater-user commented Nov 3, 2020

stakater-user commented Nov 3, 2020

artemlive commented Nov 3, 2020

stakater-user commented Nov 12, 2020

ahmedwaleedmalik commented Nov 12, 2020

ahmedwaleedmalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stakater-user commented Nov 12, 2020

rasheedamir commented Mar 14, 2021

rasheedamir commented May 11, 2021

tomjohnburton commented Aug 23, 2021

namm2 Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

mmiller1 commented Jan 21, 2022

bilby91 commented Mar 25, 2022

julien-michaud commented Mar 21, 2023

artemlive commented Mar 28, 2023

karl-johan-grahn commented Jul 26, 2023

davidgibbons commented Oct 18, 2023

andreas-mueller-bb commented Feb 26, 2024

artemlive commented Nov 2, 2020 •

edited

Loading

artemlive commented Nov 3, 2020 •

edited

Loading

namm2 Aug 24, 2021 •

edited

Loading