[Serializer] add WebDebugToolbar collector for normalize/denormalize actions inside the serializer. #39325

Basster · 2020-12-05T12:28:44Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #39102
License	MIT
Doc PR	do I need to add the collector to the docs?

Add a WebDebugToolbar collector for normalize/denormalize actions inside the serializer.

How it works?

All Normalizers are going to be decorated with specific implementations of AbstractTraceableNormalizer in the SerializerDebugPass.
- All decorated normalizers will get tagged with debug.normalizer, as well.
- I had to use three implementations here, since the Serializer utilizes a lot of instanceof checks.
The SerializerDataCollector collects all serializations, deserializations, normalizations & denormalizations from all services with the debug.normalizer tag.
On the serializer_debug.php the serializer services gets decorated with a TraceableSerializer and the SerializerDataCollector gets registered.

#SymfonyHackday

carsonbot · 2020-12-05T12:28:45Z

Hey!

Nice work here, I this makes me happy.

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

src/Symfony/Component/Serializer/Debug/Normalizer/AbstractTraceableNormalizer.php

src/Symfony/Component/Serializer/Debug/Normalizer/Denormalization.php

src/Symfony/Component/Serializer/Debug/Normalizer/Normalization.php

src/Symfony/Component/Serializer/Debug/TraceableSerializer.php

src/Symfony/Component/Serializer/Tests/Debug/TraceableSerializerTest.php

Basster · 2020-12-05T14:56:47Z

Not to self: add a __call to the AbstractTraceableNormalizer. to delegate all calls, the decorated normalizers don't know to their delegates, if they have such kind of method.

__callStatic as well? What do you think @derrabus?

derrabus

Looks good so far. I think we need to make sure that we limit the amount of data that we actually store in the profile. If an app serializes 120 MiB of data, we probably don't want the full output to be logged.

src/Symfony/Component/Serializer/DataCollector/SerializerDataCollector.php

src/Symfony/Component/Serializer/Debug/Normalizer/AbstractTraceableNormalizer.php

src/Symfony/Component/Serializer/Debug/Normalizer/Denormalization.php

src/Symfony/Component/Serializer/Debug/Normalizer/Normalization.php

src/Symfony/Component/Serializer/Tests/Debug/Normalizer/TestCacheableNormalizer.php

src/Symfony/Component/Serializer/Tests/Debug/TraceableSerializerTest.php

jderusse

Thank you for working on this @Basster

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

jderusse · 2020-12-05T17:50:32Z

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

+            ->setArguments([$normalizerDef])
+            ->addTag('debug.normalizer')
+            ->setDecoratedService($id)
+            ->setAutowired(true)


Is it needed?

same for autoconfigure

I don't think it's required indeed?

src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer_debug.php

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

src/Symfony/Component/Serializer/Debug/Normalizer/AbstractTraceableNormalizer.php

src/Symfony/Component/Serializer/Debug/SerializerAction.php

src/Symfony/Component/Serializer/Debug/TraceableSerializer.php

Basster · 2020-12-07T07:21:07Z

Thank you @derrabus and @jderusse for your input. I'm trying to get over your comments, as soon as I can.

Basster · 2020-12-21T09:46:36Z

__callStatic as well? What do you think @derrabus?

I think __callStatic makes no sense, because only injected normalizers will get decorated.

Basster · 2020-12-21T13:28:24Z

I have no clue, why travis fails this particular build?

…und it.

…tor and data_collector for the container.

* and some code improvements.

… icon on serializer.html.twig.

…ep custom methods on custom normalizers working, which are not part of the interface.

…memory by replacing too large source-objects/results as replacement (LargeContent).

…assTest.

Basster · 2021-03-06T13:05:53Z

I'm still convinced that's not my fault, that appveyor and travis fail, since the 5.x branch currently fails at the same places. So please @derrabus and @jderusse, it would be kind if you could look over this, again.

ogizanagi

This is a huge work and needs to be tested in real-life applications with extensive usage of the Serializer to see how it behaves and how we can make it the most relevant.
It'll also be the the starting point for a case I left in #27262, about having Serializer metrics in the performance panel.

I think there is still a bit of work, but thank you @Basster for working on this!

ogizanagi · 2021-10-30T09:19:02Z

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

+        } elseif ($isDenormalizer) {
+            $decoratorClass = TraceableDenormalizer::class;
+        } else {
+            throw new RuntimeException(sprintf('Normalizer with id "%s" neither implements NormalizerInterface nor DenormalizerInterface!', $id));


I think it should rather be a LogicException, since it' a user misconfiguration/code issue.
But I also wonder why this pass would throw while the SerializerPass doesn't?

If it's interesting DX-wise, it should be in the SerializerPass (not necessarily in this PR).

ogizanagi · 2021-10-30T09:26:21Z

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

+            ->setArguments([$normalizerDef])
+            ->addTag('debug.normalizer')
+            ->setDecoratedService($id)
+            ->setAutowired(true)


I don't think it's required indeed?

ogizanagi · 2021-10-30T09:34:58Z

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

+            ->setAutowired(true)
+        ;
+
+        $container->setDefinition($aliasName, $decoratorDef);


$aliasName could be inlined here. Also what about using ->register directly?

ogizanagi · 2021-10-30T09:41:54Z

src/Symfony/Component/Serializer/Debug/Normalizer/AbstractTraceableNormalizer.php

+            return $this->delegate->$name(...$arguments);
+        }
+
+        throw new \LogicException('Unexpected method call.');


Suggested change

throw new \LogicException('Unexpected method call.');

throw new \BadMethodCallException(sprintf('Unexpected "%s" method call.', $name));

?

ogizanagi · 2021-10-30T09:43:09Z

src/Symfony/Component/Serializer/Debug/Normalizer/AbstractTraceableNormalizer.php

+
+    public function setSerializer(SerializerInterface $serializer): void


Suggested change

public function setSerializer(SerializerInterface $serializer): void

public function setSerializer(SerializerInterface $serializer): void

ogizanagi · 2021-10-30T09:44:21Z

src/Symfony/Component/Serializer/Debug/SerializerActionFactoryInterface.php

+use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
+use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
+
+interface SerializerActionFactoryInterface


What is the use-case for this interface?
I don't think we need it

ogizanagi · 2021-10-30T09:58:35Z

src/Symfony/Component/Serializer/Debug/SerializerActionFactory.php

+    public function createDenormalization(DenormalizerInterface $denormalizer, $data, $result, string $type, string $format, array $context = []): Denormalization
+    {
+        return new Denormalization(
+            $denormalizer,


Do we need to hold a reference on the (de)normalizer instance here? What about storing the classname or original service name instead?

ogizanagi · 2021-10-30T10:14:17Z

src/Symfony/Component/Serializer/Debug/SerializerActionFactory.php

+
+    public function createNormalization(NormalizerInterface $normalizer, $object, $result, string $format, array $context = []): Normalization
+    {
+        return new Normalization(


Same as raised by @jderusse in #39325 (comment), I'm not convinced about the need of an object to store these information for this specific use-case (yet I usually agree about not using arrays for structured data).

Also related to my comment about the factory interface, this factory is almost pure internal details of the traceable normalizers and serializer, to share some methods, so are these objects. The factory could be internal and instantiated from the traceable objects, or refactored as a trait.

if we'd like to keep these objects, I think at least:

the factory interface should be removed

the factory class and these objects must be @internal

the factory can be instantiated from the traceable instances directly

WDYT?

ogizanagi

In an API listing endpoint, the number of normalization calls can quickly grow and exposing each of these calls might not really be relevant nor handy for the developper.

I wonder if we shouldn't target something more concise, like only showing the root method calls to the serializer's serialize, deserialize, normalize and denormalize methods, instead of decorating each normalizers/denormalizers.

I know this would defeat some use-cases of this profiler panel, but debugging each (de)normalizers output might not be the role of this panel.

We could also add the context in which was made this call (source file and line N°), as in the validation panel:

The table output might not be the best to read neither:

Perhaps reversing the row/columns with a unique table per call would be better (as done for the Messenger panel).

ogizanagi · 2021-11-02T14:28:34Z

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

+                            <td class="nowrap">{{ normalization.format }}</td>
+                            <td class="nowrap">{{ normalization.normalizer | abbr_class }}</td>
+                            <td>{{ profiler_dump(normalization.data, maxDepth=2) }}</td>
+                            <td>{{ profiler_dump(normalization.result, maxDepth=2) }}</td>


In case the normalization result is a string, it'll fail with:

Argument 2 passed to Symfony\Bundle\WebProfilerBundle\Twig\WebProfilerExtension::dumpData() must be an instance of Symfony\Component\VarDumper\Cloner\Data, string given

The trick is to use seek for such cases:

Suggested change

<td>{{ profiler_dump(normalization.result, maxDepth=2) }}</td>

<td>{{ profiler_dump(normalization.seek('result'), maxDepth=2) }}</td>

ogizanagi · 2021-11-02T14:29:40Z

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

+                            <td class="font-normal text-small text-muted nowrap">{{ loop.index }}</td>
+                            <td class="nowrap">{{ normalization.format }}</td>
+                            <td class="nowrap">{{ normalization.normalizer | abbr_class }}</td>
+                            <td>{{ profiler_dump(normalization.data, maxDepth=2) }}</td>


Since the normalizer input is usually an object, but the interface accepts mixed, it could be a string as well, so same here, use seek

ogizanagi · 2021-11-02T14:31:15Z

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

+                            <td class="nowrap">{{ denormalization.denormalizer | abbr_class }}</td>
+                            <td class="nowrap">{{ denormalization.type | abbr_class }}</td>
+                            <td>{{ profiler_dump(denormalization.data, maxDepth=2) }}</td>
+                            <td>{{ profiler_dump(denormalization.result, maxDepth=2) }}</td>


Same for denormalization data and result, use seek

ogizanagi · 2021-11-02T14:48:56Z

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

+                            <td class="font-normal text-small text-muted nowrap">{{ loop.index }}</td>
+                            <td class="nowrap">{{ serialization.format }}</td>
+                            <td>{{ profiler_dump(serialization.data, maxDepth=2) }}</td>
+                            <td>{{ serialization.result }}</td>


Serialization output is always a string, so not using profiler_dump is okay, but using it can streamline the rendering in the profiler, and ellipse too long outputs:

Suggested change

<td>{{ serialization.result }}</td>

<td>{{ profiler_dump(serialization.seek('result')) }}</td>

(should probably add a word-break: break-word; at least)

vs

don't know if we want it or not.

ogizanagi · 2021-11-02T14:49:18Z

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

+                            <td class="font-normal text-small text-muted nowrap">{{ loop.index }}</td>
+                            <td class="nowrap">{{ deserialization.format }}</td>
+                            <td class="nowrap">{{ deserialization.type | abbr_class }}</td>
+                            <td>{{ deserialization.data }}</td>


ogizanagi · 2021-11-02T14:49:58Z

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

+                            <td class="nowrap">{{ deserialization.format }}</td>
+                            <td class="nowrap">{{ deserialization.type | abbr_class }}</td>
+                            <td>{{ deserialization.data }}</td>
+                            <td>{{ profiler_dump(deserialization.result, maxDepth=2) }}</td>


Deserialization result might be a string in some cases, so let's use seek here too

ogizanagi · 2021-11-02T14:53:51Z

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

+
+    <footer>
+        <small class="text-muted">
+            The icon is taken from <a href="https://fontawesome.com/icons/exchange-alt?style=solid">fontawesome</a>


We'll probably add our own icon, once this PR is ready

ogizanagi · 2021-11-02T15:30:08Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/serializer_debug.php

+return static function (ContainerConfigurator $container) {
+    $container->services()
+        ->set('debug.serializer', TraceableSerializer::class)
+            ->decorate('serializer', null, 255)


The Serializer class also implements normalizers and encoders interfaces, hence the places in userland were the @serializer service was injected and using encode, decode, normalize or denormalize will fail with:

Attempted to call an undefined method named "decode" of class "Symfony\Component\Serializer\Debug\TraceableSerializer".

and the DI aliases on the respective interfaces won't work anymore.

So decorating this service directly might not be a good idea?

We could decorate each alias instead, but then no data would be collected where the @serializer service itself is injected.
So, should we have a traceable serializer implementing all of these interfaces? 🤔

ogizanagi · 2021-11-02T15:36:47Z

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

+{% endblock %}
+
+{% block menu %}
+    {% set disable = collector.serializations | length == 0 and collector.deserializations | length == 0 %}


Even if there is no serialization/deserialization, there could be some normalize and denormalize calls that would be caught by this panel and should make it active.

ogizanagi · 2021-11-02T15:55:07Z

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

+        </div>
+        <div class="metric">
+            <span class="value">{{ collector.denormalizations | length }}</span>
+            <span class="label">Denormalizations</span>


We already have these numbers in the tabs below, no need to duplicate here I guess

chalasr · 2022-03-22T09:31:07Z

Closing in favor of #45656 as it's up to date and it addresses the main issues reported here. Thank you for the PR.

Basster marked this pull request as ready for review December 5, 2020 12:32

Basster requested a review from dunglas as a code owner December 5, 2020 12:32

carsonbot added Status: Needs Review Serializer Feature labels Dec 5, 2020

derrabus reviewed Dec 5, 2020

View reviewed changes

src/Symfony/Component/Serializer/Debug/Normalizer/AbstractTraceableNormalizer.php Outdated Show resolved Hide resolved

derrabus requested changes Dec 5, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 5, 2020

derrabus requested changes Dec 5, 2020

View reviewed changes

jderusse reviewed Dec 5, 2020

View reviewed changes

jderusse added this to the 5.x milestone Dec 6, 2020

Basster force-pushed the 39102-serializer-collector branch from 19a4e9b to fd5f1e8 Compare December 21, 2020 12:45

Basster requested a review from derrabus December 21, 2020 13:08

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 21, 2020

Basster added 9 commits March 6, 2021 13:24

symfony#39102: created TraceableSerializer and some value objects aro…

1c730b9

…und it.

symfony#39102: migrated all the collector stuff from my PoC.

27ce406

symfony#39102: thank you fabbot.

c9e2961

symfony#39102: thank you again fabbot.

b896d64

symfony#39102: register the SerializerDebugPass and configured decora…

7368cfb

…tor and data_collector for the container.

symfony#39102: handle cachable normalizers as well.

a9ab284

* and some code improvements.

symfony#39102: fabbot again.

52ed5d0

symfony#39102: fabbot again and again.

e5afaff

symfony#39102: only run SerializerDebugPass when serializer is present.

532235c

Basster added 8 commits March 6, 2021 13:24

symfony#39102: added debug.normalizer tag to UnusedTagsPass.

d7539d4

symfony#39102: c'mon fabbot...

92e49b1

symfony#39102: register configuration on kernel.debug and use correct…

9038316

… icon on serializer.html.twig.

symfony#39102: fix code style suggestions and suggested changes, first.

c9c453e

symfony#39102: implement __call on AbstractTraceableNormalizer, to ke…

0c5a2d8

…ep custom methods on custom normalizers working, which are not part of the interface.

symfony#39102: avoid storing huge (de)serializing objects/strings in …

2c815cc

…memory by replacing too large source-objects/results as replacement (LargeContent).

symfony#39102: thank you fabbot.

6992af5

symfony#39102: register missing services/aliases for SerializerDebugP…

f861ffe

…assTest.

Basster force-pushed the 39102-serializer-collector branch from dae8641 to f861ffe Compare March 6, 2021 12:24

Basster added 2 commits March 6, 2021 13:30

symfony#39102: fixed new fabbot issues.

6de10f0

symfony#39102: another fabbot fix.

18b55e6

ogizanagi reviewed Oct 30, 2021

View reviewed changes

fabpot modified the milestones: 5.4, 6.1 Oct 30, 2021

ogizanagi reviewed Nov 2, 2021

View reviewed changes

chalasr closed this Mar 22, 2022

	throw new \LogicException('Unexpected method call.');
	throw new \BadMethodCallException(sprintf('Unexpected "%s" method call.', $name));


		public function setSerializer(SerializerInterface $serializer): void

	<td>{{ profiler_dump(normalization.result, maxDepth=2) }}</td>
	<td>{{ profiler_dump(normalization.seek('result'), maxDepth=2) }}</td>

	<td>{{ serialization.result }}</td>
	<td>{{ profiler_dump(serialization.seek('result')) }}</td>

Uh oh!

[Serializer] add WebDebugToolbar collector for normalize/denormalize actions inside the serializer. #39325

[Serializer] add WebDebugToolbar collector for normalize/denormalize actions inside the serializer. #39325

Uh oh!

Conversation

Basster commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How it works?

Uh oh!

carsonbot commented Dec 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Basster commented Dec 5, 2020

Uh oh!

derrabus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jderusse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Basster commented Dec 7, 2020

Uh oh!

Basster commented Dec 21, 2020

Uh oh!

Basster commented Dec 21, 2020

Uh oh!

Basster commented Mar 6, 2021

Uh oh!

ogizanagi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Basster commented Dec 5, 2020 •

edited

Loading