-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adding edge canary on laptop #182
base: main
Are you sure you want to change the base?
Conversation
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.
A few things to change.
canaries/generate_load/alerts.py
Outdated
import numpy as np | ||
|
||
# TODO update this to the real pager duty inbox | ||
PAGER_DUTY_INBOX = '[email protected]' |
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.
Let's change this to '[email protected]' - I set it up as an email alias.
canaries/generate_load/alerts.py
Outdated
logger = logging.getLogger(__name__) | ||
|
||
# Connect to Groundlight Cloud | ||
cloud_endpoint = 'https://api.groundlight.ai/device-api' |
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.
We shoudn't explicitly set the endpoint. This is the default, and setting it here likely makes it harder to configure/override through environment.
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.
The endpoint here is explicitly set so that this Groundlight client can point to the cloud and send a heartbeat image query. If I do not explicitly set it here, it will pick up the edge endpoint from the environment variables.
canaries/generate_load/run.py
Outdated
gl = groundlight.Groundlight() | ||
|
||
# TODO parameterize this | ||
detector_id = "det_2sxWe4bhmSjcAeqNk2R9wc2xaIb" |
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.
I'd say the best practice here would be to read this from an environment variable. Expect that whatever user is running this has the environment variables set. You need to do something special in cron to make sure those environment variables get passed into cronjobs. But then just have the code error out if the environment variables aren't set.
canaries/README.md
Outdated
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.
Let's move this whole directory under cicd/
in the repo.
if [ ! -f "$ENV_FILE" ]; then | ||
echo ".env file not found! Creating a default one..." | ||
cat <<EOF > "$ENV_FILE" | ||
GROUNDLIGHT_API_TOKEN="api_xxxxxx" |
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.
This needs to be customized somehow. We don't want to check in the real token. And it's a bad practice to modify the script before running it - because that has a tendency to lead to accidentally checking in the real token. Instead, require whatever account is running this script to have the environment variable set. In bash, check that the token is set, and if it isn't raise an error and tell them to set it.
cat <<EOF > "$ENV_FILE" | ||
GROUNDLIGHT_API_TOKEN="api_xxxxxx" | ||
GROUNDLIGHT_ENDPOINT="http://localhost:30101" | ||
RTSP_URL="rtsp://..." |
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.
Same pattern here as for API token
canaries/generate_load/run.py
Outdated
|
||
# Check that the query rate is sufficiently fast (edge speed) | ||
MINIMUM_EXPECTED_BINARY_QUERY_RATE = 5.0 # this is a little conservative to avoid alerting too much | ||
if query_rate < MINIMUM_EXPECTED_BINARY_QUERY_RATE: |
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.
We should log the observed query rate - pass or fail.
canaries/generate_load/run_cron.sh
Outdated
|
||
# Load environment variables from .env | ||
if [ -f "$ENV_FILE" ]; then | ||
export $(grep -v '^#' "$ENV_FILE" | xargs) |
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.
This is a good pattern. You can also just source $(ENV_FILE)
and have the file say export FOO=BAR...
This PR adds an
edge-endpoint
canary that runs on a laptop.Completed so far:
What remains to be done:
edge-endpoint
picks up new docker images. Is this happening automatically? Probably need to add some GHA that uploads images with acanaries
orstaging
tag prior to pushing tolatest
. Maybe that should be a separate PR?