----------------------------------------------------------- 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) <https://reviews.apache.org/r/44070/#comment183586> 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) <https://reviews.apache.org/r/44070/#comment183585> 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) <https://reviews.apache.org/r/44070/#comment183612> Includes for Owned, numify, Duration? 3rdparty/libprocess/src/metrics/metrics.cpp (line 30) <https://reviews.apache.org/r/44070/#comment183611> Pulling in the posix header means it won't compile on windows, so this should be the agnostic wrapper: ``` #include <stout/os.hpp> ``` 3rdparty/libprocess/src/metrics/metrics.cpp (line 33) <https://reviews.apache.org/r/44070/#comment183616> stale? 3rdparty/libprocess/src/metrics/metrics.cpp (line 41) <https://reviews.apache.org/r/44070/#comment183587> 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) <https://reviews.apache.org/r/44070/#comment183588> 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) <https://reviews.apache.org/r/44070/#comment183592> 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) <https://reviews.apache.org/r/44070/#comment183593> 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) <https://reviews.apache.org/r/44070/#comment183613> 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) <https://reviews.apache.org/r/44070/#comment183595> 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) <https://reviews.apache.org/r/44070/#comment183596> else if? 3rdparty/libprocess/src/metrics/metrics.cpp (line 210) <https://reviews.apache.org/r/44070/#comment183597> 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) <https://reviews.apache.org/r/44070/#comment183614> 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) <https://reviews.apache.org/r/44070/#comment183610> What is a slave? ;) 3rdparty/libprocess/src/metrics/metrics.cpp (line 253) <https://reviews.apache.org/r/44070/#comment183615> Why did you change this from a pointer to a stack object? We don't allow static non-PODs. - Ben Mahler On March 2, 2016, 5: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, 5: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 > >
