----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67959/#review210344 -----------------------------------------------------------
Some things that would be helpful in the commit summary / description: * The opt-in nature of this approach. * How the opt in support works (e.g. environment variable of process names?) * The metrics and their keys that are being added (e.g. `"processes/<name>/messages_enqueued"`). Some questions: * Should we lazily add the metrics? E.g. if the Process never receives http events, we wouldn't expose the http event ones. Seems nicer? * Should we use a pointer for the metrics struct to avoid the overhead in each Process? This would also allow us to hide the struct in the cpp file. * Per offline discussion, perhaps we can start with exact string match instead of regex? Regex seems unnecessarily complicated for this? * Right now there's no overall events counter, so to compute the queue size, you have to add all the enqueued counters and subtract all the dequeued counters, perhaps we could do the following: ``` processes/master/events_enqueued: 10 processes/master/events_enqueued/http: 4 processes/master/events_enqueued/dispatch: 5 processes/master/events_enqueued/messages: 1 processes/master/events_dequeued: 5 processes/master/events_dequeued/http: 2 processes/master/events_dequeued/dispatch: 2 processes/master/events_dequeued/messages: 1 ``` Also exposing a queue size directly (via a pull guauge that subtracts the counters) seems like a nice addition to this. Thoughts? - Benjamin Mahler On July 18, 2018, 1:28 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67959/ > ----------------------------------------------------------- > > (Updated July 18, 2018, 1:28 a.m.) > > > Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Greg Mann. > > > Repository: mesos > > > Description > ------- > > Added support for instrumenting processes. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/process.hpp > 7c255acc21c695eba37062a3dcf72ce33f650cd0 > 3rdparty/libprocess/src/process.cpp > 7c0a0bc0c1e50354b6da219032ac830cbeec0a0d > 3rdparty/libprocess/src/tests/process_tests.cpp > 8baf60d8bbb675e26fea5e76c825ef73fedbc629 > > > Diff: https://reviews.apache.org/r/67959/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
