mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 67959: Added support for instrumenting processes.
Date Tue, 06 Nov 2018 06:13:59 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message