784926 - Use metlog for CEF logging in server-core

Status: RESOLVED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Ryan Kelly [:rfkelly]]

Attachment: patch touse metlog for CEF logging in server-core

This is my first attempt at using metlog for CEF logging in server-core.

One potential gotcha is that the user must remember to configure the "metlog_plugin_cef" section in their config file.  If they have a "metlog_loader" section but do not add this plugin, then attempts to log CEF messages will fail at runtime with an unhelpful error like "object has no attribute named 'cef'".

What do you think about loading the metlog_cef plugin automatically during metlog setup if it has not been added by the config?

Comment 1

[Rob Miller [:rmiller]]

Comment on attachment 654516 [details] [diff] [review]
patch touse metlog for CEF logging in server-core

Review of attachment 654516 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. And +1 to auto-loading a reasonable default metlog-cef set up, if one isn't specified. We're already doing this for metlog itself.

Comment 2

[Ryan Kelly [:rfkelly]]

Hmm, the test setup for this is not ideal.  Running the entire test suite passes fine, but running *just* the test_wsgiauth.py file fails with  "AttributeError: 'NoneType' object has no attribute 'cef'"

Since test_wsgiauth.py does not create a BaseApp instance, it doesn't create a default metlog client when run in isolation.

So currently, the tests only pass due to global state left over by other tests.

I'm not sure whether to fix this in the tests or in the code.  For example, when the WSGIAuth class tries to access CLIENT_HOLDER.default_client and finds it to be None, should it try to create one based on the configuration it was given?  Or should we just fix the tests to ensure that a default metlog client is always created?

Comment 3

[Rob Miller [:rmiller]]

(In reply to Ryan Kelly [:rfkelly] from comment #2)
> I'm not sure whether to fix this in the tests or in the code.  For example,
> when the WSGIAuth class tries to access CLIENT_HOLDER.default_client and
> finds it to be None, should it try to create one based on the configuration
> it was given?  Or should we just fix the tests to ensure that a default
> metlog client is always created?

I'd fix this in the tests. Metlog has become much more a core dependency now, the client should be created during test setup time for all test cases, not just specific ones.

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: patch to use metlog for CEF logging in server-core

OK, I have updated this patch so that services.tests.support.TestEnv will set up metlog has part of its duties, and I have changed some tests to call initenv() that previously did not need to.

Comment 5

[Ryan Kelly [:rfkelly]]

http://hg.mozilla.org/services/server-core/rev/21c674ca9f23