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

Adds honeybadger support. #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
FROM python:3.11-slim
FROM python:3.11

# Note: We cannot use a '3.x-slim' image because we want 'gcc' installed for
# certain packages. For instance, 'honeybadger' requires gcc via a
# dependency on 'psutil'.

WORKDIR /app
COPY requirements.txt .
Expand Down
22 changes: 22 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ to `DEBUG`, `INFO`, `WARNING`, `ERROR`, or `CRITICAL`. The `DEBUG` setting is th
most permissive and shows all logging text. The `CRITICAL` prevents most logging
from happening. Most logging happens at `INFO`, which is the default setting.

To enable Honeybadger reporting for events in the application, provide the
`HONEYBADGER_API_KEY` within the configuration or as an environment variable. When
enabled, the application will notify on Exceptions raised and any `error` or `critical`
logs.

## Local Development

- Install docker
Expand Down Expand Up @@ -254,6 +259,23 @@ build and run the container against the Python tests.

For information about testing the service, see the [Testing documentation](TESTING.md).

## Secrets

Secrets for this project follow the naming convention `${EnvironmentType}/${DNSRecord}/honeybadger_api_key`. Our security model controls access to secrets based on the prefix, and will deny read access to any secret not prefixed with `development/`, so we tell our application's Cloudformation template whether we are in a development, test, or production environment type via [config files](cicd/3-app/aiproxy/config).
unlox775-code-dot-org marked this conversation as resolved.
Show resolved Hide resolved

* In production: `production/aiproxy.code.org/honeybadger_api_key`
unlox775-code-dot-org marked this conversation as resolved.
Show resolved Hide resolved
* In test: `test/aiproxy-test.code.org/honeybadger_api_key`
* In an "adhoc" development environment: `development/aiproxy-dev-mybranch.code.org/honeybadger_api_key`
* A prod-like environment, not based on `main`: `production/aiproxy-otherbranch.code.org/honeybadger_api_key`
Comment on lines +266 to +269
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm making a decision on naming convention here, open to feedback. We are somewhat limited by our IAM permissions strategy.


Read more about environments and environment types in [cicd/README.md](cicd/README.md).

### Creating new secrets

To create a new secret, define it in "[cicd/3-app/aiproxy/template.yml](cicd/3-app/aiproxy/template.yml) in the "Secrets" section. This will create an empty secret once deployed.

Once created, you can set the value of this secret (via the AWS Console, as an Admin). Finally, deploy your code that uses the new secret, loading it via the `GetSecretValue` function from the AWS SDK.
Copy link
Contributor

Choose a reason for hiding this comment

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

This admittedly isn't the best Dev experience, we could implement a similar script to bin/update_secrets over in the code-dot-org repo if we want to make this easier. Keeping it simple for MVP.


## CICD

See [CICD Readme](./cicd/README.md)
Expand Down
3 changes: 2 additions & 1 deletion cicd/3-app/aiproxy/config/dev.config.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"Parameters": {
"BaseDomainName": "code.org",
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU"
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU",
"EnvironmentType": "development"
},
"Tags": {
"EnvType": "development"
Expand Down
3 changes: 2 additions & 1 deletion cicd/3-app/aiproxy/config/production.config.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"Parameters": {
"BaseDomainName": "code.org",
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU"
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU",
"EnvironmentType": "production"
},
"Tags": {
"EnvType": "production"
Expand Down
3 changes: 2 additions & 1 deletion cicd/3-app/aiproxy/config/test.config.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"Parameters": {
"BaseDomainName": "code.org",
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU"
"BaseDomainNameHostedZonedID": "Z2LCOI49SCXUGU",
"EnvironmentType": "test"
},
"Tags": {
"EnvType": "test"
Expand Down
23 changes: 20 additions & 3 deletions cicd/3-app/aiproxy/template.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ Description: Provision an instance of the AI Proxy service.
# Dependencies: This template has dependencies, look for !ImportValue in the Resources section.

Parameters:
EnvironmentType:
Type: String
Description: Environment type (e.g. 'development', 'staging', 'test', 'prod').
Copy link
Member

Choose a reason for hiding this comment

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

is this indicative of the environment types we plan to support for aiproxy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreso just examples to provide context. Right now we have a production environment and a non-production environment (named 'test'). We can definitely add another environment ('staging') if we would like to maintain a horizontal slice between code-dot-org and aiproxy, or we could simply point all non-prod environments of code-dot-org at the 'aiproxy-test.code.org' environment.

BaseDomainName:
Type: String
Description: Base domain name (e.g. 'code.org' in 'aiproxy.code.org').
Expand All @@ -18,9 +21,6 @@ Parameters:
Type: String
Description: URI of the Docker image in ECR.

# Conditions:
# IsDevCondition: !Equals [!Ref BaseDomainName, "dev-code.org"]

Resources:

# ------------------
Expand Down Expand Up @@ -222,13 +222,30 @@ Resources:
Essential: true
PortMappings:
- ContainerPort: 80
Environment:
# The most unique identifier for the environment is the DNS record.
- Name: ENVIRONMENT
Value: !Ref DNSRecord
- Name: ENVIRONMENT_TYPE
Value: !Ref EnvironmentType
LogConfiguration:
LogDriver: awslogs
Options:
awslogs-group: !Ref LogGroup
awslogs-region: !Ref AWS::Region
awslogs-stream-prefix: ecs

# ------------------
# Secrets
# ------------------

HoneybadgerApiKeySecret:
Type: AWS::SecretsManager::Secret
Properties:
Name: !Sub "${EnvironmentType}/${DNSRecord}/honeybadger_api_key"
Description: Honeybadger API key for this service


# ------------------
# Logging & Alerts
# ------------------
Expand Down
1 change: 1 addition & 0 deletions config.txt.sample
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
OPENAI_API_KEY=
LOG_LEVEL=INFO
#HONEYBADGER_API_KEY=hbp_xxx
1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ numpy==1.24.4
openai==0.28.1
flask==3.0.0
waitress==2.1.2
honeybadger==0.20.1
pytest==7.4.3
pytest-mock==3.12.0
requests-mock==1.11.0
Expand Down
59 changes: 57 additions & 2 deletions src/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,15 @@
from src.openai import openai_routes
from src.assessment import assessment_routes

# AWS
import boto3

# Flask
from flask import Flask

# OpenAI library
import openai
# Honeybadger support
from honeybadger.contrib import FlaskHoneybadger
from honeybadger.contrib.logger import HoneybadgerHandler

def create_app(test_config=None):

Expand Down Expand Up @@ -42,6 +46,37 @@ def create_app(test_config=None):
logging.basicConfig(format='%(asctime)s: %(name)s:%(message)s', level=log_level)
logging.log(100, f"Setting up application. Logging level={log_level}")
logging.basicConfig(format='%(asctime)s: %(levelname)s:%(name)s:%(message)s', level=log_level)


honeybadger_api_key = get_secret('honeybadger_api_key')
if honeybadger_api_key:
# Add Honeybadger support
logging.info('Setting up Honeybadger configuration')

# I need to patch Flask in order for Honeybadger to load
# The honeybadger library uses this deprecated attribute
# It was deprecated because it is now always 'True'.
# See: https://github.com/pallets/flask/commit/04994df59f2f642e52ba46ca656088bcdb931262
from flask import signals
setattr(signals, 'signals_available', True)

# Log exceptions to Honeybadger
app.config['HONEYBADGER_API_KEY'] = honeybadger_api_key
app.config['HONEYBADGER_PARAMS_FILTERS'] = 'password, secret, openai_api_key, api_key'
app.config['HONEYBADGER_ENVIRONMENT'] = os.getenv('FLASK_ENV')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is what we want here. I think we want either os.getenv('ENVIRONMENT') or os.getenv('ENVIRONMENT_TYPE'), but I'm not certain how the HONEYBADGER_ENVIRONMENT is used, or whether we should instead be trying to set FLASK_ENV = ENVIRONMENT_TYPE

For reference:

  • ENVIRONMENT is something like aiproxy.code.org or aiproxy-test.code.org
  • ENVIRONMENT_TYPE is one of development, test, or production

Neither are set in local environments, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose, then, os.getenv('ENVIRONMENT', os.getenv('FLASK_ENV', 'production'))... it is just for the purposes of Honeybadger knowing the environment for its own reporting, I believe. Optional, but maybe useful. I think we would manage grouping errors by environment with separate api keys.

Honeybadger Documentation on Environments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe it is ENVIRONMENT_TYPE not ENVIRONMENT in my proposal. either way.

FlaskHoneybadger(app, report_exceptions=True)

# Also log ERROR/CRITICAL logs to Honeybadger
class NoExceptionErrorFilter(logging.Filter):
def filter(self, record):
# But ignore Python logging exceptions on 'ERROR'
return not record.getMessage().startswith('Exception on ')

hb_handler = HoneybadgerHandler(api_key=honeybadger_api_key)
hb_handler.setLevel(logging.ERROR)
hb_handler.addFilter(NoExceptionErrorFilter())
logger = logging.getLogger()
logger.addHandler(hb_handler)

# Index (a simple HTML response that will always succeed)
@app.route('/')
Expand All @@ -53,3 +88,23 @@ def root():
app.register_blueprint(assessment_routes)

return app

# Get a secret from AWS Secrets Manager or local ENV
def get_secret(secret_name):
local_env_var_name = secret_name.upper()
# AWS Secrets are named like `production/aiproxy.code.org/secret_name}`
aws_secret_id = '/'.join([os.getenv('ENVIRONMENT'), os.getenv('ENVIRONMENT_TYPE'), secret_name])

secret = ''
if os.getenv(local_env_var_name):
secret = os.getenv(local_env_var_name)
logging.info(f'Retrieved secret "{secret_name}" from local ENV')
else:
try:
client = boto3.client('secretsmanager')
logging.info(f'Retrieved secret "{secret_name}" from AWS Secrets Manager')
secret = client.get_secret_value(SecretId=aws_secret_id)
except Exception as e:
logging.error(f'Error getting "{secret_name}" from AWS Secrets Manager: {e}')

return secret
20 changes: 19 additions & 1 deletion src/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,25 @@
def test():
return {}

# A simple JSON response that always succeeds
# A simple failing request
@test_routes.route('/test/exception')
def test_exception():
raise Exception("This is a test")
return {}

# A simple post of an error message.
@test_routes.route('/test/error')
def test_error():
logging.error("This is an error log.")
return {}

# A simple post of a critical message.
@test_routes.route('/test/critical')
def test_critical():
logging.critical("This is a critical log.")
return {}

# A simple 429 failing request.
@test_routes.route('/test/429')
def test_429():
return "Too many requests", 429
Expand Down