> On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metric.cpp, lines 18-22 > > <https://reviews.apache.org/r/18718/diff/8/?file=530659#file530659line18> > > > > 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?
I had the same discussion with ~benjaminhindman and came to the same conclusion regarding blocking. I think that's safest, and shouldn't be a performance concern as we should only be creating/destroying these at initialization/finalization. I thought it was possible that process would enforce order of dispatch calls such that the call to 'remove' is guaranteed to run before the call to 'json' from HTTP requests, but blocking is safer. > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metrics.hpp, lines 18-26 > > <https://reviews.apache.org/r/18718/diff/8/?file=530660#file530660line18> > > > > I like the clarity of "registering" a metric and "unregistering" a > > metric, should we rename these to register() and unregister()? 'register' is a reserved keyword. > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metrics.cpp, line 3 > > <https://reviews.apache.org/r/18718/diff/8/?file=530661#file530661line3> > > > > This goes above ;) for now... ;) > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metrics.cpp, lines 26-27 > > <https://reviews.apache.org/r/18718/diff/8/?file=530661#file530661line26> > > > > 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); > > } nice :) > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metrics.cpp, line 78 > > <https://reviews.apache.org/r/18718/diff/8/?file=530661#file530661line78> > > > > 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. I can't imagine a need for other metrics-related endpoints that interact poorly with having the top-level endpoint be the overall snapshot of all metrics. The path then becomes a filter, whether that's the context directly, or 'keys', 'values' with context as a query, it still works perfectly well. When we come to expose timeseries and histograms, I expect those to be under a different process anyway. > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metrics.cpp, lines 101-103 > > <https://reviews.apache.org/r/18718/diff/8/?file=530661#file530661line101> > > > > Please write this out! :) I was taking this from the statistics example ;) > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/metrics_tests.cpp, lines 12-14 > > <https://reviews.apache.org/r/18718/diff/8/?file=530663#file530663line12> > > > > 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? I'd like to defer this (ha!) to a future patch to keep this initial version simple. I've been able to port the libprocess, slave, and master stats to this version without requiring defer so I don't think it's critical for a first version. > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/tests/metrics_tests.cpp, line 17 > > <https://reviews.apache.org/r/18718/diff/8/?file=530663#file530663line17> > > > > Are you planning to test the metrics registry here as well via json > > equality? I'll add a TODO for this to keep this patch small. > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metrics.cpp, line 63 > > <https://reviews.apache.org/r/18718/diff/8/?file=530661#file530661line63> > > > > 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. foreachpair doesn't seem too happy without the typedef. It looks like statistics used foreachkey instead, so i'll go with that. > On March 21, 2014, 3:10 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/src/metrics/metric.cpp, line 21 > > <https://reviews.apache.org/r/18718/diff/8/?file=530659#file530659line21> > > > > Looks like you can just bind to fatal instead of adding the wrapper > > function? > > > > Please place .onFailed on the subsequent line. fatal is a macro so i don't think i can. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18718/#review38207 ----------------------------------------------------------- On March 20, 2014, 2: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, 2: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 > >
