Added duration header #3
Conversation
| }, function (Exception $exception) use ($eventName) { | ||
| $this->stopwatch->stop($eventName, self::CATEGORY); | ||
| $this->stopwatch->stop($eventName); |
There was a problem hiding this comment.
indeed, the category parameter seems unneeded according to https://github.com/symfony/stopwatch/blob/master/Stopwatch.php
|
|
||
| return $response; | ||
| return $response->withHeader(self::HEADER, $event->getDuration()); |
There was a problem hiding this comment.
i wonder if there is a more elegant way than modifying the response with an additional header.
There was a problem hiding this comment.
AFAIK the stopwatch is supposed to be accessed from the outside as well so you can directly get event information from it. Isn't it what we do in HttplugBundle?
There was a problem hiding this comment.
yep, same here, but i didn't find a better one. i think getting the information out of stopwatch in collector is not good option. i could introduce some kind of request registry which is used by stopwatch plugin and collector, but i think it would harden the coupling, which might not be the best idea as well. so i decided to go with a header and i think if a plugin manipulates the response with an additional header that's not such a weird behavior, but i clearly get your point. did i miss a good option to avoid the header?
There was a problem hiding this comment.
Hm, so the problem is that we cannot map a request to a specific stopwatch event, right?
Because my idea was to use the stopwatch class and get the event based on something: http://api.symfony.com/3.2/Symfony/Component/Stopwatch/Stopwatch.html#method_getEvent
Maybe we could introduce an internal Request/Response ID header and use that in the event name, but that still feels hacky.
|
did i miss a good option to avoid the header?
if i had an idea, i would have told :-)
anybody else has some input on this?
|
|
What about using decorators like I did in php-http/HttplugBundle#128? For this purpose, we explicitly excluded the header from possible tracking solutions. EDIT: after a little more reading about the details of this PR, I would suggest to add to the stopwatch Symfony component the ability to use the event dispatcher and then by listening to stopwatch events, you will be able to write the request time into the collector current stack. EDIT2: if doing so, you have to go fast with the stopwatch component as the 3.3 feature freeze less than 2 weeks away! |
|
@fbourigault but in that case we would depend on symfony 3.3+ only, right? |
|
The bundle will depends on symfony/stopwatch 3.3+. The bundle could ship a compatibility layer only used when symfony/stopwatch < 3.3 is installed. |
|
I haven't interfered to much with the bundle, but last time I checked it was >=3.0. Is it a good idea to drop support for <3.3? |
|
i don't think we can bump all the way to symfony 3.3, unless stopwatch 3.3 can be installed together with symfony/symfony 3.1. i think that does not work, as symfony/symfony would contain stopwatch 3.1 and php is no table to handle multiple versions of the same dep afaik. lets find a way that works with older versions (the header if we find nothing better) and keep this idea for a version 2 of the bundle. (would still require to work on the stopwatch pull request now to get it into symfony 3.3 so we actually can use it in version 2 of this bundle) |
|
@dbu I posted an other proposal in the bundle PR. |
|
Not needed anymore to solve php-http/HttplugBundle#135 |
What's in this PR?
Extended stopwatch plugin for bundle PR