Skip to content

Enable appsembler reporting settings - #177

Merged
johnbaldwin merged 1 commit into
appsembler/ficus/masterfrom
appsembler/ficus/feature/appsembler-reporting
Nov 2, 2017
Merged

Enable appsembler reporting settings#177
johnbaldwin merged 1 commit into
appsembler/ficus/masterfrom
appsembler/ficus/feature/appsembler-reporting

Conversation

@johnbaldwin

@johnbaldwin johnbaldwin commented Nov 2, 2017

Copy link
Copy Markdown

Added capability to define settings.APPSEMBLER_REPORTING dict if
APPSEMBLER_FEATURES has 'ENABLE_APPSEMBLER_REPORTING' to true

For the ficus-common-codebase, I'm going to push an update to the appsembler_reporting repo to have the settings.py file and APPSEMBLER_REPORTING dict

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/*

Purpose is to have as small change as possible in edx-platform to support appsembler reporting development and customization

Added capability to define settings.APPSEMBLER_REPORTING dict if
APPSEMBLER_FEATURES has 'ENABLE_APPSEMBLER_REPORTING' to true
Comment thread lms/envs/appsembler.py


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

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

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?

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

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?

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

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

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

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?

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.

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

@bryanlandia
bryanlandia self-requested a review November 2, 2017 21:20

@bryanlandia bryanlandia left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OK, this is good.

@johnbaldwin

Copy link
Copy Markdown
Author

@bryanlandia Thanks, super appreciate the review!

@johnbaldwin
johnbaldwin merged commit 51e74bf into appsembler/ficus/master Nov 2, 2017
@johnbaldwin
johnbaldwin deleted the appsembler/ficus/feature/appsembler-reporting branch November 2, 2017 21:50
OmarIthawi pushed a commit that referenced this pull request Jul 16, 2020
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.

3 participants