----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18718/#review38207 -----------------------------------------------------------
Great stuff Dominic! Looking forward to moving our stats endpoints over to this!! 3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment70217> Need this? 3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment70214> What's the use case for having an explicit initial value in the constructor argument? How about we always start at 0 and see if we end up with a use-case that wants to start with a different value? 3rdparty/libprocess/include/process/metrics/counter.hpp <https://reviews.apache.org/r/18718/#comment70215> Any TODO here to replace with std::atomic? Why did you choose 'int'? I would maybe expect a signed 64 bit integer, since double can represent all integers smaller than 2^53. 3rdparty/libprocess/include/process/metrics/metric.hpp <https://reviews.apache.org/r/18718/#comment70245> Can you add a comment about why these are non-const? Here and in the Metric implementations as well. 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment70228> Looks like the pointer ownership semantics here are not safe, you're removing yourself from the metrics library in an asynchronous manner without waiting for the operation to complete. After the destructor completes, the pointer has become invalid but MetricsProcess is potentially going to read from it if an HTTP request comes in! Either we want copy-able Metrics with shared data (akin to Future / Subprocess) to avoid having to deal with lifetime, or we need to block in the destructor here. Any other options? 3rdparty/libprocess/src/metrics/metric.cpp <https://reviews.apache.org/r/18718/#comment70227> Looks like you can just bind to fatal instead of adding the wrapper function? Please place .onFailed on the subsequent line. 3rdparty/libprocess/src/metrics/metrics.hpp <https://reviews.apache.org/r/18718/#comment70207> I like the clarity of "registering" a metric and "unregistering" a metric, should we rename these to register() and unregister()? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70222> This goes above ;) 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70221> What about creating a singleton getter: MetricsProcess* MetricsProcess::instance(); Internally it would declare 'process' and 'initialized' as static variables and it could implement the one-time spawning. Then we could kill spawnProcess() and simplify the code below: Future<Nothing> remove(const string& context, const string& name) { return dispatch(MetricsProcess::instance(), &MetricsProcess::remove, context, name); } 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70230> We use 1 newline within a class, ditto below. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70223> Can you avoid using a non-const reference here? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70244> Seems clearer to understand this code without the 'Context' typedef, can we kill it? The statistics.cpp code uses the same context / name mapping and we didn't use a typedef there FWIW. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70243> The way it's designed now ("/metrics/"), we have an endpoint at the level of the libprocess name, which we've not done in the past. This means if you wanted to expose parameters off of the base URL, it would look like this: /metrics/?context=master // Query based. /metrics/master // Path based. The query based approach looks pretty strange and the path based one closes us off to being able to expose other endpoints since we're using the path at the root level. I'm curious if you're ok with this because it will close the door to exposing other metrics related endpoints, like: /metrics/timeseries /metrics/values.txt /metrics/values.json I'm not really sure what we'll want in the future but I'm a bit concerned about routing a top-level endpoint at "/" and getting bit later when we want to add some other metrics endpoints. 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70231> s/metricsHelp()/help()/? 3rdparty/libprocess/src/metrics/metrics.cpp <https://reviews.apache.org/r/18718/#comment70232> Please write this out! :) 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70218> Odds are that the majority of Gauge functions we set up will be through defer() on a libprocess Process, which I don't think will work with your current API. Can you add a test that uses a Gauge to read a member of a Process through defer? 3rdparty/libprocess/src/tests/metrics_tests.cpp <https://reviews.apache.org/r/18718/#comment70226> Are you planning to test the metrics registry here as well via json equality? - Ben Mahler On March 20, 2014, 9:44 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18718/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 9:44 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1036 > https://issues.apache.org/jira/browse/MESOS-1036 > > > Repository: mesos-git > > > Description > ------- > > See summary > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 3c6219eb6e76306463b3710ab7e50ec8b75d3d76 > 3rdparty/libprocess/include/process/metrics/counter.hpp PRE-CREATION > 3rdparty/libprocess/include/process/metrics/gauge.hpp PRE-CREATION > 3rdparty/libprocess/include/process/metrics/metric.hpp PRE-CREATION > 3rdparty/libprocess/src/metrics/metric.cpp PRE-CREATION > 3rdparty/libprocess/src/metrics/metrics.hpp PRE-CREATION > 3rdparty/libprocess/src/metrics/metrics.cpp PRE-CREATION > 3rdparty/libprocess/src/process.cpp > 6c6acc03ad78779e8f25b1a4584069fd377f647b > 3rdparty/libprocess/src/tests/metrics_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/18718/diff/ > > > Testing > ------- > > Added unit tests. make check. > > > Thanks, > > Dominic Hamon > >
