Merge lp:~bloodearnest/canonical-identity-provider/talisker into lp:canonical-identity-provider/release

[Simon Davy]

Add talisker, and remove explicit logging config, using the talisker logging configuration insteasd. Some dependencies now managed by talisker.

Add talisker, and remove explicit logging config. Some dependencies now managed by talisker.

The goal of talisker, in this instance
 - standard logging format
 - standard logging config (e.g. stderr)
 - standardise some cross app common dependencies

The idea is to remove app-specific config for base log output, allowing easier control of logging across projects.

In terms of usage, there's some subtle changes to logging in development

Previously:

make run:
  - gunicorn access and error logs to console
  - sso/django logs to ./logs/sso.log
make test:
  - sso/django logs to ./logs/sso.log
  - all else to console (warnings, etc)

Now:

make run: all logs, including warnings, w/same format, to console. Nothing to disk

make test: null handler, no log output by default, but logging still works properly. Individual testcases can still use in-memory log handlers to capture log messages for assertion.

Comments welcome.

[Ricardo Kirkner (ricardokirkner)]

+1 to logging to console (one of the 12-factor app tenants) as it makes the application more portable (environment independence)

[Simon Davy (bloodearnest)]

On Mon, Jul 11, 2016 at 3:57 PM, Ricardo Kirkner
<email address hidden> wrote:
> +1 to logging to console (one of the 12-factor app tenants) as it makes the application more portable (environment independence)
>
> Diff comments:
>
>>
>> === modified file 'requirements.txt'
>> --- requirements.txt 2016-07-03 22:36:48 +0000
>> +++ requirements.txt 2016-07-06 16:05:45 +0000
>> @@ -34,7 +36,6 @@
>> raven==5.6.0
>> requests-oauthlib==0.4.2
>> six==1.10.0
>> -statsd==3.1
>
> why is this removed?

Because talsisker brings it in as a dep (and upgrades it in the process)

The idea was to provide one place for our core cross-app deps to be
versioned/pulled from. Makes it easier to ensure we are on the same
versions across apps, and reduces the number of deps the app itself
has to version/manage.

At the moment, that's

gunicorn
statsd
requests

Make sense?

>> testresources==0.2.7
>> timeline==0.0.4
>> timeline-django==0.0.4
>
>
> --
> https://code.launchpad.net/~bloodearnest/canonical-identity-provider/talisker/+merge/297455
> You are the owner of lp:~bloodearnest/canonical-identity-provider/talisker.

[Ricardo Kirkner (ricardokirkner)]

> On Mon, Jul 11, 2016 at 3:57 PM, Ricardo Kirkner
> <email address hidden> wrote:
> > +1 to logging to console (one of the 12-factor app tenants) as it makes the
> application more portable (environment independence)
> >
> > Diff comments:
> >
> >>
> >> === modified file 'requirements.txt'
> >> --- requirements.txt 2016-07-03 22:36:48 +0000
> >> +++ requirements.txt 2016-07-06 16:05:45 +0000
> >> @@ -34,7 +36,6 @@
> >> raven==5.6.0
> >> requests-oauthlib==0.4.2
> >> six==1.10.0
> >> -statsd==3.1
> >
> > why is this removed?
>
> Because talsisker brings it in as a dep (and upgrades it in the process)
>
> The idea was to provide one place for our core cross-app deps to be
> versioned/pulled from. Makes it easier to ensure we are on the same
> versions across apps, and reduces the number of deps the app itself
> has to version/manage.
>
> At the moment, that's
>
> gunicorn
> statsd
> requests
>
> Make sense?

At some level, yes, but then it seems odd to rely on the implicit dependency when the code depends on the library explicitly. Also, it means that if for any reason we need to bump these lib's versions we will have to bump talisker first (for no real reason).

>
> >> testresources==0.2.7
> >> timeline==0.0.4
> >> timeline-django==0.0.4
> >
> >
> > --
> > https://code.launchpad.net/~bloodearnest/canonical-identity-
> provider/talisker/+merge/297455
> > You are the owner of lp:~bloodearnest/canonical-identity-provider/talisker.

[Ricardo Kirkner (ricardokirkner)]

review: Approve

[Simon Davy (bloodearnest)]

On Mon, Jul 11, 2016 at 4:08 PM, Ricardo Kirkner
<email address hidden> wrote:
>> On Mon, Jul 11, 2016 at 3:57 PM, Ricardo Kirkner
>> <email address hidden> wrote:
>> > +1 to logging to console (one of the 12-factor app tenants) as it makes the
>> application more portable (environment independence)
>> >
>> > Diff comments:
>> >
>> >>
>> >> === modified file 'requirements.txt'
>> >> --- requirements.txt 2016-07-03 22:36:48 +0000
>> >> +++ requirements.txt 2016-07-06 16:05:45 +0000
>> >> @@ -34,7 +36,6 @@
>> >> raven==5.6.0
>> >> requests-oauthlib==0.4.2
>> >> six==1.10.0
>> >> -statsd==3.1
>> >
>> > why is this removed?
>>
>> Because talsisker brings it in as a dep (and upgrades it in the process)
>>
>> The idea was to provide one place for our core cross-app deps to be
>> versioned/pulled from. Makes it easier to ensure we are on the same
>> versions across apps, and reduces the number of deps the app itself
>> has to version/manage.
>>
>> At the moment, that's
>>
>> gunicorn
>> statsd
>> requests
>>
>> Make sense?
>
> At some level, yes, but then it seems odd to rely on the implicit dependency when the code depends on the library explicitly.

Perhaps, but keeping a consistent version of gunicorn across services,
for example, is worth it, IMO. Statsd, perhaps not. But talisker
depends on statsd anyway. And in future, I hope that apps will get a
statsd instance via talisker's helper functions, rather than importing
it directly, thus decoupling some more.

Do we currently explicitly pin all our deps' deps currently? If so,
perhaps we should here too, but see below for the caveat.

> Also, it means that if for any reason we need to bump these lib's versions we will have to bump talisker first (for no real reason).

Actually, due to how pip works, we don't need to bump talisker here.
Pip's doesn't have full dependency resolution, it just uses the first
version it finds. Yes, this sucks, and it's why talisker appears first
in the requirements.txt.

But, we can (ab)use this - if we needed to bump statsd or similar,
we'd just put the statsd version pin *before* the talisker line, and
we should be good to go.

So, this bundling of a few key deps version in talisker is just an
idea, but I accept it might not be the best approach. But there are
some minimum versions (e.g we need gunicorn 19.5 for some bug fixes we
upstreamed relating to statsd support with custom logging).

[lp:~bloodearnest/canonical-identity-provider/talisker]

1474. By Simon Davy on 2016-08-05

update to talisker 0.4.0

1475. By Simon Davy on 2016-08-05

bump talisker to 0.4.1

Unmerged revisions

1475. By Simon Davy on 2016-08-05

bump talisker to 0.4.1

1474. By Simon Davy on 2016-08-05

update to talisker 0.4.0

1473. By Simon Davy on 2016-07-06

merging, plus talisker tweak

1472. By Simon Davy on 2016-06-15

remove comment

1471. By Simon Davy on 2016-06-10

merge

1470. By Simon Davy on 2016-06-10

log to file for test runs

1469. By Simon Davy on 2016-06-06

merging

1468. By Simon Davy on 2016-06-06

add talisker

=== modified file 'Makefile'
--- Makefile	2016-07-05 04:39:30 +0000
+++ Makefile	2016-08-05 14:48:11 +0000
@@ -35,6 +35,11 @@
 export PYTHONPATH
 export DJANGO_SETTINGS_MODULE
 
+# talisker config
+DEBUGLOG ?= logs/sso.log
+export DEBUGLOG
+export DEVEL
+
 ### Tarball ###
 $(TARBALL_BUILD_PATH): $(TARBALL_BUILD_DIR) tarball_exclude.txt cleanstatic collectstatic compilemessages-build
 	@echo "Creating deployment tarball at $(TARBALL_BUILD_PATH)"
@@ -67,9 +72,10 @@
 
 ### Wheels ###
 
-virtualenv:
+virtualenv: 
 	@virtualenv --clear --system-site-packages $(ENV)
 
+
 update-wheels: ARGS=-r requirements.txt
 update-wheels: virtualenv
 	$(PIP) wheel -w $(WHEELS_DIR) -f $(WHEELS_DIR) $(ARGS)
@@ -157,7 +163,7 @@
 
 run: ARGS=0.0.0.0:8000
 run: collectstatic django-check
-	$(ENV)/bin/gunicorn django_project.wsgi:application --workers=2 --reload --pid=logs/gunicorn.pid --bind=$(ARGS) --timeout=99999 --error-logfile=- --access-logfile=-
+	$(ENV)/bin/talisker django_project.wsgi:application --reload --workers=2 --pid=logs/gunicorn.pid --bind=$(ARGS) --timeout=99999 --access-logfile=-
 
 start: bootstrap start-db
 
 
=== modified file 'django_project/settings_base.py'
--- django_project/settings_base.py	2016-06-08 17:05:05 +0000
+++ django_project/settings_base.py	2016-08-05 14:48:11 +0000
@@ -323,52 +323,7 @@
 LOCALE_PATHS = [
     os.path.join(BASE_DIR, 'branches/translations/locale'),
 ]
-LOGGING = {
-    'disable_existing_loggers': False,
-    'filters': {},
-    'formatters': {
-        'simple': {
-            'datefmt': None,
-            'format': '%(asctime)s %(levelname)s %(name)s %(message)s',
-        },
-        'verbose': {
-            'datefmt': None,
-            'format': ('%(asctime)s %(levelname)s %(module)s %(process)d '
-                       '%(thread)d %(message)s'),
-        },
-    },
-    'handlers': {
-        'file': {
-            'filename': os.path.join(LOGS_DIR, 'sso.log'),
-            'formatter': 'simple',
-            'class': 'logging.FileHandler',
-            'filters': '',
-            'level': 'INFO',
-        },
-    },
-    'incremental': False,
-    'loggers': {
-        'sso': {
-            'level': 'INFO',
-            'propagate': False,
-            'filters': [],
-            'handlers': ['file'],
-        },
-        'django': {
-            'level': 'INFO',
-            'propagate': False,
-            'filters': [],
-            'handlers': ['file'],
-        },
-    },
-    'root': {
-        'level': 'INFO',
-        'filters': [],
-        'handlers': ['file'],
-    },
-    'version': 1,
-}
-LOGGING_CONFIG = 'django.utils.log.dictConfig'
+LOGGING_CONFIG = None
 LOGIN_REDIRECT_URL = '/'
 LOGIN_URL = '/+login'
 LOGOUT_URL = '/accounts/logout/'
 
=== modified file 'requirements.txt'
--- requirements.txt	2016-08-01 15:29:42 +0000
+++ requirements.txt	2016-08-05 14:48:11 +0000
@@ -1,3 +1,6 @@
+# talisker must go first, to ensure its versions of various dependencies are met.
+talisker==0.4.1
+
 bson==0.3.3
 canonical-raven==0.0.3
 cffi==1.6.0
@@ -14,7 +17,6 @@
 enum34==1.1.5
 gargoyle-yplan==1.2.5
 gpgservice-client==0.0.11
-gunicorn==19.3.0
 idna==2.1
 ipaddress==1.0.16
 lazr.authentication==0.1.3
@@ -34,7 +36,6 @@
 raven==5.6.0
 requests-oauthlib==0.4.2
 six==1.10.0
-statsd==3.1
 testresources==0.2.7
 timeline==0.0.4
 timeline-django==0.0.4
 
=== modified file 'src/testing/runner.py'
--- src/testing/runner.py	2016-06-09 12:44:32 +0000
+++ src/testing/runner.py	2016-08-05 14:48:11 +0000
@@ -10,6 +10,7 @@
 from gargoyle import gargoyle
 from olstests import suites
 from olstestsdjango import runner
+import talisker.logs
 
 try:
     import cProfile as profile
@@ -26,6 +27,7 @@
     """
 
     def setUp(self):
+        talisker.logs.configure_test_logging()
         super(SSOTestSuite, self).setUp()
         # gargoyle caches settings. Setting timeout to 0 prevents this,
         # increasing test isolation.