Added cache data collector and profiler page #21065

Nyholm · 2016-12-27T15:14:45Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#19297
License	MIT
Doc PR	n/a

Adding a first version of cache profiler page. Most things are taken from PHP-cache.
FYI: @aequasi

What is included?

A collector, recording adapter and a profiler page.

What is not included?

A good logo
Nice design on the profiler page

This PR aims to pass as the minimum requirement for a cache page. Im happy to do fancy extra features but those should be a different PR.

Nyholm · 2016-12-27T15:19:58Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

-            $loaders = array_map(function ($loader) { return new Reference($loader); }, $config['loaders']);
+            $loaders = array_map(function ($loader) {
+                return new Reference($loader);
+            }, $config['loaders']);


This change was from fabbot.io

you should revert them, fabbot is broken these days

Okey, thank you

Nyholm · 2016-12-27T15:20:02Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -674,7 +677,9 @@ private function registerTemplatingConfiguration(array $config, ContainerBuilder
        ));

        $container->setParameter('templating.engines', $config['engines']);
-        $engines = array_map(function ($engine) { return new Reference('templating.engine.'.$engine); }, $config['engines']);
+        $engines = array_map(function ($engine) {


This change was from fabbot.io

javiereguiluz · 2016-12-27T15:33:53Z

@Nyholm thanks for this PR. Don't worry about the logo because the SensioLabs designer will take care of that. And same thing about the design, don't worry because we can iterate it later. Let's focus on the contents and the information provided. Thanks!

Nyholm · 2016-12-27T15:34:38Z

That is great. Then Im done with this PR =)

("Done" = "Waiting for community feedback")

Koc · 2016-12-27T15:35:41Z

It would be very useful collect this information also from production env and analyze it later. Can anybody idea how to make it possible without enabling whole profiler?

Nyholm · 2016-12-27T15:37:16Z

It would be very useful collect this information also from production env and analyze it later. Can anybody idea how to make it possible without enabling whole profiler?

With the current implementation you should only load cache_debug.xml and then make sure to store the data from the collector somewhere on kernel.terminate

Renrhaf · 2016-12-27T16:57:26Z

src/Symfony/Component/Cache/Adapter/RecordingAdapter.php

+     *
+     * @return string
+     */
+    private function getValueRepresentation($value)


@Nyholm I'm not sure, but can't we use the VarDumper component here to print the value ? (BTW thanks a lot for this first collector, it's really nice!)

Oh. That is a good idea. I'll see what I can do. Thanks.

the representation should be handled by the data collector, using the cloneVar method of the base DataCollector

I cannot use the DataCollector::cloneVar in this context. The RecordingAdapter is not a data collector. However, I use the cloneVar once the data is moved to the CacheDataCollector

@Renrhaf I've updated the PR using your suggestion. Thank you!

@Nyholm seems good to me ;) thank you !

nicolas-grekas · 2016-12-28T07:48:12Z

src/Symfony/Component/Cache/Adapter/RecordingAdapter.php

+     *
+     * @return object
+     */
+    private function timeCall($name, array $arguments = array())


what about using a stopwatch instead?

nicolas-grekas · 2016-12-28T08:02:53Z

src/Symfony/Component/Cache/Adapter/RecordingAdapter.php

+     *
+     * @param $call
+     */
+    private function addCall($call)


this function wrapper for a oneliner could be removed

nicolas-grekas · 2016-12-28T08:03:57Z

src/Symfony/Component/Cache/Adapter/RecordingAdapterInterface.php

+/**
+ * @author Tobias Nyholm <tobias.nyholm@gmail.com>
+ */
+interface RecordingAdapterInterface


I'd remove this interface entirely: we don't want a contract here, we need only a behavior (an implem)

nicolas-grekas · 2016-12-28T08:06:40Z

src/Symfony/Component/Cache/DataCollector/CacheDataCollector.php

+            foreach ($calls as $call) {
+                $statistics[$name]['calls'] += 1;
+                $statistics[$name]['time'] += $call->time;
+                if ($call->name === 'getItem') {


what about getItems?

Thank you. If one do getItems(['a', 'b', 'c']). I will count that as 1 call and 3 reads.

nicolas-grekas · 2016-12-28T14:31:23Z

src/Symfony/Component/Cache/Adapter/RecordingAdapter.php

+        $result = $call->result;
+        $hits = 0;
+        $items = array();
+        foreach ($result as $item) {


In the case of a generator, I see two issues here:

this is consuming the generator. Since these are not rewindable, return $result; returns a non usable generator.

this is couting hits, but maybe the app didn't consume the generator on its side.

This means I think we should have two code paths here: one for array (doing the same as the current code),
and one for Traversable, which should wrap this in a generator-closure, and do things lazily.

I've wrapped the result in a generator closure as you suggested. I fail to understand we need a code path for arrays. I know that getItems will return something that you can do foreach with. Why can't I just always wrap it in a generator-closure?

no strong reason, good also via a generator

nicolas-grekas · 2016-12-28T14:40:20Z

src/Symfony/Component/Cache/Adapter/RecordingAdapter.php

+ * @author Aaron Scherer <aequasi@gmail.com>
+ * @author Tobias Nyholm <tobias.nyholm@gmail.com>
+ */
+final class RecordingAdapter implements AdapterInterface


Oh, I guess this should be named TraceableAdapter for consistency with the code base (eg TraceableUrlMatcher, etc.)

nicolas-grekas · 2016-12-28T15:18:49Z

src/Symfony/Component/Cache/Adapter/TraceableAdapter.php

+     *
+     * @return string
+     */
+    private function getValueRepresentation($value)


I think we don't need this logic, let's just use $value directly.
The clone may fail (implem can throw in __clone), and is fragile (not recursive).
and for other types of data, we don't care (resources cannot come up from a cache pool)
removing also will make the layer slimmer, which is also a nice target

Will do. Thank you.

nicolas-grekas · 2016-12-28T17:08:01Z

src/Symfony/Component/Cache/Adapter/TraceableAdapter.php

+namespace Symfony\Component\Cache\Adapter;
+
+use Psr\Cache\CacheItemInterface;
+use Psr\Cache\CacheItemPoolInterface;


not used anymore (mybad)

stof · 2016-12-28T17:00:30Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheCollectorPass.php

+<?php
+
+/*
+ * This file is part of php-cache\cache-bundle package.


this is wrong

stof · 2016-12-28T17:00:54Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheCollectorPass.php

+/*
+ * This file is part of php-cache\cache-bundle package.
+ *
+ * (c) 2015-2015 Aaron Scherer <aequasi@gmail.com>, Tobias Nyholm <tobias.nyholm@gmail.com>


Symfony code must use the Symfony copyright

Sorry, Copy/paste issue.. An my IDE does not show file headers. I will fix these

stof · 2016-12-28T17:03:29Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/cache.html.twig

+{% block head %}
+    {{ parent() }}
+    <style>
+        .color-red {


would be better to include necessary styles in the CSS, to be usable for all collectors

stof · 2016-12-28T17:06:21Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/cache.html.twig

+        {{ include('@WebProfiler/Icon/cache.svg') }}
+    </span>
+    <strong>Cache</strong>
+    <span class="count">


I don't think we should have the time here anymore, for consistency with other panels

Do you want me to remove it?

stof · 2016-12-28T17:07:08Z

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/cache.html.twig

+
+{% block panel %}
+    <h2>Cache</h2>
+    {% for name, calls in collector.calls %}


It would be great to have the aggregated stats first as big numbers (as done in other panels), before having the detail for each pool

no it is not done

Sorry, I misunderstood you. I put the aggregated stats for each adapter as big numbers.

stof · 2016-12-28T17:10:51Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheCollectorPass.php

+            $container->register($id.'.recorder', TraceableAdapter::class)
+                ->setDecoratedService($id)
+                ->addArgument(new Reference($id.'.recorder.inner'))
+                ->addArgument(new Reference('debug.stopwatch'))


The implementation does not use Stopwatch

Nyholm · 2016-12-28T19:41:02Z

xabbuh · 2016-12-28T19:55:03Z

Just by looking at the screenshot I was not able to easily recognise what "ratio" refers to. Does it mean hits/reads?

Nyholm · 2016-12-28T20:01:40Z

Sorry about that. Yes, the ratio is the hits/reads ratio. See new screenshot:

FYI: I created a separate PR with the adapters. I added some tests and fixed some bugs in it. I will rebase this PR when/if #21082 gets merged.

xabbuh · 2016-12-28T20:26:26Z

I think I would change the label from "ratio" to "hits/reads" then.

xabbuh · 2016-12-28T20:28:47Z

And what about providing aggregated data across all pools?

Nyholm · 2016-12-28T20:41:09Z

I will.

And what about providing aggregated data across all pools?

That is only in the toolbar atm.

ro0NL · 2016-12-29T10:50:33Z

Nice :)

What about a tab per pool? That should organize things right?

The table with calls looks rather technical, so maybe we can improve that UX-wise.

Try to keep simple key | value | time headers (only save calls seem to have extra metadata)
Differ with colors between gets, deletes or saves (an in-row label or so)

👍

This PR was squashed before being merged into the 3.3-dev branch (closes #21082). Discussion ---------- [Cache] TraceableAdapter | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | Related to #21065 | License | MIT | Doc PR | n/a I moved the TraceableAdapter to a separate PR and added some tests. I found a small bug which was fixed. Commits ------- 5cc4baf [Cache] TraceableAdapter

Nyholm · 2016-12-29T11:11:51Z

Thank you @ro0NL. I agree with your comments that this could be improved and I really like your suggestions. I think this is "good enough" and I will let other PRs improve this. Does it sound reasonable?

I've rebased this PR on master.

Nyholm · 2016-12-29T11:14:00Z

src/Symfony/Component/Cache/DataCollector/CacheDataCollector.php

+                    $statistics[$name]['misses'] += $count - $call->misses;
+                } elseif ($call->name === 'hasItem') {
+                    $statistics[$name]['reads'] += 1;
+                    if ($call->result->getRawData()[0][0] === false) {


This looks a bit ugly.

$call->result is a Symfony\Component\VarDumper\Cloner\Data. I know that hasItem will always return a boolean thanks to PSR-6. So I believe this is safe.

No better way for now, I may work on something better soon but this is the way to go for now.

stof · 2017-01-02T17:40:35Z

And what about providing aggregated data across all pools?

That is only in the toolbar atm.

That's unfortunate, as it means that the main info displayed in the toolbar is not available anymore when looking at details.
Btw, I made the same comment earlier about that (and you said it was done while it is not

Nyholm · 2017-01-03T09:44:47Z

Ping @stof

xabbuh · 2017-01-03T09:48:14Z

@Nyholm Any reason not to aggregate missed and deletes? I would also change the order to be consistent with how it is for the cache pools.

Nyholm · 2017-01-03T09:52:40Z

I updated the order and added deletes. Thank you for the quick feedback.

rvanlaak · 2017-01-03T12:26:35Z

We chain a apcu pool with a filesystem pool, which means we have three pools in the profiler overview. It would be nice (for a future PR) to think about some chain-specific aggregates, or maybe allow to disable profiler data collection per pool?

From DX perspective it also would be great to have some kind of (javascript?) filtering in the panel, as that would allow you to instantly see the behavior path of a specific cache item.

Nyholm · 2017-01-12T13:07:08Z

Is there anything else I can do to help getting this PR merged?

fabpot · 2017-01-12T15:58:03Z

@Nyholm If you can rebase on current master, I can merge it :)

nicolas-grekas · 2017-01-12T16:00:36Z

👍 from me
we should open an issue with the remaining ideas to not forget about them
for this next issue : about the compiler pass, ideally, we could keep track of removed private pools so that the panel show them eg grayed.

Nyholm · 2017-01-14T09:19:00Z

Thank you. The PR is rebased and tests are all green.

fabpot · 2017-01-18T05:46:31Z

Thank you @Nyholm.

@aequasi

This PR was squashed before being merged into the 3.3-dev branch (closes #21065). Discussion ---------- Added cache data collector and profiler page | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19297 | License | MIT | Doc PR | n/a Adding a first version of cache profiler page. Most things are taken from PHP-cache. FYI: @aequasi ### What is included? A collector, recording adapter and a profiler page. ![screen shot 2016-12-27 at 16 07 35](https://cloud.githubusercontent.com/assets/1275206/21502325/4bee2ed4-cc4f-11e6-89fc-37ed16aca864.png) ![screen shot 2016-12-27 at 16 07 45](https://cloud.githubusercontent.com/assets/1275206/21502326/4bee9450-cc4f-11e6-904d-527b7b0ce85b.png) ### What is not included? * A good logo * Nice design on the profiler page This PR aims to pass as the minimum requirement for a cache page. Im happy to do fancy extra features but those should be a different PR. Commits ------- 7497f1c Added cache data collector and profiler page

Nyholm · 2017-01-18T06:00:37Z

Thank you for merging

Renrhaf · 2017-01-18T07:56:34Z

Many thanks @Nyholm :)

carsonbot added Status: Needs Review Feature labels Dec 27, 2016

Nyholm mentioned this pull request Dec 27, 2016

[RFC] Integrating the Cache component in the Symfony Profiler #19297

Closed

Nyholm commented Dec 27, 2016

View reviewed changes

Renrhaf reviewed Dec 27, 2016

View reviewed changes

nicolas-grekas reviewed Dec 28, 2016

View reviewed changes

stof requested changes Dec 28, 2016

View reviewed changes

Nyholm mentioned this pull request Dec 28, 2016

[Cache] TraceableAdapter #21082

Closed

Nyholm force-pushed the cache-data-collector branch from 3e5b858 to 7341375 Compare December 29, 2016 11:09

Nyholm commented Dec 29, 2016

View reviewed changes

Nyholm added 8 commits January 14, 2017 10:02

Added cache data collector and profiler page

23c0039

Make sure we count hits/reads from hasItem

8f73434

Update logo

32376dd

yoda style

d63adcd

Added large summary numbers

321e526

rewite to show attributes

26ea3e9

Changed order

9e12fac

Added deletes

d09bcf3

Nyholm force-pushed the cache-data-collector branch from b9114b8 to d09bcf3 Compare January 14, 2017 09:03

Fixed rebase

9f1aa40

fabpot closed this Jan 18, 2017

Nyholm deleted the cache-data-collector branch January 18, 2017 08:00

javiereguiluz mentioned this pull request Feb 1, 2017

The Cache component added a data collector symfony/symfony-docs#7437

Closed

ro0NL mentioned this pull request Mar 23, 2017

[WebProfilerBundle] Fix rendering empty cache calls #22127

Closed

nicolas-grekas modified the milestone: 3.x Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Added cache data collector and profiler page #21065

Added cache data collector and profiler page #21065

Conversation

Nyholm commented Dec 27, 2016

What is included?

What is not included?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Dec 27, 2016

Nyholm commented Dec 27, 2016 • edited Loading

Koc commented Dec 27, 2016

Nyholm commented Dec 27, 2016

Renrhaf Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Dec 28, 2016

xabbuh commented Dec 28, 2016

Nyholm commented Dec 28, 2016

xabbuh commented Dec 28, 2016

xabbuh commented Dec 28, 2016

Nyholm commented Dec 28, 2016

ro0NL commented Dec 29, 2016

Nyholm commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 2, 2017

Nyholm commented Jan 3, 2017 • edited Loading

xabbuh commented Jan 3, 2017

Nyholm commented Jan 3, 2017

rvanlaak commented Jan 3, 2017

Nyholm commented Jan 12, 2017

fabpot commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017

Nyholm commented Jan 14, 2017

fabpot commented Jan 18, 2017

Nyholm commented Jan 18, 2017

Renrhaf commented Jan 18, 2017

Nyholm commented Dec 27, 2016 •

edited

Loading

Renrhaf Dec 27, 2016 •

edited

Loading

nicolas-grekas Dec 28, 2016 •

edited

Loading

Nyholm Dec 28, 2016 •

edited

Loading

nicolas-grekas Dec 28, 2016 •

edited

Loading

Nyholm commented Jan 3, 2017 •

edited

Loading