Implement caching for registered metrics#82
Open
daniil-berg wants to merge 3 commits intomainfrom
Open
Conversation
The manager still does the `sync` operation, but DB fetching for metrics is now done by `registered_metric`. The manager is stateless (aside from its `metric_collection`). Due to static accelleration by the MUC, `registered_metric` instances do not need to be stored on the manager itself. The manager allows array-like subscript access to individual metrics. Its `filter` method allows retrieving all or some of the registered metrics from the cache. The manager also implements the `data_source_interface` to load missing metrics into the cache. It also does null-caching on collected but not yet registered metrics. The manager relies on dependency injection now. Its `dispatch_hook` method is gone. The `metric_collection` hook now defines its own dependency injection config to get dispatched automatically. The `registered_metric::to_db` method is now public. `registered_metric` implements the `cacheable_object_interface` The new `metric_tag` class extends the `core_tag_tag` for convenience. It also implements the `cacheable_object_interface`. Cache getting and setting is handled by the new `local\metrics_cache` class. To properly invalidate caches, event observers are registered for tag changes. Unit tests and Behat tests are updated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As I mentioned previously, I wanted to implement caching for our registered metrics meta-data as well as the associated tags to speed up the endpoint as much as possible. This may or may not be overkill in practice, but it was a fun exercise and helped me understand the MUC much better.
Here I introduce a couple of changes, mostly to the
metrics_manager, to make this work. These changes incidentally also made the manager's interface a bit nicer in my opinion. But I am curious what you think.TL;DR
Get the manager
Chain with
syncGet a specific registered metric by qualified name
Get all enabled metrics that carry the
footagGet all collected and registered metrics
Changes
Dependency injection
The
metric_collectionhook now defines its own dependency injection config to get dispatched automatically, when Moodle's DI container injects it somewhere. This allows themetrics_managerto be retrieved via DI as well and there is no longer a need to actively "dispatch the hook" from the outside. Since the DI container is automatically reset in Moodle's PHPUnit setup, testing also becomes much clearer. Thus thedispatch_hookmethod is gone from the manager; as is the optional$collectparameter in itssyncmethod.The canonical way to get a metrics manager is now the same as with Moodle's hook manager:
All metrics that hook into our
metrics_collectionevent, are guaranteed to be collected and stored in the manager at this point. And since the DI container shares the reference to its dependencies for the lifetime of the request, subsequent retrievals of the manager will not trigger collection again.In tests, to substitute a specific collection, we can now just do this:
As long as
resetAfterTestis called in the test method, this has no effect on subsequent tests.Array-like manager
The manager allows subscript access to individual
registered_metricinstances by qualified name, like an associative array. It implementsArrayAccess. (It only allows reading, not removing/modifying keys.)Metrics cache and filtering
Collected and registered metrics can be retrieved via the
filtermethod.Calling it without arguments will return all of them.
It always tries to get them from the cache first. With static acceleration configured, this is almost as fast as storing them on the manager itself (for the lifetime of a request).
Since the manager implements the
core_cache\data_source_interface, metrics not yet cached are fetched from the DB automatically and loaded into the cache.The manager also does explicit null-caching, so if a qualified name requested from it did not match a collected metric or that metric was not registered (yet), that fact is cached and subsequent requests no longer touch the DB.
Calling the
syncmethod of course will register any newly picked up metrics and now also re-builds the cache.Cacheable
registered_metricTo make this all convenient, the
registered_metricimplements thecacheable_object_interfaceand it stores its tags in a separate property.metric_tagsubclassThis exists mostly for convenience and extends the
core_tag_tagclass. It also implements thecacheable_object_interface.metrics_cachehelper classThis is just a convenient and type safe wrapper around the cache store functions.
Tag change events
To properly invalidate caches, event observers are registered for tag changes.
I would really appreciate your feedback on this one. I know it is a lot to go through, but I promise this is the last major feature/change I want before the v1.0 release.