Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
5 changes: 5 additions & 0 deletions include/envoy/api/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ class Api {
* @return a reference to the TimeSource
*/
virtual TimeSource& timeSource() PURE;

/**
* @return a constant reference to the root Stats::Scope
*/
virtual const Stats::Scope& rootScope() PURE;
};

typedef std::unique_ptr<Api> ApiPtr;
Expand Down
21 changes: 21 additions & 0 deletions include/envoy/stats/scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,27 @@ class Scope {
*/
virtual Histogram& histogram(const std::string& name) PURE;

/**
* @param The name of the stat, obtained from the SymbolTable.
* @return a pointer to a counter within the scope's namespace, or nullptr if
Comment thread
ahedberg marked this conversation as resolved.
Outdated
* no counter with that name exists.
*/
virtual const Counter* getCounter(StatName name) const PURE;
Comment thread
ahedberg marked this conversation as resolved.
Outdated

/**
* @param The name of the stat, obtained from the SymbolTable.
* @return a pointer to a gauge within the scope's namespace, or nullptr if
* no gauge with that name exists.
*/
virtual const Gauge* getGauge(StatName name) const PURE;

/**
* @param The name of the stat, obtained from the SymbolTable.
* @return a pointer to a histogram within the scope's namespace, or nullptr if
* no histogram with that name exists.
*/
virtual const Histogram* getHistogram(StatName name) const PURE;

/**
* @return a reference to the symbol table.
*/
Expand Down
7 changes: 4 additions & 3 deletions source/common/api/api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@
namespace Envoy {
namespace Api {

Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system,
Filesystem::Instance& file_system)
: thread_factory_(thread_factory), time_system_(time_system), file_system_(file_system) {}
Impl::Impl(Thread::ThreadFactory& thread_factory, Stats::Store& store,
Event::TimeSystem& time_system, Filesystem::Instance& file_system)
: thread_factory_(thread_factory), store_(store), time_system_(time_system),
file_system_(file_system) {}

Event::DispatcherPtr Impl::allocateDispatcher() {
return std::make_unique<Event::DispatcherImpl>(*this, time_system_);
Expand Down
4 changes: 3 additions & 1 deletion source/common/api/api_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace Api {
*/
class Impl : public Api {
public:
Impl(Thread::ThreadFactory& thread_factory, Stats::Store&, Event::TimeSystem& time_system,
Impl(Thread::ThreadFactory& thread_factory, Stats::Store& store, Event::TimeSystem& time_system,
Filesystem::Instance& file_system);

// Api::Api
Expand All @@ -25,9 +25,11 @@ class Impl : public Api {
Thread::ThreadFactory& threadFactory() override { return thread_factory_; }
Filesystem::Instance& fileSystem() override { return file_system_; }
TimeSource& timeSource() override { return time_system_; }
const Stats::Scope& rootScope() override { return store_; }

private:
Thread::ThreadFactory& thread_factory_;
Stats::Store& store_;
Event::TimeSystem& time_system_;
Filesystem::Instance& file_system_;
};
Expand Down
12 changes: 12 additions & 0 deletions source/common/stats/isolated_store_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ template <class Base> class IsolatedStatsCache {
return *new_stat;
}

const Base* get(StatName name) const {
Comment thread
ahedberg marked this conversation as resolved.
Outdated
auto stat = stats_.find(name);
if (stat == stats_.end()) {
name.debugPrint();
return nullptr;
}
return stat->second.get();
}

std::vector<std::shared_ptr<Base>> toVector() const {
std::vector<std::shared_ptr<Base>> vec;
vec.reserve(stats_.size());
Expand Down Expand Up @@ -69,6 +78,9 @@ class IsolatedStoreImpl : public StoreImpl {
Histogram& histogram = histograms_.get(name);
return histogram;
}
const Counter* getCounter(StatName name) const override { return counters_.get(name); }
const Gauge* getGauge(StatName name) const override { return gauges_.get(name); }
const Histogram* getHistogram(StatName name) const override { return histograms_.get(name); }

// Stats::Store
std::vector<CounterSharedPtr> counters() const override { return counters_.toVector(); }
Expand Down
18 changes: 18 additions & 0 deletions source/common/stats/scope_prefixer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,24 @@ Histogram& ScopePrefixer::histogramFromStatName(StatName name) {
return scope_.histogramFromStatName(StatName(stat_name_storage.get()));
}

const Counter* ScopePrefixer::getCounter(StatName name) const {
Stats::SymbolTable::StoragePtr stat_name_storage =
scope_.symbolTable().join({prefix_.statName(), name});
return scope_.getCounter(StatName(stat_name_storage.get()));
}

const Gauge* ScopePrefixer::getGauge(StatName name) const {
Stats::SymbolTable::StoragePtr stat_name_storage =
scope_.symbolTable().join({prefix_.statName(), name});
return scope_.getGauge(StatName(stat_name_storage.get()));
}

const Histogram* ScopePrefixer::getHistogram(StatName name) const {
Stats::SymbolTable::StoragePtr stat_name_storage =
scope_.symbolTable().join({prefix_.statName(), name});
return scope_.getHistogram(StatName(stat_name_storage.get()));
}

void ScopePrefixer::deliverHistogramToSinks(const Histogram& histograms, uint64_t val) {
scope_.deliverHistogramToSinks(histograms, val);
}
Expand Down
4 changes: 4 additions & 0 deletions source/common/stats/scope_prefixer.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ class ScopePrefixer : public Scope {
return histogramFromStatName(storage.statName());
}

const Counter* getCounter(StatName name) const override;
const Gauge* getGauge(StatName name) const override;
const Histogram* getHistogram(StatName name) const override;

const SymbolTable& symbolTable() const override { return scope_.symbolTable(); }
virtual SymbolTable& symbolTable() override { return scope_.symbolTable(); }

Expand Down
132 changes: 131 additions & 1 deletion source/common/stats/thread_local_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,6 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
StatMap<std::shared_ptr<StatType>>* tls_cache, StatNameHashSet* tls_rejected_stats,
StatType& null_stat) {

// We do name-rejections on the full name, prior to truncation.
if (tls_rejected_stats != nullptr &&
tls_rejected_stats->find(name) != tls_rejected_stats->end()) {
return null_stat;
Expand Down Expand Up @@ -329,12 +328,62 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
// If we have a TLS cache, insert the stat.
if (tls_cache) {
tls_cache->insert(std::make_pair((*central_ref)->statName(), *central_ref));
ENVOY_LOG_MISC(debug, "stat added to central cache map");
name.debugPrint();
ENVOY_LOG_MISC(debug, "central_cache_map = {}", static_cast<void*>(&central_cache_map));
ENVOY_LOG_MISC(debug, "this = {}", static_cast<const void*>(this));
ENVOY_LOG_MISC(debug, "parent_ = {}", static_cast<void*>(&parent_));
Comment thread
ahedberg marked this conversation as resolved.
Outdated
}

// Finally we return the reference.
return **central_ref;
}

// TODO(ahedberg): This is largely duplicated from safeMakeStat and should
// be cleaned up at some point.
Comment thread
ahedberg marked this conversation as resolved.
Outdated
template <class StatType>
const StatType* ThreadLocalStoreImpl::ScopeImpl::safeGetStat(
StatName name, StatMap<std::shared_ptr<StatType>>& central_cache_map,
StatMap<std::shared_ptr<StatType>>* tls_cache, StatNameHashSet* tls_rejected_stats) const {

if (tls_rejected_stats != nullptr &&
Comment thread
ahedberg marked this conversation as resolved.
Outdated
tls_rejected_stats->find(name) != tls_rejected_stats->end()) {
return nullptr;
}

// If we have a valid cache entry, return it.
if (tls_cache) {
Comment thread
ahedberg marked this conversation as resolved.
Outdated
auto pos = tls_cache->find(name);
if (pos != tls_cache->end()) {
return pos->second.get();
}
}

// We must now look in the central store so we must be locked. We grab a reference to the
// central store location. It might contain nothing. In this case, we return
// nullptr.
// already holding lock in parent
auto iter = central_cache_map.find(name);
if (iter == central_cache_map.end()) {
// The stat doesn't exist in the central store, so return nullptr.
ENVOY_LOG_MISC(debug, "stat not in central cache map");
name.debugPrint();
ENVOY_LOG_MISC(debug, "central_cache_map = {}", static_cast<void*>(&central_cache_map));
ENVOY_LOG_MISC(debug, "this = {}", static_cast<const void*>(this));
ENVOY_LOG_MISC(debug, "parent_ = {}", static_cast<void*>(&parent_));
return nullptr;
}

std::shared_ptr<StatType>* central_ref = &(iter->second);
// If we have a TLS cache, insert the stat.
if (tls_cache) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you indicated 'done' in the comment I made earlier suggesting we not bother with the tls cache, since you are holding a lock anyway. I see you are no longer reading from it, but I'm not sure that usage of the 'find' method is a good signal that we should populate the current thread's cache. In particular -- not really fully understanding the application -- I was wondering if this might tend to be run from the main thread, whose tls cache will (I believe) be consulted approximately never. @mattklein123 & @fredlas for opinions on this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see--I think I misunderstood a previous conversation that we should populate the tls cache upon reading. I don't feel strongly either way.

We're planning on calling this from a filter, similar to eds_ready_filter.cc, if that changes anyone else's opinions.

tls_cache->insert(std::make_pair((*central_ref)->statName(), *central_ref));
}

// Finally we return the pointer to the stat.
return central_ref->get();
}

Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatName(StatName name) {
if (parent_.rejectsAll()) {
return parent_.null_counter_;
Expand Down Expand Up @@ -473,6 +522,87 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name)
return **central_ref;
}

const Counter* ThreadLocalStoreImpl::ScopeImpl::getCounter(StatName name) const {
if (parent_.rejectsAll()) {
Comment thread
ahedberg marked this conversation as resolved.
Outdated
return nullptr;
}

// See comments in counter(). There is no super clean way (via templates or otherwise) to
// share this code so I'm leaving it largely duplicated for now.
Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name});
StatName final_stat_name(final_name.get());

StatMap<CounterSharedPtr>* tls_cache = nullptr;
StatNameHashSet* tls_rejected_stats = nullptr;
if (!parent_.shutting_down_ && parent_.tls_) {
TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
tls_cache = &entry.counters_;
tls_rejected_stats = &entry.rejected_stats_;
}

return safeGetStat<Counter>(final_stat_name, central_cache_.counters_, tls_cache,
tls_rejected_stats);
}

const Gauge* ThreadLocalStoreImpl::ScopeImpl::getGauge(StatName name) const {
if (parent_.rejectsAll()) {
return nullptr;
}

// See comments in counter(). There is no super clean way (via templates or otherwise) to
// share this code so I'm leaving it largely duplicated for now.
Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name});
StatName final_stat_name(final_name.get());

StatMap<GaugeSharedPtr>* tls_cache = nullptr;
StatNameHashSet* tls_rejected_stats = nullptr;
if (!parent_.shutting_down_ && parent_.tls_) {
TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
tls_cache = &entry.gauges_;
tls_rejected_stats = &entry.rejected_stats_;
}

return safeGetStat<Gauge>(final_stat_name, central_cache_.gauges_, tls_cache, tls_rejected_stats);
}

const Histogram* ThreadLocalStoreImpl::ScopeImpl::getHistogram(StatName name) const {
if (parent_.rejectsAll()) {
return nullptr;
}

// See comments in counter(). There is no super clean way (via templates or otherwise) to
// share this code so I'm leaving it largely duplicated for now.
Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name});
StatName final_stat_name(final_name.get());

StatMap<ParentHistogramSharedPtr>* tls_cache = nullptr;
StatNameHashSet* tls_rejected_stats = nullptr;
if (!parent_.shutting_down_ && parent_.tls_) {
TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
tls_cache = &entry.parent_histograms_;
auto iter = tls_cache->find(final_stat_name);
if (iter != tls_cache->end()) {
return iter->second.get();
}
tls_rejected_stats = &entry.rejected_stats_;
if (tls_rejected_stats->find(final_stat_name) != tls_rejected_stats->end()) {
return nullptr;
}
}

auto iter = central_cache_.histograms_.find(final_stat_name);
ParentHistogramImplSharedPtr* central_ref = nullptr;
if (iter == central_cache_.histograms_.end()) {
return nullptr;
}

central_ref = &iter->second;
if (tls_cache != nullptr) {
tls_cache->insert(std::make_pair((*central_ref)->statName(), *central_ref));
}
return central_ref->get();
}

Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name,
ParentHistogramImpl& parent) {
// tlsHistogram() is generally not called for a histogram that is rejected by
Expand Down
53 changes: 51 additions & 2 deletions source/common/stats/thread_local_store.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,39 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); }
SymbolTable& symbolTable() override { return alloc_.symbolTable(); }
const TagProducer& tagProducer() const { return *tag_producer_; }

const Counter* getCounter(StatName name) const override {
const Counter* found_counter = nullptr;
Thread::LockGuard lock(lock_);
for (ScopeImpl* scope : scopes_) {
found_counter = scope->getCounter(name);
if (found_counter != nullptr) {
return found_counter;
}
}
return nullptr;
}
const Gauge* getGauge(StatName name) const override {
const Gauge* found_gauge = nullptr;
Thread::LockGuard lock(lock_);
for (ScopeImpl* scope : scopes_) {
found_gauge = scope->getGauge(name);
if (found_gauge != nullptr) {
return found_gauge;
}
}
return nullptr;
}
const Histogram* getHistogram(StatName name) const override {
const Histogram* found_histogram = nullptr;
Thread::LockGuard lock(lock_);
for (ScopeImpl* scope : scopes_) {
found_histogram = scope->getHistogram(name);
if (found_histogram != nullptr) {
return found_histogram;
}
}
return nullptr;
}
// Stats::Store
std::vector<CounterSharedPtr> counters() const override;
std::vector<GaugeSharedPtr> gauges() const override;
Expand Down Expand Up @@ -239,6 +271,10 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo

NullGaugeImpl& nullGauge(const std::string&) override { return parent_.null_gauge_; }

const Counter* getCounter(StatName name) const override;
const Gauge* getGauge(StatName name) const override;
const Histogram* getHistogram(StatName name) const override;

template <class StatType>
using MakeStatFn = std::function<std::shared_ptr<StatType>(StatDataAllocator&, StatName name,
absl::string_view tag_extracted_name,
Expand All @@ -262,6 +298,19 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
StatMap<std::shared_ptr<StatType>>* tls_cache,
StatNameHashSet* tls_rejected_stats, StatType& null_stat);

/**
* Looks up an existing stat, populating the local cache if necessary.
*
* @param name the full name of the stat (not tag extracted).
* @param central_cache_map a map from name to the desired object in the central cache.
* @return a pointer to the named stat, or nullptr if it doesn't exist.
*/
template <class StatType>
const StatType* safeGetStat(StatName name,
StatMap<std::shared_ptr<StatType>>& central_cache_map,
StatMap<std::shared_ptr<StatType>>* tls_cache,
StatNameHashSet* tls_rejected_stats) const;

void extractTagsAndTruncate(StatName& name,
std::unique_ptr<StatNameManagedStorage>& truncated_name_storage,
std::vector<Tag>& tags, std::string& tag_extracted_name);
Expand All @@ -271,7 +320,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
const uint64_t scope_id_;
ThreadLocalStoreImpl& parent_;
StatNameStorage prefix_;
CentralCacheEntry central_cache_;
mutable CentralCacheEntry central_cache_;
};

struct TlsCache : public ThreadLocal::ThreadLocalObject {
Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ envoy_cc_test(
":http_integration_lib",
"//source/common/upstream:load_balancer_lib",
"//test/config:utility_lib",
"//test/integration/filters:eds_ready_filter_config_lib",
"//test/test_common:network_utility_lib",
"@envoy_api//envoy/api/v2:eds_cc",
],
Expand Down
Loading