Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger][Profiler] Trace middleware execution#27321
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| privatefunctionregisterBusMiddleware(ContainerBuilder$container,string$busId,array$middlewareCollection) | ||
| { | ||
| $debug =$container->getParameter('kernel.debug') &&class_exists(Stopwatch::class) &&$container->has($this->debugStopwatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't believe you need to check for theStopwatch class here, let it "normally" fail if the developer replaces the stopwatch service with something else :)
| $eventName .=" (bus:{$this->busName})"; | ||
| } | ||
| $this->stopwatch->start($eventName,$category ='messenger.middleware'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd put the$category in the constructor's argument (with this value as default obsly)
| /** | ||
| * @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
| */ | ||
| class TraceableMiddlewareTestextends TestCase |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
ogizanagi commentedJun 30, 2018
@sroze : Thanks for the review. I'll try to update this soon 👍 |
ogizanagi commentedAug 21, 2018
PR updated. ("soon" wasn't really reliable here, sorry ^^') |
| } | ||
| if ($debug) { | ||
| $container->register($debugMiddlewareId ="debug.traced.$messengerMiddlewareId", TraceableMiddleware::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would usemessenger.debug.traced. as prefix, to ensure that we don't get conflicts with other components wanting to do debug things in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and we might want to make it hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here,'debug.traced.'.$messengerMiddlewareId would be more coherent with the rest of the codebase
| // Decorates with a high priority so it's applied the earliest: | ||
| ->setDecoratedService($messengerMiddlewareId,null,100) | ||
| ->setArguments(array( | ||
| newReference("$debugMiddlewareId.inner"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use concatenation rather than interpolation, according to our existing codebase
| * @return Envelope|object The original message or the envelope if the target supports it | ||
| * (i.e implements {@link EnvelopeAwareInterface}). | ||
| */ | ||
| publicfunctiongetMessageFor($target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO, this should rather be a private method in TraceableMiddleware instead of exposing this method here publicly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Happy either way. For the records, we had this method at the early stage of theEnvelope proposal, because it was convenient to be able to deal with either the message or the "enveloped" message in a similar fashion.
| */ | ||
| publicfunctionhandle($message,callable$next) | ||
| { | ||
| $eventName =\get_class($this->inner); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should add support for anonymous classes, as done in#28218
| newReference($this->debugStopwatch), | ||
| // If definition isn't abstract, we can't be sure the service instance | ||
| // is only used in one bus at the same time, so we don't inject the bus name in this case: | ||
| $isAbstract ?$busId :null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the implementation seems to do exactly the opposite of the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If definition isn't abstract [...] we don't inject the bus name
Feels like it is to me. But perhaps can be reworded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but here, you do inject itonly when it is abstract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good here. If the middlewaredefinition is abstract (henceisDefinitionAbstract as a variable would be clearer) it means we will create a service for each bus. So it means we are sure to have only this middleware for this bus.
| 'event_listener':'#00B8F5', | ||
| 'template':'#66CC00', | ||
| 'doctrine':'#FF6633', | ||
| 'controller.argument_value_resolver':'#8c5de6', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
that one should belong to a different PR IMO, as it is not about Messenger
| { | ||
| $method =\get_class($this->inner).'::'.__FUNCTION__; | ||
| $this->stopwatch->start($method); | ||
| $this->stopwatch->start($method,'controller.argument_value_resolver'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same comment as@stof on the color scheme above: great idea but I'd move it to another PR :)
| private$busTag; | ||
| private$senderTag; | ||
| private$receiverTag; | ||
| private$debugStopwatch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$debugStopwatchId?
| } | ||
| if ($debug) { | ||
| $container->register($debugMiddlewareId ="debug.traced.$messengerMiddlewareId", TraceableMiddleware::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same here,'debug.traced.'.$messengerMiddlewareId would be more coherent with the rest of the codebase
| } | ||
| if (($definition =$container->findDefinition($messengerMiddlewareId))->isAbstract()) { | ||
| if ($isAbstract =($definition =$container->findDefinition($messengerMiddlewareId))->isAbstract()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$isDefinitionAbstract would be clearer.
| newReference($this->debugStopwatch), | ||
| // If definition isn't abstract, we can't be sure the service instance | ||
| // is only used in one bus at the same time, so we don't inject the bus name in this case: | ||
| $isAbstract ?$busId :null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good here. If the middlewaredefinition is abstract (henceisDefinitionAbstract as a variable would be clearer) it means we will create a service for each bus. So it means we are sure to have only this middleware for this bus.
| * @return Envelope|object The original message or the envelope if the target supports it | ||
| * (i.e implements {@link EnvelopeAwareInterface}). | ||
| */ | ||
| publicfunctiongetMessageFor($target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Happy either way. For the records, we had this method at the early stage of theEnvelope proposal, because it was convenient to be able to deal with either the message or the "enveloped" message in a similar fashion.
fabpot commentedSep 4, 2018
@ogizanagi Do you have time to finish this PR? |
ogizanagi commentedSep 7, 2018
chalasr left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fine using FQCNs to me, should be easy to reconsider if needed.
| * | ||
| * @author Maxime Steinhausser <maxime.steinhausser@gmail.com> | ||
| * | ||
| * @experimental in 4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can be removed I guess
… in performances panel (ogizanagi)This PR was merged into the 4.2-dev branch.Discussion----------[HttpKernel][Profiler] Add arg value resolver category in performances panel| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | part of#27262 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AExtracted from#27321<img width="1071" alt="screenshot 2018-05-20 a 12 23 55" src="https://user-images.githubusercontent.com/2211145/40278071-98c04924-5c2a-11e8-9770-d78ac62d2c16.PNG">Commits-------b24e054 [HttpKernel][Profiler] Add arg value resolver category in performances panel
sroze left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(rebase needed though)
fabpot commentedSep 8, 2018
Thank you@ogizanagi. |
…anagi)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger][Profiler] Trace middleware execution| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | part of#27262 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AThis is a start for#27262 with:- traceable Messenger middlewares- ~~a dedicated category for http kernel controller args resolvers~~ => See#28387<img width="1071" alt="screenshot 2018-05-20 a 12 23 55" src="https://user-images.githubusercontent.com/2211145/40278071-98c04924-5c2a-11e8-9770-d78ac62d2c16.PNG">Messenger middleware are traced, with bus info (if not shared accros buses):<img width="1069" alt="screenshot 2018-05-20 a 12 28 15" src="https://user-images.githubusercontent.com/2211145/40278073-9e6979f4-5c2a-11e8-9657-ee3aa057a5be.PNG">Another possibility is to use the middleware id instead of the class (with or without extra bus info?):<img width="1074" alt="screenshot 2018-05-20 a 12 32 24" src="https://user-images.githubusercontent.com/2211145/40278074-9e85f43a-5c2a-11e8-9f13-ad41de342079.PNG">(_of course, collected times are faked here using `usleep` in the traceable middleware_)Commits-------e974f67 [Messenger][Profiler] Trace middleware execution
Uh oh!
There was an error while loading.Please reload this page.
This is a start for#27262 with:
a dedicated category for http kernel controller args resolvers=> See[HttpKernel][Profiler] Add arg value resolver category in performances panel #28387Messenger middleware are traced, with bus info (if not shared accros buses):
Another possibility is to use the middleware id instead of the class (with or without extra bus info?):
(of course, collected times are faked here using
usleepin the traceable middleware)