inconsistent behavior w/ use of stats decorators

[rafrombrc]

Attempts to use the metlog.decorators.stats.timeit and metlog.decorators.stats.incr_count decorators in the Sync Server 1.1 code was giving unusual results (recorded in https://bugzilla.mozilla.org/show_bug.cgi?id=781382 and https://bugzilla.mozilla.org/show_bug.cgi?id=787287). Possible bug in decorator predicate system.

[rafrombrc]

Okay, well, rfkelly was right in that this turned out to be related to global state in the decorator. The decorator caches the metlog client for performance reasons, but in test environments where the client is torn down and reconstructed for each test case this can cause problems.

The correct way to handle this is for on the tests side, I think. If a test is explicitly testing the behavior of a metlog decorator, it should reset the decorator client to a pristine state between every test to prevent leakage. If a test is NOT directly testing a metlog decorator, then metlog decorator output should be ignored.

I've added a couple of tests (test_module_scope_multiple1 and test_module_scope_multiple2) to demonstrate how this might be done: 6a7b90ac91eab382f1369664172c7f34b73a015d. (Note the timed_add._client = None statement in the DecoratorTestBase.tearDown method.)

[rfk]

One way to handle this transparently could be to use internal signalling system and clear the cached value whenever the global state is changed. E.g. each decorator registers itself with an "on client changed" callback, and then resets its internal state each time the default client is changed.

But that may be far too much work for too little gain. Just a thought :-)

[rafrombrc]

It's definitely a reasonable idea, one that I thought about a bit. But it's not quite as straightforward as you described, since the client that is cached on a decorator might be the default client, might be some other client that is stored in the CLIENT_HOLDER, or might even be passed into the decorator directly when the decorator is instantiated. So we'd have to ignore the cases where the client was passed in directly, and the CLIENT_HOLDER would have to allow the decorators to register their callbacks against specific clients so the cache would be invalidated in the right instances. Well, I guess we could just invalidate them all whenever any CLIENT_HOLDER client was changed, it'd only be slightly less efficient to repopulate them all.

In any event, it started to feel like a bit more effort than it was worth at this very moment. I'll open another ticket to capture the idea, though, and if it becomes a pain point going forward then it might prove useful.