Skip to content
This repository was archived by the owner on Dec 16, 2020. It is now read-only.

Switch to new Stats API. #121

Merged
jplevyak merged 4 commits into from
Aug 13, 2019
Merged

Switch to new Stats API. #121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d702df
Switch to new Stats API.
jplevyak Aug 9, 2019
c968020
Address comments.
jplevyak Aug 12, 2019
2e5c9d2
Address comments.
jplevyak Aug 12, 2019
d74aef8
Address comments.
jplevyak Aug 12, 2019
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
1 change: 1 addition & 0 deletions source/extensions/common/wasm/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ envoy_cc_library(
"//include/envoy/server:wasm_interface",
"//include/envoy/upstream:cluster_manager_interface",
"//source/common/common:stack_array",
"//source/common/stats:symbol_table_lib",
"//source/extensions/filters/http:well_known_names",
"@envoy_api//envoy/config/filter/http/wasm/v2:wasm_cc",
],
Expand Down
21 changes: 10 additions & 11 deletions source/extensions/common/wasm/wasm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1680,23 +1680,21 @@ void Context::onGrpcReceiveTrailingMetadata(uint32_t token, Http::HeaderMapPtr&&
}

uint32_t Context::defineMetric(MetricType type, absl::string_view name) {
auto stat_name = wasm_->stat_name_set_.getStatName(name);
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Aug 9, 2019

Choose a reason for hiding this comment

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

Generally looks correct. If you know a priori some stat-names you want to use, even if it's the context of concatenating them with other stat-names, you may want to have an interface to 'remember' them into the StatNameSet to avoid taking locks here.

I think to do this you'd need to carve out more of your integer space below for names that could be explicitly declared and saved by WASM code.

The current interface you provide here where you take a string will definitely wind up taking a lock. Not sure how important that is for wasm performance.

Copy link
Copy Markdown
Contributor Author

@jplevyak jplevyak Aug 9, 2019

Choose a reason for hiding this comment

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

I'd rather avoid the lock or share the set in some way. In the normal case we are going to have 1 wasm VM per thread/silo and they will be registering the same stats but the stats are not known a priori (this is a dynamic plugin system).

So either std::shared_ptr over a shared StatNameSet or I reimplement it without a lock (since the VMs are always single threaded).

Alternatively I can have a separate interface for pre-registering strings for stats. That would add a good deal more complexity. We are already planning making tags first class entities. It seems like a string registration facility would fit well in that model.

However, in the short term I would like to just move to the new stat system so perhaps this code is a good interm step.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Aug 9, 2019

Choose a reason for hiding this comment

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

Sharing or replicating the StatNameSet is not going to help, because there will be locks on the underlying symbol table.

I'm not sure exactly what it means to have tags be first class entities, but that sounds promising :) As long as you are not converting string_view to StatName on the hot-path it should be fine.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz Aug 10, 2019

Choose a reason for hiding this comment

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

Sorry, reflecting on my statement above, it is incorrect :)

Replicating a StatNameSet per thread will indeed eliminate global symbol-lock contention (once the system is warm, but at the cost of a lot of memory. The StatName/SymbolTable infrastructure is designed to avoid keeping elaborated stat names in memory, and StatNameSet defeats that. It was introduced for a few specific use-cases where some of the tokens used to compose stat-names are discovered only in the request, but are expected to be small in number.

Copy link
Copy Markdown
Contributor Author

@jplevyak jplevyak Aug 12, 2019

Choose a reason for hiding this comment

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

So this PR will work but it no optimal in memory use. That is fine for now since we will have to change the way WebAssembly modules are initialized in order to take advantage of per-registration of stats/symbols and we would like to avoid the global locking.

Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Aug 12, 2019

Choose a reason for hiding this comment

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

Add TODO(jplevyak): ... to all those calls to replace it with something better in the future?

Copy link
Copy Markdown
Contributor Author

@jplevyak jplevyak Aug 12, 2019

Choose a reason for hiding this comment

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

Added a comment block.

if (type == MetricType::Counter) {
auto id = wasm_->nextCounterMetricId();
wasm_->counters_.emplace(id, &wasm_->scope_.counter(std::string(
name))); // This is inefficient, but it is the Scope API.
auto c = &wasm_->scope_.counterFromStatName(stat_name);
wasm_->counters_.emplace(id, c);
return id;
} else if (type == MetricType::Gauge) {
auto id = wasm_->nextGaugeMetricId();
wasm_->gauges_.emplace(
id,
&wasm_->scope_.gauge(
std::string(name),
Stats::Gauge::ImportMode::Accumulate)); // This is inefficient, but it is the Scope API.
auto g = &wasm_->scope_.gaugeFromStatName(stat_name, Stats::Gauge::ImportMode::Accumulate);
wasm_->gauges_.emplace(id, g);
return id;
} else if (type == MetricType::Histogram) {
auto id = wasm_->nextHistogramMetricId();
wasm_->histograms_.emplace(id, &wasm_->scope_.histogram(std::string(
name))); // This is inefficient, but it is the Scope API.
auto h = &wasm_->scope_.histogramFromStatName(stat_name);
wasm_->histograms_.emplace(id, h);
return id;
}
return 0;
Expand Down Expand Up @@ -1766,7 +1764,8 @@ Wasm::Wasm(absl::string_view vm, absl::string_view id, absl::string_view vm_conf
Stats::ScopeSharedPtr owned_scope)
: cluster_manager_(cluster_manager), dispatcher_(dispatcher), scope_(scope),
local_info_(local_info), listener_metadata_(listener_metadata), owned_scope_(owned_scope),
time_source_(dispatcher.timeSource()), vm_configuration_(vm_configuration) {
time_source_(dispatcher.timeSource()), vm_configuration_(vm_configuration),
stat_name_set_(scope_.symbolTable()) {
wasm_vm_ = Common::Wasm::createWasmVm(vm);
id_ = std::string(id);
}
Expand Down Expand Up @@ -1946,7 +1945,7 @@ Wasm::Wasm(const Wasm& wasm, Event::Dispatcher& dispatcher)
: std::enable_shared_from_this<Wasm>(wasm), cluster_manager_(wasm.cluster_manager_),
dispatcher_(dispatcher), scope_(wasm.scope_), local_info_(wasm.local_info_),
listener_metadata_(wasm.listener_metadata_), id_(wasm.id_), owned_scope_(wasm.owned_scope_),
time_source_(dispatcher.timeSource()) {
time_source_(dispatcher.timeSource()), stat_name_set_(scope_.symbolTable()) {
wasm_vm_ = wasm.wasmVm()->clone();
vm_context_ = std::make_shared<Context>(this);
getFunctions();
Expand Down
6 changes: 6 additions & 0 deletions source/extensions/common/wasm/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "common/common/c_smart_ptr.h"
#include "common/common/logger.h"
#include "common/common/stack_array.h"
#include "common/stats/symbol_table_impl.h"

#include "extensions/common/wasm/well_known_names.h"
#include "extensions/filters/http/well_known_names.h"
Expand Down Expand Up @@ -761,6 +762,11 @@ class Wasm : public Envoy::Server::Wasm,
std::unique_ptr<Global<double>> global_Infinity_;

// Stats/Metrics
// TODO(jplevyak): replace the use of Stats::StatNameSet with something more efficient.
// By having a separate StatNameSet per Wasm we are duplicating all the strings but
// avoiding locks. Consider lock-free hash tables or pre-registering stats.
Stats::StatNameSet stat_name_set_;
absl::flat_hash_map<std::string, Stats::StatName> stat_names_;
uint32_t next_counter_metric_id_ = kMetricTypeCounter;
uint32_t next_gauge_metric_id_ = kMetricTypeGauge;
uint32_t next_histogram_metric_id_ = kMetricTypeHistogram;
Expand Down
3 changes: 1 addition & 2 deletions tools/check_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@
# https://github.com/envoyproxy/envoy/pull/7573 and others.
#
# TODO(#4196): Eliminate this list completely and then merge #4980.
STAT_FROM_STRING_WHITELIST = ("./source/extensions/common/wasm/wasm.cc",
"./source/extensions/filters/http/fault/fault_filter.cc",
STAT_FROM_STRING_WHITELIST = ("./source/extensions/filters/http/fault/fault_filter.cc",
"./source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc",
"./source/extensions/filters/network/mongo_proxy/proxy.cc",
"./source/extensions/stat_sinks/common/statsd/statsd.cc",
Expand Down