-
Notifications
You must be signed in to change notification settings - Fork 825
allow Histograms and Summaries to time Callables #368
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
Conversation
| } | ||
|
|
||
| /** | ||
| * Executes callable code (i.e. a Java 8 Lambda) and observes a duration of how long it took to run. |
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.
e.g.
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.
There is no example in existing time() method, but I can certainly add one here.
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 meant that i.e. is incorrect as it can be any Callable, not just a Java 8 Lambda.
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 see. Then on old method this should also be corrected, as I copied text from there.
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.
Yes, I must have missed that back when that was added.
| * @param timeable Code that is being timed | ||
| * @return Result returned by callable. | ||
| */ | ||
| public <E> E time(Callable<E> timeable) { |
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.
Gauge also has a time() method
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 can add that too.
| try { | ||
| return timeable.call(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); |
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.
Why are you wrapping this exception rather than just letting it propagate naturally?
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.
Cause in most cases there will be no checked exceptions within that Callable.
And I would not make time() method throw checked exception, as this will require try-catch around it, which makes usage in streams quite painful.
Supplier class would be much more appropriate for this method but there is only a Callable alternative in Java 6.
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.
Okay
Signed-off-by: Yaroslav Stavnichiy <yarosla@gmail.com>
|
I've fixed all the comments. Don't know how to deal with this DCO thing. |
|
The easiest thing to do is to squash the commits with rebase, and make sure the remaining commit has the signoff. |
|
I've created new PR #378 from squashed branch. |
@brian-brazil
Another usability enhancement allowing instead of:
to write:
This is especially useful when programming (reactive) streams.
Trying to make metric code as unobtrusive as possible.
BTW Micrometer already does that.