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

Easily time functions with a closure. #43

Open
dav-m85 opened this issue Jun 29, 2015 · 4 comments
Open

Easily time functions with a closure. #43

dav-m85 opened this issue Jun 29, 2015 · 4 comments

Comments

@dav-m85
Copy link
Collaborator

dav-m85 commented Jun 29, 2015

An idea I find interesting, found on https://github.com/thephpleague/statsd:

$statsd->time('api.dbcall', function () {
    // this code execution will be timed and recorded in ms
});

Would be nice to implement here :)

@tgr
Copy link
Contributor

tgr commented Jul 23, 2015

Another nice pattern is

public function foo() {
    $scopedTimer = $statsd->scopedTimer('foo');
    // ...
}

and then the destructor of $scopedTimer will measure the time spent in the function. This can be convenient for functions with lots of exit points.

@dav-m85
Copy link
Collaborator Author

dav-m85 commented Jul 23, 2015

To delegate the destructor the job of actually sending the timing results bugs me a little. Mainly for two reasons:

  • In my mind, destructor is here to cleanup a database connection, a file descriptor, and so on. Making it open a "dirty" socket on purpose can be misleading.
  • You would have to be extra cautious to make sure destruction happens at the end of your function execution. Leave a reference somewhere and bam. Inconsistent timing.

However I can see your point here. We could imagine for example enclosing foo() execution in the code of the issue proposal:

$statsd->time('api.dbcall', function () use (&$fooReturn) {
    $fooReturn = foo(); // your foo function
});

An idea to dig into.

@tgr
Copy link
Contributor

tgr commented Jul 23, 2015

It is a hack, but a very convenient one. E.g.

public function foo($statsd) {
    $startTime = microtime();
    doStuff();
    if (someFailureCondition()) {
         $statsd->time('foo', microtime() - $startTime);
         return false;
    }
    foreach (data() as $d) {
        try {
            doSomethingDangerousWith($d);
        } catch($e) {
            $statsd->time('foo', microtime() - $startTime);
            throw $e;
        }
    }
    $statsd->time('foo', microtime() - $startTime);
}

This kind of sucks. It is a lot nicer to say (although admittedly harder to understand at first glance):

public function foo($statsd) {
    $scopedTimer = $statsd->scopedTimer('foo');
    doStuff();
    if (someFailureCondition()) {
         return false;
    }
    foreach (data() as $d) {
        doSomethingDangerousWith($d);
    }
}

MediaWiki uses this pattern quite a bit (e.g. ScopedCallback); it's strange at first but works well.

@liuggio
Copy link
Owner

liuggio commented Jul 23, 2015

@dav-m85 I like your idea please when you have time add it...
I think is better to place the flush in the deconstructor anyway with or without the @tgr idea

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

No branches or pull requests

3 participants