mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 70783: Added debug logging for metrics which are slow to become ready.
Date Wed, 05 Jun 2019 15:40:08 GMT


> On June 4, 2019, 8:42 p.m., Joseph Wu wrote:
> > I think it is a little unfortunate that we end up with a conditional `select()`
and an extra chaining step in this endpoint.  But since we aren't saving the vector of keys/futures/statistics
anywhere, the chaining is mostly unavoidable.
> > 
> > Since this adds some significant logic, I would like there to be at least an addition
to the tests.  We can't read the resulting log line, but we can at least exercise the code.
 For example, copying this chunk of existing test code and increasing the in-test timeout
to 10+ seconds:
> > 
> > ```
> > TEST_F(MetricsTest, THREADSAFE_SnapshotTimeout)
> > {
> >   ...
> >   
> >   // Get the snapshot.
> >   Future<Response> response = http::get(upid, "snapshot", "timeout=2secs");
// <----------------- Increase this timeout.
> > 
> >   // Make sure the request is pending before the timeout is exceeded.
> >   //
> >   // TODO(neilc): Replace the `sleep` here with a less flaky
> >   // synchronization method.
> >   os::sleep(Milliseconds(10));
> >   Clock::settle();
> > 
> >   ASSERT_TRUE(response.isPending());
> > 
> >   // Advance the clock to trigger the timeout.
> >   Clock::advance(Seconds(2)); // <----------------------------- And this timeout.
> > 
> >   AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
> > 
> >   // Parse the response.
> >   Try<JSON::Object> responseJSON = JSON::parse<JSON::Object>(response->body);
> >   ASSERT_SOME(responseJSON);
> > 
> >   // We can't use simple JSON equality testing here as initializing
> >   // libprocess adds metrics to the system. We want to only check if
> >   // the metrics from this test are correctly handled.
> >   map<string, JSON::Value> values = responseJSON->values;
> > 
> >   EXPECT_EQ(1u, values.count("test/counter"));
> >   EXPECT_DOUBLE_EQ(0.0, values["test/counter"].as<JSON::Number>().as<double>());
> > 
> >   EXPECT_EQ(1u, values.count("test/gauge"));
> >   EXPECT_DOUBLE_EQ(42.0, values["test/gauge"].as<JSON::Number>().as<double>());
> > 
> >   EXPECT_EQ(0u, values.count("test/gauge_fail"));
> >   EXPECT_EQ(0u, values.count("test/gauge_timeout"));
> > ```

Good call, I updated the test.


- Greg


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


On June 5, 2019, 3:39 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70783/
> -----------------------------------------------------------
> 
> (Updated June 5, 2019, 3:39 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, Joseph Wu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added debug logging for metrics which are slow to become ready.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/metrics/metrics.hpp 75711edbaf46797e5eb54ba720ea11cf3de81522

>   3rdparty/libprocess/src/metrics/metrics.cpp 623d44adbe838f995ddbe89ee26f5bcc9c600be5

>   3rdparty/libprocess/src/tests/metrics_tests.cpp 881275693e67f3c9fb670c7e70cb5014090ed7a5

> 
> 
> Diff: https://reviews.apache.org/r/70783/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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