822113 - add in-memory cache for /api/device/*

Status: RESOLVED FIXED

(Testing Graveyard :: Mozpool, defect)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

The UI already hits these endpoints quite a lot.  And I'm about to add a *lot* of HTTP requests for device statuses, based on mozpool.

We should cache this data in memory with a very short (5s?) TTL, and cache it in such a way that we only perform a few queries of the devices table during that TTL (regardless of the mix of single-device and collection API calls).  This should be done in a sufficiently generic way that the cache can be invalidated whenever a device's state changes.

This will dramtically lower the cost of serving the lifeguard.html and bmm.html pages.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Now that I look more closely, /api/device/{id}/status/ also includes logs, which we definitely do not want to query regularly via nagios.

Rather than try to cache (and invalidate) all of the existing API endpoints, I think I'll just add

 /api/device/{id}/state/

which only returns the state, and is explicitly documented to be a cached value.  Its implementation will examine the in-memory cache, and if it's out of date, fetch state for *all* devices.  For fun, I'll even include HTTP caching headers (so Apache can avoid calling the mozpool daemon).

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug822113.patch

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 692769 [details] [diff] [review]
bug822113.patch

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

::: mozpool/lifeguard/handlers.py
@@ +14,5 @@
>  urls = (
>    "/device/([^/]+)/event/([^/]+)/?", "event",
>    "/device/([^/]+)/state-change/([^/]+)/to/([^/]+)/?", "state_change",
> +  "/device/([^/]+)/status/?", "device_status",
> +  "/device/([^/]+)/state/?", "device_state",

We should document somewhere some general rules for what functions go where.  Originally I think we were putting most read-only handlers into mozpool--are we not doing that here just to keep caching in one location? (although that reasoning would only apply to /state/).

::: mozpool/web/handlers.py
@@ +4,2 @@
>  
>  """Functions common to all handlers."""

Tiny nit: these are no longer just functions. :)

@@ +52,5 @@
> +            cls = type.__new__(meta, classname, bases, classDict)
> +            cls.cache_data = None
> +            cls.cache_expires = 0
> +            cls.cache_lock = threading.Lock()
> +            return cls

I haven't used metaclasses much, so for my education, is this used to instantiate variables common to exactly one derived class, so we can have several instantiations of that class that share cache data?

@@ +56,5 @@
> +            return cls
> +
> +    def cache_fetch(self):
> +        print "FETCH"
> +        raise NotImplementedError

I find this function name a little confusing, since "fetch" and "get" are very similar verbs.  Maybe update_cache()?

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

I moved those steps because they largely return lifeguard-related data (state).  Yeah, some guidelines would be good :)

Yes, that's what the metaclass is for.

I'll fix up the other minor changes ("Functions" and update_cache) and commit.