> On April 23, 2014, 6:17 p.m., Ben Mahler wrote: > > src/master/registrar.cpp, lines 148-151 > > <https://reviews.apache.org/r/20581/diff/4/?file=565299#file565299line148> > > > > Would love to see some structs here (we can name them and add > > constructors if necessary) > > > > struct Timers > > { > > Timer state_fetch; > > Timer state_store; > > } timers; > > > > struct Gauges > > { > > Gauge queued_operations; > > } gauges; > > > > It seems that we should make an exception here and use snake_case so > > that we match the names of the metrics as they are exposed in the endpoint? > > Dominic Hamon wrote: > This slightly complicates the gauge construction as the struct doesn't > have access to the access method for the operations queue size. I'll put up a > patch with this so you can see what's necessary and compare. > > Also - naming exceptions as a policy for metrics seem like the thin end > of a wedge. I don't think it's onerous to search for a metric name and find > the variable being constructed.
Ok will take a look. As for naming, it's important for us to consider the impact on the understandability of our system. I think for metrics, it is perfectly reasonable to make these match the external metric names. We considered this in a similar way when we were designing the cgroups::memory library. Is it clear to read cgroups::memory::limit_in_bytes and realize that this is touching the "limit_in_bytes" file? I think so. In this case, I think it's more clear to read "timers.state_fetch" and be able to directly map that to the metric name, rather than requiring a mental level of indirection in order to map "stateFetch" to "state_fetch". I would prefer clearer code that makes exceptions to rules than compromising clarity in favor of always following rules. As for thin edge of a wedge, that's what code reviews are for! We'll make sure rules are broken in reasonable ways. :) Does this make sense to you Dominic? - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20581/#review41173 ----------------------------------------------------------- On April 23, 2014, 6:44 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20581/ > ----------------------------------------------------------- > > (Updated April 23, 2014, 6:44 p.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Added Timers for state store/fetch and a Gauge to track the length of the > operations queue. > > > Diffs > ----- > > src/master/registrar.cpp 38040bdb519c7f0562ddfeb58d2809cffa6f4c18 > > Diff: https://reviews.apache.org/r/20581/diff/ > > > Testing > ------- > > local build + mesos-local.sh + manual inspection. > make check. > > > Thanks, > > Dominic Hamon > >
