Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Ben Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/#review121768
---



Thanks for the patience, I left some comments for some issues that remained. 
I'll get this committed shortly with adjustments based on the comments.


3rdparty/libprocess/include/process/metrics/metrics.hpp (lines 33 - 36)


This should be called from process::initialize, if we'd like this to be 
consistent with Clock::initialize, mime::initialize, EventLoop::initialize:

```
  // TODO(bmahler): Consider initializing this consistently with
  // the other global Processes.
  metrics::internal::MetricsProcess* metricsProcess =
metrics::internal::MetricsProcess::instance();

  CHECK_NOTNULL(metricsProcess);
```

Becomes:

```
  // Ensure metrics process is running.
  metrics::initialize();
```

Two newlines below but one above?



3rdparty/libprocess/include/process/metrics/metrics.hpp (line 79)


Hm.. it's hard to tell what "setting up global infrastructure" mean? How 
about:

```
// Needed to access the private constructor.
```



3rdparty/libprocess/src/metrics/metrics.cpp (lines 19 - 30)


Includes for Owned, numify, Duration?



3rdparty/libprocess/src/metrics/metrics.cpp (line 30)


Pulling in the posix header means it won't compile on windows, so this 
should be the agnostic wrapper: 

```
#include 
```



3rdparty/libprocess/src/metrics/metrics.cpp (line 33)


stale?



3rdparty/libprocess/src/metrics/metrics.cpp (line 41)


Looking around at the globals in the code base, I can't seem to find an 
instance of us naming the global "singleton". How about "metrics_process" to be 
consistent with the naming in proces.cpp for now?



3rdparty/libprocess/src/metrics/metrics.cpp (line 196)


Since it's at the top of the header, can you move this to the top of the 
.cpp file?

Also, whoops, brace should be on the next line here.



3rdparty/libprocess/src/metrics/metrics.cpp (lines 197 - 260)


Hm.. any reason you introduced two ways to check that we only initialize 
once? The first check seems unreliable as Alexander mentioned, so is it a 
performance optimization?

Also, it seems like the rate limiting parsing and creation should only 
occur when the once determines this is the only execution, right? Was this 
intentionally moved outside the once for some reason? Because as it stands we 
may create some unnecessary rate limiters and throw them away, which seems odd.



3rdparty/libprocess/src/metrics/metrics.cpp (line 201)


How about s/rateLimiter/limiter/ here and elsewhere to be consistent with 
how it is named in the MetricsProcess?



3rdparty/libprocess/src/metrics/metrics.cpp (line 205)


We planned to add more endpoints in the future, so perhaps 
LIBPROCESS_METRICS_SNAPSHOT_ENDPOINT_RATE_LIMIT would be more appropriate here. 
For example, if we exposed an endoint for getting historical data, that may not 
need the same rate limiting since it doesn't hit components through gauges.



3rdparty/libprocess/src/metrics/metrics.cpp (lines 207 - 209)


A comment that we default to 2 per second would be great here, noting that 
we default to this for backwards compatibility. The default was originally in 
place to apply a reasonable limit, and now we have to keep it for backwards 
compatibility.



3rdparty/libprocess/src/metrics/metrics.cpp (lines 209 - 210)


else if?



3rdparty/libprocess/src/metrics/metrics.cpp (line 210)


You can use -> instead of .get()., can you do a sweep within your code?

```
value->empty()
// vs
value.get().empty()
```



3rdparty/libprocess/src/metrics/metrics.cpp (lines 215 - 238)


It would be nice to do error messaging once rather than 3 places.

I realize this was copied, but we tend to EXIT upon invalid user input. 
Since LOG(FATAL) prints a stack trace, users tend to think mesos has crashed 
because there is a bug in mesos.



3rdparty/libprocess/src/metrics/metrics.cpp (line 236)


 

Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Alexander Rojas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/#review121743
---




3rdparty/libprocess/src/metrics/metrics.cpp (lines 197 - 199)


This is definitely not enough to prevent a race here. Two `rateLimiters` 
may very well instantiated (though the once below will prevent the singleton 
from being initialized twice).

Can we move the `Once initialize` test right after this if?


- Alexander Rojas


On March 2, 2016, 6:48 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44070/
> ---
> 
> (Updated March 2, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4776
> https://issues.apache.org/jira/browse/MESOS-4776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed disabling metrics endpoint rate limiting via the environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 
> 09b716be56eac38f75d79d917799c001aa0b205c 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> a9840083722dd6b7b6aab692ed449407ab125ac7 
> 
> Diff: https://reviews.apache.org/r/44070/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, not optimized)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/
---

(Updated March 2, 2016, 6:48 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Addressed offline comments from bmahler.


Bugs: MESOS-4776
https://issues.apache.org/jira/browse/MESOS-4776


Repository: mesos


Description
---

Allowed disabling metrics endpoint rate limiting via the environment.


Diffs (updated)
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
09b716be56eac38f75d79d917799c001aa0b205c 
  3rdparty/libprocess/src/metrics/metrics.cpp 
a9840083722dd6b7b6aab692ed449407ab125ac7 

Diff: https://reviews.apache.org/r/44070/diff/


Testing
---

make check (OS X, not optimized)


Thanks,

Benjamin Bannier

Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/
---

(Updated March 1, 2016, 10:04 p.m.)


Review request for mesos.


Changes
---

Addressed offline bmahler's comments.

* Added a metrics initialization function
* use an optional rate limiter instead of a nop instance
* use a different input format for rate limit.


Bugs: MESOS-4776
https://issues.apache.org/jira/browse/MESOS-4776


Repository: mesos


Description
---

Allowed disabling metrics endpoint rate limiting via the environment.


Diffs (updated)
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
09b716be56eac38f75d79d917799c001aa0b205c 
  3rdparty/libprocess/src/metrics/metrics.cpp 
a9840083722dd6b7b6aab692ed449407ab125ac7 

Diff: https://reviews.apache.org/r/44070/diff/


Testing
---

make check (OS X, not optimized)


Thanks,

Benjamin Bannier

Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/
---

(Updated Feb. 28, 2016, 10:28 p.m.)


Review request for mesos.


Changes
---

Addressed comments from Alexander, allowed to either disable rate limiting or 
set it.


Bugs: MESOS-4776
https://issues.apache.org/jira/browse/MESOS-4776


Repository: mesos


Description
---

Allowed disabling metrics endpoint rate limiting via the environment.


Diffs (updated)
-

  3rdparty/libprocess/include/process/limiter.hpp 
25254cdc2cb095b96d58d3f73dafc0ea48824998 
  3rdparty/libprocess/include/process/metrics/metrics.hpp 
09b716be56eac38f75d79d917799c001aa0b205c 
  3rdparty/libprocess/src/metrics/metrics.cpp 
a9840083722dd6b7b6aab692ed449407ab125ac7 

Diff: https://reviews.apache.org/r/44070/diff/


Testing
---

make check (OS X, not optimized)


Thanks,

Benjamin Bannier

Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Alexander Rojas]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/#review120908
---




3rdparty/libprocess/include/process/metrics/metrics.hpp (line 74)


We don't really use `std::unique_ptr` yet. A grep for it only shows 
messages to do it at some point. In the meantime, let's be consistent and keep 
using `Owned`.



3rdparty/libprocess/src/metrics/metrics.cpp (lines 38 - 44)


I think it would be nicer to have this in the `limiter.hpp` file.



3rdparty/libprocess/src/metrics/metrics.cpp (lines 48 - 50)


We have `Option os::getenv(const std::string&)` which I think 
is a better fit for what you're trying to achieve.



3rdparty/libprocess/src/metrics/metrics.cpp (line 55)


Not yours but, could you add at least a TODO saying this should be 
configurable? Two permissions every second sounds rather arbitrary.


- Alexander Rojas


On Feb. 26, 2016, 4:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44070/
> ---
> 
> (Updated Feb. 26, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4776
> https://issues.apache.org/jira/browse/MESOS-4776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed disabling metrics endpoint rate limiting via the environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 
> 09b716be56eac38f75d79d917799c001aa0b205c 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> a9840083722dd6b7b6aab692ed449407ab125ac7 
> 
> Diff: https://reviews.apache.org/r/44070/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, not optimized)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/#review120882
---




3rdparty/libprocess/src/metrics/metrics.cpp (line 50)


Use `LIBPROCESS_METRICS_RATE_LIMIT` here which can denote queries per 
second. If set but contains no value we can interpret it as completely 
disabling rate limiting.


- Benjamin Bannier


On Feb. 26, 2016, 4:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44070/
> ---
> 
> (Updated Feb. 26, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4776
> https://issues.apache.org/jira/browse/MESOS-4776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allowed disabling metrics endpoint rate limiting via the environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 
> 09b716be56eac38f75d79d917799c001aa0b205c 
>   3rdparty/libprocess/src/metrics/metrics.cpp 
> a9840083722dd6b7b6aab692ed449407ab125ac7 
> 
> Diff: https://reviews.apache.org/r/44070/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, not optimized)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 44070: Allowed disabling metrics endpoint rate limiting via the environment.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44070/
---

Review request for mesos.


Bugs: MESOS-4776
https://issues.apache.org/jira/browse/MESOS-4776


Repository: mesos


Description
---

Allowed disabling metrics endpoint rate limiting via the environment.


Diffs
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
09b716be56eac38f75d79d917799c001aa0b205c 
  3rdparty/libprocess/src/metrics/metrics.cpp 
a9840083722dd6b7b6aab692ed449407ab125ac7 

Diff: https://reviews.apache.org/r/44070/diff/


Testing
---

make check (OS X, not optimized)


Thanks,

Benjamin Bannier