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/05/22 21:15:50 UTC
Review Request 59464: Add fetch success metric.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/
-----------------------------------------------------------
Review request for mesos and Mesos Reviewbot.
Bugs: MESOS-7524
https://issues.apache.org/jira/browse/MESOS-7524
Repository: mesos
Description
-------
Add fetch success metric.
Diffs
-----
src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043
src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611
Diff: https://reviews.apache.org/r/59464/diff/1/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59464: Add Fetcher total and success metrics.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/#review176340
-----------------------------------------------------------
It looks like all of your patches are getting marked with 'mesos-review' as the reviewer?
- Benjamin Mahler
On May 22, 2017, 9:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59464/
> -----------------------------------------------------------
>
> (Updated May 22, 2017, 9:15 p.m.)
>
>
> Review request for mesos and Mesos Reviewbot.
>
>
> Bugs: MESOS-7524
> https://issues.apache.org/jira/browse/MESOS-7524
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add the Fetcher metrics to track the number of fetch requests sent to
> the Fetcher (`containerizer/fetcher/total`) and the number of errors
> reported by the Fetcher (`containerizer/fetcher/errors`).
>
>
> Diffs
> -----
>
> src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043
> src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611
>
>
> Diff: https://reviews.apache.org/r/59464/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59464: Add Fetcher task total and failed fetch
metrics.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/#review178543
-----------------------------------------------------------
Ship it!
LGTM. I can add the comment mentioned below before committing.
src/slave/containerizer/fetcher.hpp
Lines 327-328 (patched)
<https://reviews.apache.org/r/59464/#comment252597>
It's important enough to add a comment here about the counter resolution. This number will go up once per task, rather than once per artifact.
So if 1 task asks for 10 artifacts, the total number of fetches is 1. If 1 to 10 of those artifacts fail to fetch, the total number of failures is also 1.
src/slave/containerizer/fetcher.cpp
Lines 68-78 (original), 70-78 (patched)
<https://reviews.apache.org/r/59464/#comment252596>
This change isn't strictly related to the metrics, so it could be moved elsewhere.
- Joseph Wu
On June 9, 2017, 11:55 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59464/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 11:55 a.m.)
>
>
> Review request for mesos and Joseph Wu.
>
>
> Bugs: MESOS-7524
> https://issues.apache.org/jira/browse/MESOS-7524
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add the Fetcher metrics to track the number of fetch requests
> sent to the Fetcher (`containerizer/fetcher/task_fetches_total`)
> and the number of errors reported by the Fetcher
> (`containerizer/fetcher/task_fetches_failed`).
>
>
> Diffs
> -----
>
> src/slave/containerizer/fetcher.hpp efeadbf4b7804ea4c1e443d1e5212e303796ace4
> src/slave/containerizer/fetcher.cpp 770cad3e046e8a6d58b6bc9176eb7ecdbd340db4
>
>
> Diff: https://reviews.apache.org/r/59464/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59464: Add Fetcher task total and failed fetch
metrics.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/
-----------------------------------------------------------
(Updated June 9, 2017, 6:55 p.m.)
Review request for mesos and Joseph Wu.
Changes
-------
Rebased and addressed review feedback.
Bugs: MESOS-7524
https://issues.apache.org/jira/browse/MESOS-7524
Repository: mesos
Description
-------
Add the Fetcher metrics to track the number of fetch requests
sent to the Fetcher (`containerizer/fetcher/task_fetches_total`)
and the number of errors reported by the Fetcher
(`containerizer/fetcher/task_fetches_failed`).
Diffs (updated)
-----
src/slave/containerizer/fetcher.hpp efeadbf4b7804ea4c1e443d1e5212e303796ace4
src/slave/containerizer/fetcher.cpp 770cad3e046e8a6d58b6bc9176eb7ecdbd340db4
Diff: https://reviews.apache.org/r/59464/diff/4/
Changes: https://reviews.apache.org/r/59464/diff/3-4/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59464: Add Fetcher task total and failed fetch
metrics.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/#review177428
-----------------------------------------------------------
src/slave/containerizer/fetcher.hpp
Lines 113-114 (patched)
<https://reviews.apache.org/r/59464/#comment250972>
Normally, metrics live in the libprocess `Process` rather than the wrapper.
Doing it this way will also make incrementing the counters a bit more intuitive (since you'll be incrementing success/failure right after it happens, rather than when a future returns).
src/slave/containerizer/fetcher.cpp
Lines 71 (patched)
<https://reviews.apache.org/r/59464/#comment250968>
A metrics prefix only makes sense when we're creating multiple processes. In case of the fetcher, there's only ever one of them, so it doesn't need a prefix.
- Joseph Wu
On June 6, 2017, 1:21 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59464/
> -----------------------------------------------------------
>
> (Updated June 6, 2017, 1:21 p.m.)
>
>
> Review request for mesos and Mesos Reviewbot.
>
>
> Bugs: MESOS-7524
> https://issues.apache.org/jira/browse/MESOS-7524
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add the Fetcher metrics to track the number of fetch requests
> sent to the Fetcher (`containerizer/fetcher/task_fetches_total`)
> and the number of errors reported by the Fetcher
> (`containerizer/fetcher/task_fetches_failed`).
>
>
> Diffs
> -----
>
> src/slave/containerizer/fetcher.hpp efeadbf4b7804ea4c1e443d1e5212e303796ace4
> src/slave/containerizer/fetcher.cpp 770cad3e046e8a6d58b6bc9176eb7ecdbd340db4
>
>
> Diff: https://reviews.apache.org/r/59464/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59464: Add Fetcher task total and failed fetch
metrics.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/
-----------------------------------------------------------
(Updated June 6, 2017, 8:21 p.m.)
Review request for mesos and Mesos Reviewbot.
Summary (updated)
-----------------
Add Fetcher task total and failed fetch metrics.
Bugs: MESOS-7524
https://issues.apache.org/jira/browse/MESOS-7524
Repository: mesos
Description (updated)
-------
Add the Fetcher metrics to track the number of fetch requests
sent to the Fetcher (`containerizer/fetcher/task_fetches_total`)
and the number of errors reported by the Fetcher
(`containerizer/fetcher/task_fetches_failed`).
Diffs (updated)
-----
src/slave/containerizer/fetcher.hpp efeadbf4b7804ea4c1e443d1e5212e303796ace4
src/slave/containerizer/fetcher.cpp 770cad3e046e8a6d58b6bc9176eb7ecdbd340db4
Diff: https://reviews.apache.org/r/59464/diff/3/
Changes: https://reviews.apache.org/r/59464/diff/2-3/
Testing
-------
make check (Fedora 25)
Thanks,
James Peach
Re: Review Request 59464: Add Fetcher total and success metrics.
Posted by James Peach <jp...@apache.org>.
> On May 23, 2017, 1:04 a.m., Joseph Wu wrote:
> > What do you think about making fetch failures a sub-metric of `container_launch_errors`? The fetcher is only called during container launch. If fetching fails, container launch will also fail (and increment this counter). It might be useful to separate out fetch failures from other types of launch failures. This approach might be a bit more work to do so, since the parent metric lives inside the Agent, rather than the Fetcher process.
I like the idea of more specific launch errors, but that's not equivalent to what this metric gives. This lets you calculate the number of tasks that fetch assets as well as the proportion of asset fetches that fail. In the future I'd like to add more metrics under `containerizer/fetcher`; we could track disk usage, URI scheme, total URI fetch failures, cache hit and miss, etc. That's more complicated, so I'm proposing this to get something simple but useful.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/#review175740
-----------------------------------------------------------
On May 22, 2017, 9:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59464/
> -----------------------------------------------------------
>
> (Updated May 22, 2017, 9:15 p.m.)
>
>
> Review request for mesos and Mesos Reviewbot.
>
>
> Bugs: MESOS-7524
> https://issues.apache.org/jira/browse/MESOS-7524
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add the Fetcher metrics to track the number of fetch requests sent to
> the Fetcher (`containerizer/fetcher/total`) and the number of errors
> reported by the Fetcher (`containerizer/fetcher/errors`).
>
>
> Diffs
> -----
>
> src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043
> src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611
>
>
> Diff: https://reviews.apache.org/r/59464/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 59464: Add Fetcher total and success metrics.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59464/#review175740
-----------------------------------------------------------
What do you think about making fetch failures a sub-metric of `container_launch_errors`? The fetcher is only called during container launch. If fetching fails, container launch will also fail (and increment this counter). It might be useful to separate out fetch failures from other types of launch failures. This approach might be a bit more work to do so, since the parent metric lives inside the Agent, rather than the Fetcher process.
- Joseph Wu
On May 22, 2017, 2:15 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59464/
> -----------------------------------------------------------
>
> (Updated May 22, 2017, 2:15 p.m.)
>
>
> Review request for mesos and Mesos Reviewbot.
>
>
> Bugs: MESOS-7524
> https://issues.apache.org/jira/browse/MESOS-7524
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add the Fetcher metrics to track the number of fetch requests sent to
> the Fetcher (`containerizer/fetcher/total`) and the number of errors
> reported by the Fetcher (`containerizer/fetcher/errors`).
>
>
> Diffs
> -----
>
> src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043
> src/slave/containerizer/fetcher.cpp a910fea5a5556afb376524c5bb2ff98d7d84e611
>
>
> Diff: https://reviews.apache.org/r/59464/diff/2/
>
>
> Testing
> -------
>
> make check (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>