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

Adds honeybadger support. #10

wants to merge 5 commits into from

Conversation

wilkie
Copy link
Contributor

@wilkie wilkie commented Oct 20, 2023

The Dockerfile is required to be in a non-slim image because honeybadger introduces a dependency that is natively built. We will likely run into these things, anyway.

Adds the honeybadger Python library to requirements.txt

New configuration option via env to provide the honeybadger project key within the HONEYBADGER_API_KEY variable.

If the local HONEYBADGER_API_KEY variable is empty, it will check AWS Secrets Manager

Sets it up to log all exceptions that are unhandled within the app.

Sets it up to log all CRITICAL logs that happen in the app.

Sets it up to log all ERROR logs (that aren't exceptions) in the app.

Here is a view of the Honeybadger reports:

image

The first is any time logging.critical happens.

The second is any time logging.error happens.

The third is for a general Exception being raised and unhandled (which would mean a 500 error)

Digging in, here is the backtrace it provides for an Exception:

image

The Dockerfile is required to be in a non-slim image because honeybadger
introduces a dependency that is natively built. We will likely run into
these things, anyway.

Adds the `honeybadger` Python library to `requirements.txt`

New configuration option via env to provide the honeybadger project key
within the `HONEYBADGER_API_KEY` variable.

Sets it up to log all exceptions that are unhandled within the app.

Sets it up to log all CRITICAL logs that happen in the app.

Sets it up to log all ERROR logs (that aren't exceptions) in the app.
Comment on lines +64 to +67
* In production: `production/aiproxy.code.org/honeybadger_api_key`
* 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`
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.


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.

# 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.

@cat5inthecradle cat5inthecradle requested a review from a team October 20, 2023 21:37
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@wilkie
Copy link
Contributor Author

wilkie commented Oct 23, 2023

The changes to add the AWS secret integration look good. Really simple! Seems to work locally using get_secret's getenv overriding power.

Pending concerns:

  • There is some discussion of the proper way to tag the environment that would work locally and with our deployment.
  • Clarifications might be needed in the README to speak to local development or external contribution (as in, you can ignore secrets for local development)

@@ -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.

Copy link

@unlox775-code-dot-org unlox775-code-dot-org left a comment

Choose a reason for hiding this comment

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

It's been a while since I first looked here. I finally got back here. It looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants