Skip to content

Enable appsembler reporting settings #177

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.

Sign up for GitHub

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

Merged
johnbaldwin merged 1 commit into from
Nov 2, 2017
Merged

Enable appsembler reporting settings #177

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions lms/envs/appsembler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,10 @@
INTERCOM_APP_ID = APPSEMBLER_FEATURES.get('INTERCOM_APP_ID', os.environ.get('INTERCOM_APP_ID', ''))
INTERCOM_API_KEY = APPSEMBLER_FEATURES.get('INTERCOM_API_KEY', os.environ.get('INTERCOM_API_KEY', ''))
INTERCOM_USER_EMAIL = APPSEMBLER_FEATURES.get('INTERCOM_USER_EMAIL', os.environ.get('INTERCOM_USER_EMAIL', ''))


if APPSEMBLER_FEATURES.get('ENABLE_APPSEMBLER_REPORTING', False):
from appsembler_reporting.settings import APPSEMBLER_REPORTING

@johnbaldwin johnbaldwin Nov 2, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Let me know if you think I need to wrap this in a try block. I didn't because this is only called if lms.env.json has ENABLE_APPSEMBLER_REPORTING as not False

@bryanlandia bryanlandia Nov 2, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@johnbaldwin I'm not sure what you mean by

Note: This will fail if server-vars/lms.env.json has 'APPSEMBLER_FEATURES' 'ENABLE_APPSEMBLER_REPORTING as true. BUT this setting is not enabled for existing deployments that use appsembler/ficus/develop or appsembler/ficus/open-university/*

Can you rephrase or clarify? Why would it fail? Isn't the point to have this work when ENABLE_APPSEMBLER_REPORTING is true?

@bryanlandia bryanlandia Nov 2, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd probably include a try block, for cases where ENABLE_APPSEMBLER_REPORTING is accidentally left set true but the package is missing.

@johnbaldwin johnbaldwin Nov 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@bryanlandia if the appsembler_reporting is not installed then the import will fail, but to make this happen you'd have to set:

"APPSEMBLER_FEATURES": {
  "ENABLE_APPSEMBLER_REPORTING": true
}

AND not pip install appsembler_reporting

Does that help explain?

@johnbaldwin johnbaldwin Nov 2, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

A little bit belt and suspenders, but I'll add a try block and log an error message, so there is at least a clue that something is not right if the installer enables reporting but forgets to add the package

@johnbaldwin johnbaldwin Nov 2, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@bryanlandia Before I add a logger to lms/envs/appsembler.py, how would you suggest logging errors that might happen in envs/appsembler.py. Thing is, I don't want this to silently fail since the only way this can fali now is if the site is misconfigured

@bryanlandia bryanlandia Nov 2, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would just think to do a basic

import logging
logger = logging.getLogger(__name__)


try:
    ...
except ImportError:
    logger.warn('FOO')

so it shows up in the main /edx/var/log/lms/edx.log

@johnbaldwin johnbaldwin Nov 2, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So I'm less inclined to wrap this import in a try block considering the options wen the site is misconfigured (meaning appsembler_reporting NOT installed and ENABLE_APPSEMBLER_REPORTING is true:

  1. Site gets a 500 and an ImportError is shown in the logs: Should be clear what to fix there
  2. Silently fail in running reporting and unless the developer knows what shakes, spend some cycles debuging
  3. Figure out how we want to alert the admin that the site is misconfigured in production mode

Or am I missing something fundamental?

@bryanlandia bryanlandia Nov 2, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I was just thinking more about it... yes, you're right.

@johnbaldwin johnbaldwin Nov 2, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

cool, thanks! One thing I'm thinking about as we've talked over this is some easy way to get an "App health report" when we start or restart the LMS/CMS That eschews noise and just logs any failures


APPSEMBLER_REPORTING.update(APPSEMBLER_FEATURES.get(
'APPSEMBLER_REPORTING', {} ))