-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Description
IsolatedStoreImpl is used in a few spots, mostly in tests. One example of this is stats_store_ in ServerInstanceImplTest. I was trying to add to the Stats unit test there, which asserts that a few essential server stats are populated, and noticed that only counters and gauges are tested, not histograms:
EXPECT_NE(nullptr, TestUtility::findCounter(stats_store_, "server.watchdog_miss"));
EXPECT_EQ(2L, TestUtility::findGauge(stats_store_, "server.concurrency")->value());
EXPECT_EQ(3L, TestUtility::findGauge(stats_store_, "server.hot_restart_epoch")->value());I wanted to test the presence of a new histogram, so the next thing I discovered is that TestUtility has no findHistogram. And when I implemented it, it always failed. Finally, digging deeper, I discovered:
std::vector<ParentHistogramSharedPtr> histograms() const override {
return std::vector<ParentHistogramSharedPtr>{};
}IsolatedStoreImpl always returns an empty set of histograms. This makes sense: we're not using thread-local storage and merging histograms across threads, so there is no ParentHistogram. Likewise, deliverHistogramToSinks is empty, and when we allocate histograms in the isolated store, we don't allocate a parent:
histograms_([this](const std::string& name) -> HistogramSharedPtr {
return std::make_shared<HistogramImpl>(name, *this, std::string(name), std::vector<Tag>());
})Still, it would be nice for IsolatedStoreImpl::histograms() to return something. Would it be possible to create a trivial IsolatedParentHistogramImpl that just references a single histogram? @jmarantz