Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stats: refactoring MetricImpl without strings #4190

Merged
merged 4 commits into from
Aug 17, 2018
Merged

stats: refactoring MetricImpl without strings #4190

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fb3aae
MetricImpl without std::string
ambuc Aug 16, 2018
5b149f5
Merge remote-tracking branch 'upstream/master' into metric-impl-dedup
ambuc Aug 17, 2018
d40d1cd
Remove MetricImpl::name(), to be overridden everywhere
ambuc Aug 17, 2018
506006a
Better documentation for MetricImpl
ambuc Aug 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/envoy/stats/stats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class Metric {
/**
* Returns the full name of the Metric.
*/
virtual const std::string& name() const PURE;
virtual const std::string name() const PURE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need to make this change in this PR? I understand why you'd need to make it in the SymbolTable PR.

Ditto for the other changes where you were returning a reference and now you are returning a copy.

Copy link
Contributor Author

@ambuc ambuc Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it has to be this way because we are relying on the private member variable name_ inside RawStatData. name_ is a char[], so we could return a string_view to it or a string of it, but not a string ref. If we returned a string_view I think we would have to rework lots of other function interfaces where we expect a string. (lookups in maps, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, makes sense. In the context of this PR, string_view is the right option. You'll have to return string-by-value if/when we move to SymbolTable.

The advantage of your approach here (returning string-by-value) is that it will work for SymbolTable. Switching callers to take string_view is optimal in this PR, but would actually would be the wrong direction to go if we are heading to SymbolTable. I'm hopeful the compiler will tell us if we write:

  absl::string_view_name = stat->name();

if name() returns a string-by-value. If not, in the SymbolTable PR, you can just rename the method (even temporarily) to force you to examine all methods via compiler errors.

So I think my feeling is you should do the right thing or this PR and return a string_view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep this as-is, since we might have to convert this back into a string at one of the aforementioned interfaces/callsites, and since an incumbent symbol_table PR will play more cleanly with a string interface. Converting this to a string_view everywhere means the reworking of maps and structures which store strings now to store container<string_view, _, StringViewHash_, StringViewCompare_>, and they'll just end up churning again into container<StatNamePtr, _, StatNamePtrHash_, StatNamePtrCompare_>.


/**
* Returns a vector of configurable tags to identify this Metric.
Expand Down
1 change: 1 addition & 0 deletions source/common/stats/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ envoy_cc_library(
hdrs = ["metric_impl.h"],
deps = [
"//include/envoy/stats:stats_interface",
"//source/common/common:assert_lib",
],
)

Expand Down
5 changes: 5 additions & 0 deletions source/common/stats/heap_stat_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ struct HeapStatData {
*/
absl::string_view key() const { return name_; }

/**
* @returns std::string the name as a std::string.
*/
std::string name() const { return name_; }

std::atomic<uint64_t> value_{0};
std::atomic<uint64_t> pending_increment_{0};
std::atomic<uint16_t> flags_{0};
Expand Down
7 changes: 6 additions & 1 deletion source/common/stats/histogram_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ class HistogramImpl : public Histogram, public MetricImpl {
public:
HistogramImpl(const std::string& name, Store& parent, std::string&& tag_extracted_name,
std::vector<Tag>&& tags)
: MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), parent_(parent) {}
: MetricImpl(std::move(tag_extracted_name), std::move(tags)), parent_(parent), name_(name) {}

// Stats:;Metric
const std::string name() const override { return name_; }

// Stats::Histogram
void recordValue(uint64_t value) override { parent_.deliverHistogramToSinks(*this, value); }
Expand All @@ -56,6 +59,8 @@ class HistogramImpl : public Histogram, public MetricImpl {
private:
// This is used for delivering the histogram data to sinks.
Store& parent_;

const std::string name_;
};

} // namespace Stats
Expand Down
8 changes: 4 additions & 4 deletions source/common/stats/metric_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#include "envoy/stats/stats.h"

#include "common/common/assert.h"

namespace Envoy {
namespace Stats {

Expand All @@ -14,10 +16,9 @@ namespace Stats {
*/
class MetricImpl : public virtual Metric {
public:
MetricImpl(const std::string& name, std::string&& tag_extracted_name, std::vector<Tag>&& tags)
: name_(name), tag_extracted_name_(std::move(tag_extracted_name)), tags_(std::move(tags)) {}
MetricImpl(std::string&& tag_extracted_name, std::vector<Tag>&& tags)
: tag_extracted_name_(std::move(tag_extracted_name)), tags_(std::move(tags)) {}

const std::string& name() const override { return name_; }
const std::string& tagExtractedName() const override { return tag_extracted_name_; }
const std::vector<Tag>& tags() const override { return tags_; }

Expand All @@ -30,7 +31,6 @@ class MetricImpl : public virtual Metric {
};

private:
const std::string name_;
const std::string tag_extracted_name_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we could get rid of this too; we'd have to change consumers of tagExtractedName() and tags() to compute them on demand from TagProducerImpl::produceTags(). That could be a separate PR.

I don't know what the CPU cost of that would be; if that happens during operation, or would be only while setting up a stats sinks. @mrice32 , WDYT?

If not, the tag_extracted_name_ string would be ripe for symbolizing later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with spinning this off into a separate PR, but yeah, if we do this for one variable we might well do it for several.

const std::vector<Tag> tags_;
};
Expand Down
5 changes: 5 additions & 0 deletions source/common/stats/raw_stat_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ struct RawStatData {
*/
absl::string_view key() const { return absl::string_view(name_); }

/**
* Returns the name as a std::string.
*/
std::string name() const { return std::string(name_); }

std::atomic<uint64_t> value_;
std::atomic<uint64_t> pending_increment_;
std::atomic<uint16_t> flags_;
Expand Down
12 changes: 8 additions & 4 deletions source/common/stats/stat_data_allocator_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,12 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
public:
CounterImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
std::string&& tag_extracted_name, std::vector<Tag>&& tags)
: MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
alloc_(alloc) {}
: MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {}
~CounterImpl() { alloc_.free(data_); }

// Stats::Metric
const std::string name() const override { return data_.name(); }

// Stats::Counter
void add(uint64_t amount) override {
data_.value_ += amount;
Expand All @@ -92,10 +94,12 @@ template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
public:
GaugeImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
std::string&& tag_extracted_name, std::vector<Tag>&& tags)
: MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
alloc_(alloc) {}
: MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {}
~GaugeImpl() { alloc_.free(data_); }

// Stats::Metric
const std::string name() const override { return data_.name(); }

// Stats::Gauge
virtual void add(uint64_t amount) override {
data_.value_ += amount;
Expand Down
8 changes: 4 additions & 4 deletions source/common/stats/thread_local_store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,8 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(const std::string& name
ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(const std::string& name,
std::string&& tag_extracted_name,
std::vector<Tag>&& tags)
: MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), current_active_(0),
flags_(0), created_thread_id_(std::this_thread::get_id()) {
: MetricImpl(std::move(tag_extracted_name), std::move(tags)), current_active_(0), flags_(0),
created_thread_id_(std::this_thread::get_id()), name_(name) {
histograms_[0] = hist_alloc();
histograms_[1] = hist_alloc();
}
Expand All @@ -373,10 +373,10 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) {
ParentHistogramImpl::ParentHistogramImpl(const std::string& name, Store& parent,
TlsScope& tls_scope, std::string&& tag_extracted_name,
std::vector<Tag>&& tags)
: MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), parent_(parent),
: MetricImpl(std::move(tag_extracted_name), std::move(tags)), parent_(parent),
tls_scope_(tls_scope), interval_histogram_(hist_alloc()), cumulative_histogram_(hist_alloc()),
interval_statistics_(interval_histogram_), cumulative_statistics_(cumulative_histogram_),
merged_(false) {}
merged_(false), name_(name) {}

ParentHistogramImpl::~ParentHistogramImpl() {
hist_free(interval_histogram_);
Expand Down
8 changes: 8 additions & 0 deletions source/common/stats/thread_local_store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,16 @@ class ThreadLocalHistogramImpl : public Histogram, public MetricImpl {
void recordValue(uint64_t value) override;
bool used() const override { return flags_ & Flags::Used; }

// Stats::Metric
const std::string name() const override { return name_; }

private:
uint64_t otherHistogramIndex() const { return 1 - current_active_; }
uint64_t current_active_;
histogram_t* histograms_[2];
std::atomic<uint16_t> flags_;
std::thread::id created_thread_id_;
const std::string name_;
};

typedef std::shared_ptr<ThreadLocalHistogramImpl> TlsHistogramSharedPtr;
Expand Down Expand Up @@ -86,6 +90,9 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl {
}
const std::string summary() const override;

// Stats::Metric
const std::string name() const override { return name_; }

private:
bool usedLockHeld() const EXCLUSIVE_LOCKS_REQUIRED(merge_lock_);

Expand All @@ -98,6 +105,7 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl {
mutable Thread::MutexBasicLockable merge_lock_;
std::list<TlsHistogramSharedPtr> tls_histograms_ GUARDED_BY(merge_lock_);
bool merged_;
const std::string name_;
};

typedef std::shared_ptr<ParentHistogramImpl> ParentHistogramImplSharedPtr;
Expand Down
2 changes: 0 additions & 2 deletions test/mocks/stats/mocks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ namespace Envoy {
namespace Stats {

MockCounter::MockCounter() {
ON_CALL(*this, name()).WillByDefault(ReturnRef(name_));
ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_));
ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_));
ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_));
Expand All @@ -24,7 +23,6 @@ MockCounter::MockCounter() {
MockCounter::~MockCounter() {}

MockGauge::MockGauge() {
ON_CALL(*this, name()).WillByDefault(ReturnRef(name_));
ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_));
ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_));
ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_));
Expand Down
14 changes: 10 additions & 4 deletions test/mocks/stats/mocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@ class MockCounter : public Counter {
MockCounter();
~MockCounter();

// Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I still don't understand what this means and why it happens (I realize this was copied).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand it either. I think it was introduced in #1803. @mrice32, can you supply context here?

// creates a deadlock in gmock and is an unintended use of mock functions.
const std::string name() const override { return name_; };

MOCK_METHOD1(add, void(uint64_t amount));
MOCK_METHOD0(inc, void());
MOCK_METHOD0(latch, uint64_t());
MOCK_CONST_METHOD0(name, const std::string&());
MOCK_CONST_METHOD0(tagExtractedName, const std::string&());
MOCK_CONST_METHOD0(tags, const std::vector<Tag>&());
MOCK_METHOD0(reset, void());
Expand All @@ -49,10 +52,13 @@ class MockGauge : public Gauge {
MockGauge();
~MockGauge();

// Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
// creates a deadlock in gmock and is an unintended use of mock functions.
const std::string name() const override { return name_; };

MOCK_METHOD1(add, void(uint64_t amount));
MOCK_METHOD0(dec, void());
MOCK_METHOD0(inc, void());
MOCK_CONST_METHOD0(name, const std::string&());
MOCK_CONST_METHOD0(tagExtractedName, const std::string&());
MOCK_CONST_METHOD0(tags, const std::vector<Tag>&());
MOCK_METHOD1(set, void(uint64_t value));
Expand All @@ -73,7 +79,7 @@ class MockHistogram : public Histogram {

// Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
// creates a deadlock in gmock and is an unintended use of mock functions.
const std::string& name() const override { return name_; };
const std::string name() const override { return name_; };

MOCK_CONST_METHOD0(tagExtractedName, const std::string&());
MOCK_CONST_METHOD0(tags, const std::vector<Tag>&());
Expand All @@ -92,7 +98,7 @@ class MockParentHistogram : public ParentHistogram {

// Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
// creates a deadlock in gmock and is an unintended use of mock functions.
const std::string& name() const override { return name_; };
const std::string name() const override { return name_; };
void merge() override {}
const std::string summary() const override { return ""; };

Expand Down