feat: add health check package and update SQS receive func

[bpeng]

Proposed Changes

Ticket https://github.com/GeoNet/tickets/issues/13878

Changes proposed in this pull request:

• package to check health
• update sqs receiveMesage to pass in a callback func to do something when the receive continues waiting on empty queue

Production Changes

The following production changes are required to deploy these changes:

• None

Review

Check the box that applies to this code review. If necessary please seek help with adding a checklist guide for the reviewer.
When assigning the code review please consider the expertise needed to review the changes.

• This is a content (documentation, web page etc) only change.
• This is a minor change (meta data, bug fix, improve test coverage etc).
• This is a larger change (new feature, significant refactoring etc). Please use the code review guidelines to add a checklist below to guide the code reviewer.

Code Review Guide

Insert check list here if needed.

[CallumNZ]

So from what I can tell, the health package is to be used by services that don't have an http server for their primary function, and simply adds one with an /soh endpoint - is that right?

How does the ECS health check work - does ECS call the endpoint at regular intervals?
How would normal usage of this package look like?

[bpeng]

The check is scheduled on AWS via terraform , the health status can be viewed on dev

[CallumNZ]

Seems wrong to couple SQS to the health check.
Can the caller be responsible for running the health check? Like how Mark does it? https://github.com/GeoNet/tickets/issues/13878#issuecomment-1667189581

[bpeng]

the health check is run by the caller, this is just sending a heartbeat in between the SQS receives, otherwise it will wait on the empty queue for ages (like the tilde-ingest-backfill) and the health check will fail, unless we don't do age check.

[CallumNZ]

Can we not trust if the queue is being listened to, all is well?

What is the ECS health check trying to achieve? Is getting a successful response from the health http server not enough? What failure states are possible?

[bpeng]

Yes, this is another option to do, but allow to send a heartbeat in between is also reasonable

[CallumNZ]

What's the reason for scoping ok like this? Strange to read.

[bpeng]

Mark's code, technically correct but not easy to read

[CallumNZ]

What's the use case for these? Might be unneeded complexity.
If the application has a long running process, wouldn't you just adjust aged ?

[bpeng]

Currently not used, don't know if there is any use case by Mark, i would keep it in case we need it later

[wilsonjord]

I think you could Pause() while you wait to receive a message, for example

[CallumNZ]

This value will need to be coordinated with the ECS health check frequency in terraform, is that right?

[bpeng]

Yes

[junghao]

My suggestion to this:

In sqs.go:

type SQS struct {
	client   *sqs.Client
	Callback *func()
}

Then in (s *SQS) receiveMessage():

for {
		if s.Callback != nil {
			(*s.Callback)()
		}
                .....

So from the service, we do

	sqsClient, err = sqs.New()
	if err != nil {
		log.Fatal(err)
	}

	callback := func() {
		log.Println("callback")
	}

	sqsClient.Callback = &callback

In this way, it becomes optional and not change the signature of the main function in SQS, nor it creates a new function to hook the callback.

[junghao]

Retrying this doesn't seem to help.

No matter it's retried or not, the caller has to dealt with IsNoMessageError, and continue the loop with the same sqsClient.receiveMewssage() call.

eg.

m, err := sqsClient.receiveMessage()
if err, ok := err.(* IsNoMessageError); ok {
  continue
}

No matter we have retry inside the wrapper or not, the caller's code doesn't change. And retrying inside the wrapper does the same number of API requests as looping from the caller.

So I suggest that we remove this retry, and let the caller decide whether it wants to count the number of retries.

[bpeng]

Retrying this doesn't seem to help.

Retrying just lets it to wait longer for messages to come, and with less interaction

[junghao]

Say, the message arrives after 99 aws.receiveMessage.

• The existing implementation takes one call to kit.receive, and no need to deal with empty retires.
  I would say this is the reason we want a retry inside kit.
• Removing the retry completely takes 99 calls to kit.receive, and in main we need to deal with empty retries.
• Adding 3 retries takes 33 calls to kit.receive, and in main we need to deal with empty retries.

Reducing the number of call to kit.receive doesn't save anything, but adding the complexity of retrying inside kit.

[bpeng]

Callback
In sqs.go:

type SQS struct {
client *sqs.Client
Callback *func()
}

Yes, this is a good option to solve the problem

[junghao]

Let's use this as a start.

[CallumNZ]

Looks good 👍