-
Notifications
You must be signed in to change notification settings - Fork 18
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 Config switch for Xray #2261
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.
LGTM
signals.task_postrun.connect(xray_task_postrun) | ||
signals.task_prerun.connect(xray_task_prerun) | ||
signals.beat_init.connect(xray_task_prerun) | ||
if app.config["AWS_XRAY_ENABLED"] == "true": |
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.
To minimize typo and typing errors, we want:
current_app.config["AWS_XRAY_ENABLED"] is True
But that would require for the AWS_XRAY_ENABLED
constant to be declared using the bool
function of the environs
module as such:
AWS_XRAY_ENABLED = env.bool("AWS_XRAY_ENABLED", False)
The environs module is a nifty library to make configuration parsing more reliable in general.
@@ -240,6 +240,9 @@ class Config(object): | |||
DEBUG = False | |||
NOTIFY_LOG_PATH = os.getenv("NOTIFY_LOG_PATH") | |||
|
|||
# AWS Xray | |||
AWS_XRAY_ENABLED = os.getenv("AWS_XRAY_ENABLED", "false") |
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.
AWS_XRAY_ENABLED = env.bool("AWS_XRAY_ENABLED", False)
XRayMiddleware(application, xray_recorder) | ||
if application.config["AWS_XRAY_ENABLED"] == "true": | ||
xray_recorder.configure(service='celery') | ||
XRayMiddleware(application, xray_recorder) |
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 wonder how that XRayMiddleware
object works, because we do not make use of it afterward. The xray_celery_handlers do get a hold of the xray_recorder via an import and I guess this might be a singleton? but the XRayMiddleware is just left alone afterward. 🤔
Summary | Résumé
Adding logic to rely upon an environment variable to turn Xray Tracing on or off
Related Issues | Cartes liées
https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/400
Test instructions | Instructions pour tester la modification
Release Instructions | Instructions pour le déploiement
Verify the config value in the production .env file to ensure it's what we want it to be.
Reviewer checklist | Liste de vérification du réviseur