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

NEW Add new execmetric debur URL parameter to print out exection time and peak memory usage #8733

Merged

Conversation

maxime-rainville
Copy link
Contributor

This is a quick something I put together to help debug #8664

Basically, if you add execmetrics as a URL Get parameter, you'll get a print out of how long the request took and the peak memory usage.

Figure it might be helpful going forward.

@maxime-rainville maxime-rainville force-pushed the pulls/4/add-exectmetric-url-debug-param branch from 2ab31b8 to 70a38a0 Compare January 17, 2019 03:31
Copy link
Contributor

@bergice bergice left a comment

Choose a reason for hiding this comment

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

Looks okay, but we probably want to be careful changing HTTPApplication. Maybe get someone elses opinion on this before we merge.

src/Control/HTTPApplication.php Outdated Show resolved Hide resolved
src/Control/HTTPApplication.php Outdated Show resolved Hide resolved
Copy link
Contributor

@ScopeyNZ ScopeyNZ left a comment

Choose a reason for hiding this comment

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

I feel like this would be better suited as a middleware - just one at the top of the chain - first in / last out.

@maxime-rainville maxime-rainville force-pushed the pulls/4/add-exectmetric-url-debug-param branch from 66709ef to c4bf06f Compare January 29, 2019 04:29
@maxime-rainville
Copy link
Contributor Author

I witch to using middlewares as suggested by @ScopeyNZ. Seems to be working pretty well.

A minor concern is the order in which the middle ware get added will potentially impact the actual memory usage. There's not really a way of giving those a priority right now. Probably not a big deal, because you would hope your middlewares are pretty light weight any way.

@bergice bergice merged commit d2c8530 into silverstripe:4 Jan 29, 2019
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Sorry, some retrospective review comments.

I personally don't think this change is warranted overall. I think this could be a documentation example or a separate module rather than a part of framework.

* Get the peak memory usage formatted has a string and a meaningful unit.
* @return string
*/
private function formatPeakMemoryUsage()
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer protected over private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those 3 methods are implementations details and are not meant to be reuse. In theory, I could have implemented all of those directly in process, but I wanted to split them out for readability. In this context, I don't think it's worthwhile increasing our public API surface.

{
/**
* @inheritdoc
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is equivalent to not having a doc block at all

use SilverStripe\Dev\Debug;

/**
* Secures requests by only allowing a whitelist of Host values
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that really what this middleware does?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh whoops that's a retrospective review. Explains why I didn't see all the options when submitting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ I copied over one of the other files. Forgot to update the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxime-rainville maxime-rainville deleted the pulls/4/add-exectmetric-url-debug-param branch January 30, 2019 01:02
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.

4 participants