Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Serializer] Add serializer profiler #45656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Mar 6, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

By design, it's quite hard to find out what happens when serializing data using the serializer.
Indeed, it's hard to discover which normalizer and which encoder was used due to the "kind of chain of responsibility pattern".

This PR adds a way to trace what happens during the serialization process (and how long each step took).

Screenshot from 2022-03-06 11-51-17
Screenshot from 2022-03-06 11-51-40

To do that, the serializer service is decorated to trace serialize, deserialize, normalize, denormalize, encode and decode methods calls.
Each normalizer and encoder is decorated as well so that it can fill the trace when called.
Finally, the SerializerDataCollector is collecting data during the whole process and aggregating it once as the end to make it available to the profiler.

Please note that tracing is only available when calling the serializer service.
When calling a normalizer/encoder directly, nothing will be traced (which is fair enough IMHO as most of the users are serializing data using the serializer service).

At the moment, I reused the validator icon because I haven't created one specifically for the serializer.
I asked somebody to create that icon for me, but maybe there is someone used to it that wanna take care of it?

Since Symfony 6.1 will require PHP 8.1 as a minimum dependency, I used constructor property promotion.
But I'm not sure that's the coding standard that Symfony wanna use in the future, so I can use regular constructor property assignment if it's preferred.

@carsonbot
Copy link

Hey!

I think @alexandre-daubois has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

src/Symfony/Component/Serializer/TraceableSerializer.php Outdated Show resolved Hide resolved
@mtarld mtarld force-pushed the feat/serializer-profiler branch from a57cce3 to 0e20ad6 Compare April 1, 2022 12:19
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, worked well so far. Nice one!


{% block menu %}
<span class="label {{ not collector.handledCount ? 'disabled' }}">
<span class="icon">{{ include('@WebProfiler/Icon/validator.svg') }}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Icon needs to be updated. We are working on updating all icons soon, so let's not block this PR for now.

@fabpot
Copy link
Member

fabpot commented Apr 1, 2022

Thank you @mtarld.

@fabpot fabpot merged commit f3ec7a0 into symfony:6.1 Apr 1, 2022
@fabpot fabpot mentioned this pull request Apr 15, 2022
fabpot added a commit that referenced this pull request May 4, 2022
…ofiler panel (javiereguiluz)

This PR was merged into the 6.1 branch.

Discussion
----------

[WebProfilerBundle] Update the icon of the Serializer profiler panel

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

In the PR that added the Serializer profiler panel, the new icon was left as a pending thing. See #45656 (comment)

This PR adds the new icon and it looks like this:

<img width="264" alt="icon-serializer" src="https://user-images.githubusercontent.com/73419/166722845-a9852079-1436-44a9-9421-399e2e9d312e.png">

Two comments:

(1) The icon style doesn't 100% match the style of the other icons. Don't worry about that. We plan to update all icons for Symfony 6.2 and they all will use that style.

(2) I know the icon might not be super clear and I'm open to better suggestions for this icon ... but it seems that it's impossible to conceptualize the "serializer" concept in an icon. See the biggest icon libraries in the world:

* Iconfinder: it has nothing about that: https://www.iconfinder.com/search?q=serializer
* The Noun Project: it has an icon of a triangle. It looks totally random to me: https://thenounproject.com/search/icons/?iconspage=1&q=serializer

So, I did this: I looked for "serializer" in Google Images (https://www.google.com/search?q=serializer&tbm=isch) and found that most images are block diagrams with lots of horizontal arrows. That's why I opted for the "two horizontal arrows" icon.

Commits
-------

8bfb80b [WebProfilerBundle] Update the icon of the Serializer profiler panel
@mtarld mtarld deleted the feat/serializer-profiler branch May 17, 2022 12:20
Guikingone added a commit to Guikingone/SchedulerBundle that referenced this pull request May 26, 2022
Guikingone added a commit to Guikingone/SchedulerBundle that referenced this pull request May 31, 2022
Guikingone added a commit to Guikingone/SchedulerBundle that referenced this pull request Jun 1, 2022
…ormalizer added (#260)

* refactor(serializer): support for TraceableNormalizer && TraceableDenormalizer added

See symfony/symfony#45656

* refactor(core): serializer dependencies improved

* refactor(core): improvements on static analysis

* refactor(serializer): improvements

* refactor(serializer): TaskNormalizer fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.