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

Support Server Timing Spec #125

Closed
Garbee opened this Issue Mar 15, 2016 · 9 comments

Comments

Projects
None yet
3 participants
@Garbee
Contributor

Garbee commented Mar 15, 2016

It would be a handy addition to add Server Timing support to Clockwork. This would allow a quick glance at timing information to be provided by the DevTools while still offering the full details over in the Clockwork panel.

Server timing is implemented as additional headers on responses. So we'd do something like an after middleware for Laravel. Adding the headers computed from whatever data was collected as the application ran, compressed into singular entities to keep things small. i.e. Do one "Database" heading with the full time spent in the DB. Not 50 individual calls for each query made.

@itsgoingd

This comment has been minimized.

Owner

itsgoingd commented Mar 16, 2016

Hey, this looks pretty cool, though it doesn't seem to be supported by Chrome dev tools yet. At least I couldn't find any mention in the docs and adding the header doesn't seem to do anything.

@Garbee

This comment has been minimized.

Contributor

Garbee commented Mar 17, 2016

Not yet, code is still in the review process fit an initial implementation.
Should land soon. Still nice to get ahead for when it lands.

On Wed, Mar 16, 2016, 7:16 PM its notifications@github.com wrote:

Hey, this looks pretty cool, though it doesn't seem to be supported by
Chrome dev tools yet. At least I couldn't find any mentions in the docs and
adding the header doesn't seem to do anything.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#125 (comment)

@Garbee

This comment has been minimized.

Contributor

Garbee commented Mar 21, 2016

FYI if you want to follow the implementation discussion the bug is 594800. Looks like we are making some good headway now.

@itsgoingd

This comment has been minimized.

Owner

itsgoingd commented May 18, 2016

Thanks, I'll keep an eye on the bug and get implementation done in master for testing.

It's pretty cool idea, but I'm not yet sure if it really proves useful as it's also pretty limited, eg. you can't specify "start time" for the measurement so you can't build a real "timeline". Will decide to keep it or get rid of it before Clockwork 2.0 release based on real usage experience.

@Garbee

This comment has been minimized.

Contributor

Garbee commented May 18, 2016

Yea, it isn't for building a full timeline. That is where clockwork is still a great tool to have in your kit. This would however provide overview "total times" of where time is spent.

This isn't replacing any current functionality nor does it mean to take something away. It adds to the existing functionality provided to offer developers a quick overview of their application in the toolset they are already using.

i.e. You get a quick view in the Network Panel (once it lands) saying 4 seconds was spent total collecting information from the database. While 300 milliseconds were spent say getting items from a cache. And total app runtime was 6 seconds. You'd know pretty clearly that you want to dig into the Database calls more, so you'd hop into Clockwork itself and see the actual logistics of the request details.

@ItsEcholot

This comment has been minimized.

ItsEcholot commented Feb 12, 2017

This is now supported by chrome (as seen here).
Just as a heads up.

@itsgoingd

This comment has been minimized.

Owner

itsgoingd commented Feb 18, 2017

Hey, this is now implemented in the v1 branch (require v1.x-dev in composer). Not yet sure if I want to make this configurable or always on. Also it takes only the first 20 timeline events atm to avoid sending large headers. Try it out and let me know what you think!

@Garbee

This comment has been minimized.

Contributor

Garbee commented Feb 18, 2017

I'll give it a whirl in a few days while I'm working on some new features. I think making the number of events to collect configurable would be nice and having a lower default. Like say 10 events. That seems like a nice number to have as the default since even 20 seems a bit excessive for most. And if you do need more, it's just a config away.

Other than that once piece, I'm not sure what should be configurable. Things are pretty opaque on the server-side.

@Garbee

This comment has been minimized.

Contributor

Garbee commented Apr 4, 2017

Just got around to testing this out today. +1 from me on implementation. Works as expected!

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment