You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/07/25 23:26:49 UTC
Review Request 61124: Push metric history entries with their
timestamp.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61124/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
Bugs: MESOS-6918
https://issues.apache.org/jira/browse/MESOS-6918
Repository: mesos
Description
-------
In the Timer metric, we already have a current timestamp when
we push the sample into the history buffer. Added an optional
timestamp to Metric::push() so that we can avoid unnecessary
overhead of calling Clock::now() in the common case of Timer
metrics.
Diffs
-----
3rdparty/libprocess/include/process/metrics/metric.hpp 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721
3rdparty/libprocess/include/process/metrics/timer.hpp 0a9c0227c457c6c81a59f65f901a5464ee00983d
Diff: https://reviews.apache.org/r/61124/diff/1/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 61124: Push metric history entries with their
timestamp.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61124/#review183412
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On July 25, 2017, 4:26 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 4:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/metric.hpp 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721
> 3rdparty/libprocess/include/process/metrics/timer.hpp 0a9c0227c457c6c81a59f65f901a5464ee00983d
>
>
> Diff: https://reviews.apache.org/r/61124/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 61124: Push metric history entries with their
timestamp.
Posted by Benjamin Mahler <bm...@apache.org>.
> On July 31, 2017, 8:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metric.hpp
> > Line 63 (original), 63 (patched)
> > <https://reviews.apache.org/r/61124/diff/1/?file=1782382#file1782382line63>
> >
> > Can this just be:
> >
> > ```
> > void push(double value, Time now = Clock::now())
> > ```
> >
> > ?
>
> James Peach wrote:
> The common case for metrics is that there is no time series, in which case we don't need the current timestamp. If I add `Clock::now` as a default parameter we would be unnecessarily sampleing the clock (hitting a global lock) on every metrics update.
Ah whoops!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61124/#review181830
-----------------------------------------------------------
On July 25, 2017, 11:26 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 11:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/metric.hpp 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721
> 3rdparty/libprocess/include/process/metrics/timer.hpp 0a9c0227c457c6c81a59f65f901a5464ee00983d
>
>
> Diff: https://reviews.apache.org/r/61124/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 61124: Push metric history entries with their
timestamp.
Posted by James Peach <jp...@apache.org>.
> On July 31, 2017, 8:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metric.hpp
> > Line 63 (original), 63 (patched)
> > <https://reviews.apache.org/r/61124/diff/1/?file=1782382#file1782382line63>
> >
> > Can this just be:
> >
> > ```
> > void push(double value, Time now = Clock::now())
> > ```
> >
> > ?
The common case for metrics is that there is no time series, in which case we don't need the current timestamp. If I add `Clock::now` as a default parameter we would be unnecessarily sampleing the clock (hitting a global lock) on every metrics update.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61124/#review181830
-----------------------------------------------------------
On July 25, 2017, 11:26 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 11:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/metric.hpp 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721
> 3rdparty/libprocess/include/process/metrics/timer.hpp 0a9c0227c457c6c81a59f65f901a5464ee00983d
>
>
> Diff: https://reviews.apache.org/r/61124/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 61124: Push metric history entries with their
timestamp.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61124/#review181830
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/include/process/metrics/metric.hpp
Line 63 (original), 63 (patched)
<https://reviews.apache.org/r/61124/#comment257594>
Can this just be:
```
void push(double value, Time now = Clock::now())
```
?
- Benjamin Mahler
On July 25, 2017, 11:26 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 11:26 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
>
>
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/metric.hpp 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721
> 3rdparty/libprocess/include/process/metrics/timer.hpp 0a9c0227c457c6c81a59f65f901a5464ee00983d
>
>
> Diff: https://reviews.apache.org/r/61124/diff/1/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>