Skip to content
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

Add Commands/Queries Profiler tab #14198

Merged
merged 3 commits into from Jul 9, 2019

Conversation

@sarjon
Copy link
Member

commented Jun 13, 2019

Questions Answers
Branch? develop
Description? Adds Profiler tab that informs developers about executed Commands and Queries.
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #14154
How to test? Not needed, it's a dev tool. See screenshot below.

Profiler tab

Screenshot from 2019-06-13 17-38-25

Profiler page

Screenshot (3)


This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 13, 2019

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

@matks is this something you expected? Do you have any suggestions for improvements?

*
* @return string
*/
public function parse($commandName)

This comment has been minimized.

Copy link
@matks

matks Jun 14, 2019

Contributor

In the future, maybe we could find whether this is a query or command from the bus used prestashop.core.command_bus or prestashop.core.query_bus

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 14, 2019

Author Member

Maybe, but now we are using the same bus behind the scene for both.

@matks

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@matks is this something you expected? Do you have any suggestions for improvements?

I ❤️ it !

@PierreRambaud Does this PR addresses your need about CQRS being hard to navigate ? 😄

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@matks Oh yeah! ❤️

@@ -136,3 +136,10 @@ prestashop:
api_client:
ttl: 7200 # 2h
# verify_ssl: ~ # Bundle CA by default, declaring "addons.api_client.verify_ssl" parameter overrides its usage

tactician:

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Jun 14, 2019

Contributor

if this middleware only needs to be enabled in dev mode, how about put this configuration in config_dev.yml file instead? This may remove the check about the application environment in the class.

This comment has been minimized.

Copy link
@sarjon

sarjon Jun 17, 2019

Author Member

Yeah, that would make sense.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I'm wondering who is the real user/use case of introducing this?

Don't expect PrestaShop developers to expect what is CQRS and/or to figure out what/why this information is displayed in the debug toolbar...

As we have the related hooks for each command/query, for an integrator this tab has no value (in its current state), but we could keep the application (ie the page "Command/Query) in the web profiler if it helps the core team and the Symfony developers who want to play with PrestaShop using the "CQRS way" ?

Think about it: the debug toolbar is not infinite so we need to care a lot about what information should be available for the PrestaShop developers.

For example, I think we should also remove the one about Guzzle as no ones really figure out what it does and no one really cares in the community.

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Don't expect PrestaShop developers to expect what is CQRS and/or to figure out what/why this information is displayed in the debug toolbar...

This is a dev tool for PS developers, sooner or later they will have to start using it I think (and from trainings I see a very big interest in CQRS from PS devs). There is already a documentation about CQRS (not so complete, but still). Either way, it should not harm anyone having this tool around.

Think about it: the debug toolbar is not infinite so we need to care a lot about what information should be available for the PrestaShop developers.

There is already a lot of information in toolbar that is not really related to PS nor needed for PS devs. If we see that this tool is not needed (and you can only know by getting feedback from developers), then we can remove it from debug toolbar without any BC break I think.

For example, I think we should also remove the one about Guzzle as no ones really figure out what it does and no one really cares in the community.

Agree, as mentioned before, not all information in debug toolbar is now useful and could be removed. Unfortunately, it's not part of this PR to do so.

@matks

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

I'm wondering who is the real user/use case of introducing this?

Don't expect PrestaShop developers to expect what is CQRS and/or to figure out what/why this information is displayed in the debug toolbar...

We'll tell them 😄. This tool addresses a need expressed by @PierreRambaud : "I'm working on a page, I see some commands and queries are dispatched and I find it hard to trace them to the right Handler to find out where the code I need to debug/update is." This is a common issue when using events (tracing the listener from the dispatch) and even when using Symfony services. It can be solved by going through the configuration, but the aim is to make things clearer, faster, easier.

As we have the related hooks for each command/query

🤔 I dont see what you mean. Are you sure it's there ?

Think about it: the debug toolbar is not infinite so we need to care a lot about what information
should be available for the PrestaShop developers.

Indeed. But since this is a dev-only tool any mistake we do will not critical. Also I'm thinking that maybe we can work around this matter by allowing dev to toggle the features they want to see in the debug bar.

For example, I think we should also remove the one about Guzzle as no ones really figure out what it does and no one really cares in the community.

Yep, makes sense.

@matks

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@sarjon As suggested by @mickaelandrieu, a nice improvement (maybe for a future PR) would be to add profiler capabilities to this tab. Being able to see the response time of a handler and the consumed memory could be great debug assets 👍

@sarjon

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Hmm, tests are failing, could you rerun them?

@sarjon sarjon changed the title [WIP] Add Commands/Queries Profiler tab Add Commands/Queries Profiler tab Jun 18, 2019

@matks matks added the migration label Jun 24, 2019

@matks

matks approved these changes Jul 9, 2019

@matks matks added this to the 1.7.7.0 milestone Jul 9, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Thank you @sarjon ❤️

@matks matks merged commit a51cb21 into PrestaShop:develop Jul 9, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.