ARC-788 error handling for the push queue (SQS) #922

KAbakumovAtl · 2021-11-17T08:14:52Z

In this PR:

Metrics emission from processPush function was removed
Added push queue error handler & tests

…rror-handling-for-push-queue

KAbakumovAtl · 2021-11-17T08:16:50Z

src/sqs/queues.ts

@@ -22,7 +22,7 @@ const sqsQueues = {
 		queueRegion: envVars.SQS_PUSH_QUEUE_REGION,
 		longPollingIntervalSec: LONG_POLLING_INTERVAL_SEC,
 		timeoutSec: 60,
-		maxAttempts: 5}, pushQueueMessageHandler,	defaultErrorHandler),
+		maxAttempts: 5}, pushQueueMessageHandler,	pushQueueErrorHandler),


We don't need a feature flag for this change because if something goes wrong we can always use existing feature flag to fallback to the Redis queue.

KAbakumovAtl · 2021-11-17T08:19:01Z

src/transforms/push.ts

@@ -106,110 +106,103 @@ export function processPushJob(app: Application) {
 }

 export const processPush = async (github: GitHubAPI, payload, rootLogger: LoggerWithTarget) => {


The only thing I did here is removing try/catch block. Many lines got changes because rows got shifted

} catch (err) { log.error({ err }, "Failed to process push"); emitWebhookFailedMetrics("push"); throw err; }

We don't need to do it here because:

If this function fail in the middleware during webhook processing, we already emit metrics in the middleware error handling (currently we emitting failure metrics twice)

For SQS we'll have its own error handling. (Otherwise we'll emit a failure on each retry)

We don't use Redis for pushes anymore, so we won't loosing its metrics

bgvozdev · 2021-11-17T22:03:37Z

src/sqs/push.ts

+
+	//Handle non-failure cases
+	const maybeResult = maybeHandleNonFailureCase(error, context);
+	if(maybeResult) {


supernit: here and in other cases: there should either be a space between if and ( everywhere or nowhere. I see both in different parts of your PR

Looks like we need to add some liner rules so it will be auto-formatted

bgvozdev · 2021-11-17T22:04:02Z

src/sqs/push.ts

+export const pushQueueErrorHandler : ErrorHandler<PushQueueMessagePayload> = async (error: JiraClientError | Octokit.HookError | RateLimitingError | Error,
+	context: Context<PushQueueMessagePayload>) : Promise<ErrorHandlingResult> => {
+
+	//Handle non-failure cases


nit: the comment repeats the code, I suggest to remove it

bgvozdev · 2021-11-17T22:04:51Z

src/sqs/push.ts

+	//Handle non-failure cases
+	const maybeResult = maybeHandleNonFailureCase(error, context);
+	if(maybeResult) {
+		return maybeResult;


~~nit: Would return !maybeResult make it clearer?~~
nit: Would return maybeResult! make it clearer?

But if there is a result we don't need to return

Ah, wait, sorry, my bad! I meant return maybeResult! 🤦 🤣 Explicitly show in the code that we are returning a value, not "maybe" value - this is what I wanted

bgvozdev · 2021-11-17T22:05:25Z

src/sqs/push.ts

+		return maybeResult;
+	}
+
+	//Handle failure cases


nit: same, IMHO the commend should tell "why?", not "what?". I'd suggest to remove the comment

bgvozdev · 2021-11-17T22:05:42Z

src/sqs/push.ts

+	const errorHandlingResult = handleFailureCase(error, context);
+
+	if(!errorHandlingResult.retryable || context.lastAttempt ) {
+		//Emit metrics if the failure is not retryable


nit: same, IMHO the commend should tell "why?", not "what?". I'd suggest to remove the comment

Ok, I guess log message explains it enough

bgvozdev · 2021-11-17T22:07:30Z

src/sqs/push.ts

+function maybeHandleNonFailureCase(error: Error, context: Context<PushQueueMessagePayload>): ErrorHandlingResult | undefined {
+	if(error instanceof JiraClientError &&
+		error.status &&
+		NON_ERRONEOUS_STATUSES.includes(error.status)) {


nit: I'd suggest NON_FAILURE_STATUSES, because you've used word "failure" for this situation before (less notions => easier to follow IMHO)

Actually, should it be something like RETRYABLE_STATUSES? I feel like "retryable/non-retryable" is more clear and explicit than "error/failure"....

And the same goes to the names of functions (e.g. maybeHandleRetryableCase)...

So the thing is when it is not a failure, we won't send a faulre metric.

So I wanted to name it to reflect taht it is not a failure

"Non-failure = we retry", is my understanding correct?

bgvozdev · 2021-11-17T22:08:36Z

src/sqs/push.ts

+	//If error is Octokit.HookError, then we need to check the response status
+	//Unfortunately we can't check if error is instance of Octokit.HookError because it is not a calss, so we'll just rely on status
+	//TODO Add error handling for the new GitHub client when it will be done
+	const maybeErrorWithStatus : any = error;


nit: Could it be const { status } = error; instead?

That was done to avoid typescript errors. If I do const { status } = error; It will say that the error doesnt' have "status" field. However its subclasses can. So I had to do this to avoid ignoring typescript&linter errors

bgvozdev · 2021-11-17T22:09:58Z

src/sqs/push.ts

+
+function handleFailureCase(error: Error, context: Context<PushQueueMessagePayload>): ErrorHandlingResult {
+	if (error instanceof RateLimitingError) {
+		// delaying until the rate limit is reset (plus a buffer of a couple seconds)


nit: same, IMHO the commend should tell "why?", not "what?". I'd suggest to remove the comment

bgvozdev · 2021-11-17T22:10:47Z

src/sqs/push.ts

@@ -39,3 +41,60 @@ export const pushQueueMessageHandler : MessageHandler<PushQueueMessagePayload> =
 	await processPush(github, payload, wrapLogger(context.log));
 }

+
+const NON_ERRONEOUS_STATUSES = [401, 404, 403];
+const RATE_LIMITING_DELAY_BUFFER = 10;


Should we maybe also include measurement units? Is it seconds? Minutes?

bgvozdev · 2021-11-17T22:11:06Z

src/sqs/push.ts

+
+const NON_ERRONEOUS_STATUSES = [401, 404, 403];
+const RATE_LIMITING_DELAY_BUFFER = 10;
+const EXPONENTIAL_BACKOFF_BASE = 60;


Should we maybe also include measurement units? Is it seconds? Minutes?

bgvozdev · 2021-11-17T22:12:56Z

src/sqs/push.ts

+		return {retryable: true, retryDelaySec: delaySec}
+	}
+
+	//Use exponential backoff in any other case


nit: same, IMHO the commend should tell "why?", not "what?". I'd suggest to remove the comment

bgvozdev · 2021-11-17T22:13:08Z

src/sqs/push.ts

+	}
+
+	//Use exponential backoff in any other case
+	const delaySec = EXPONENTIAL_BACKOFF_BASE * Math.pow(3, context.receiveCount);


Should 3 be a constant, too?

bgvozdev · 2021-11-17T22:15:27Z

test/unit/sqs/push.test.ts

+describe("Push Queue Error Handler", () => {
+
+
+	let statsdIncrementSpy = jest.spyOn(statsd, "histogram");


supernit: formatting (empty lines)

bgvozdev · 2021-11-17T22:15:53Z

test/unit/sqs/push.test.ts

+	});
+
+
+	function getJiraClientError(code: number) {


supernit: too many empty lines

bgvozdev · 2021-11-17T22:18:02Z

Great work, @KAbakumovAtl ! In general it looks good to me, the only sensible comment I have is about the notions that you are using. I see that you've used 3 different words: failure, error and non-retryable. I'd suggest to stick to a single one and rename everything else to use it. My personal perefence is "retryable/non-retryable" cause I think it is pretty explicit and describes the behaviour of the system best.

consider unifying the used notions describing "retryable/non-retryable" errors

KAbakumovAtl · 2021-11-17T22:40:38Z

I fixed what I could. Ready for re-review

KAbakumovAtl added 8 commits November 17, 2021 15:14

ARC-788 Fix approximate receive counter

2fe59f5

Reverted accidentally committed file

1e6c216

Added push queue error handler

6d6a5e6

Fixed comments

011e8db

Merge branch 'ARC-788-fix-approximate-receive-counter' into ARC-788-e…

a6121a5

…rror-handling-for-push-queue

Merge branch 'main' into ARC-788-error-handling-for-push-queue

cf0c4e0

Added tests

4898f84

Added more tests

740b967

KAbakumovAtl requested review from thombergs and bgvozdev November 17, 2021 08:14

KAbakumovAtl commented Nov 17, 2021

View reviewed changes

KAbakumovAtl changed the title ~~ARC-788 error handling for push queue~~ ARC-788 error handling for the push queue (SQS) Nov 17, 2021

bgvozdev reviewed Nov 17, 2021

View reviewed changes

test/unit/sqs/push.test.ts

});

function getJiraClientError(code: number) {

Copy link

Contributor

bgvozdev Nov 17, 2021

Choose a reason for hiding this comment

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

supernit: too many empty lines

Fixed review comments

30e87a5

bgvozdev previously approved these changes Nov 17, 2021

View reviewed changes

Changed max receive count for the push queue

8679afa

KAbakumovAtl dismissed bgvozdev’s stale review via 8679afa November 17, 2021 23:47

bgvozdev approved these changes Nov 17, 2021

View reviewed changes

tarakpurohit approved these changes Nov 17, 2021

View reviewed changes

KAbakumovAtl merged commit f6a72ca into main Nov 17, 2021

KAbakumovAtl deleted the ARC-788-error-handling-for-push-queue branch November 17, 2021 23:59

rachellerathbone mentioned this pull request May 14, 2022

[Snyk] Security upgrade sequelize-cli from 5.5.1 to 6.3.0 atlassian-forks/github-for-jira#14

Open

nickfnblum mentioned this pull request May 14, 2022

[Snyk] Security upgrade sequelize-cli from 5.5.1 to 6.3.0 nickfnblum/jira#12

Open

Manny27nyc mentioned this pull request May 14, 2022

[Snyk] Security upgrade sequelize-cli from 5.5.1 to 6.3.0 Manny27nyc/github-for-jira#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARC-788 error handling for the push queue (SQS) #922

ARC-788 error handling for the push queue (SQS) #922

KAbakumovAtl commented Nov 17, 2021

KAbakumovAtl Nov 17, 2021

KAbakumovAtl Nov 17, 2021 •

edited

Loading

bgvozdev Nov 17, 2021

KAbakumovAtl Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021 •

edited

Loading

KAbakumovAtl Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

KAbakumovAtl Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

KAbakumovAtl Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

KAbakumovAtl Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev Nov 17, 2021

bgvozdev commented Nov 17, 2021 •

edited by KAbakumovAtl

Loading

KAbakumovAtl commented Nov 17, 2021

		@@ -106,110 +106,103 @@ export function processPushJob(app: Application) {
		}

		export const processPush = async (github: GitHubAPI, payload, rootLogger: LoggerWithTarget) => {

		describe("Push Queue Error Handler", () => {


		let statsdIncrementSpy = jest.spyOn(statsd, "histogram");

ARC-788 error handling for the push queue (SQS) #922

ARC-788 error handling for the push queue (SQS) #922

Conversation

KAbakumovAtl commented Nov 17, 2021

Choose a reason for hiding this comment

KAbakumovAtl Nov 17, 2021 • edited Loading

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

bgvozdev Nov 17, 2021 • edited Loading

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

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

bgvozdev commented Nov 17, 2021 • edited by KAbakumovAtl Loading

KAbakumovAtl commented Nov 17, 2021

KAbakumovAtl Nov 17, 2021 •

edited

Loading

bgvozdev Nov 17, 2021 •

edited

Loading

bgvozdev commented Nov 17, 2021 •

edited by KAbakumovAtl

Loading