Use StatNameManagedStorage for creating StatName(s).#452
Use StatNameManagedStorage for creating StatName(s).#452jplevyak merged 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
jplevyak
changed the title
Signed-off-by: John Plevyak <jplevyak@gmail.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
lgtm modulo type nit and leaving a TODO for the scope.
| WasmResult Context::defineMetric(MetricType type, absl::string_view name, uint32_t* metric_id_ptr) { | ||
| auto stat_name = wasm_->stat_name_set_->add(name); | ||
| Stats::StatNameManagedStorage storage(name, wasm()->scope_->symbolTable()); | ||
| auto stat_name = storage.statName(); |
There was a problem hiding this comment.
s/auto/Stats::StatName/
https://google.github.io/styleguide/cppguide.html#Type_deduction
the type of this is obvious to me personally but will not necessarily be to readers of this code, as it is not derived from context. The type is not onerous either so I think the guideline clearly states that explicit type is preferred here.
There was a problem hiding this comment.
Done: nit and added a TODO.
Signed-off-by: John Plevyak <jplevyak@gmail.com>
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
Thanks; looks great. One question, which doesn't block submission in my opinion.
| ALL_WASM_STATS(POOL_COUNTER_PREFIX(*scope_, absl::StrCat("wasm.", runtime, ".")), | ||
| POOL_GAUGE_PREFIX(*scope_, absl::StrCat("wasm.", runtime, ".")))}), | ||
| stat_name_set_(scope_->symbolTable().makeSet("Wasm").release()) { | ||
| POOL_GAUGE_PREFIX(*scope_, absl::StrCat("wasm.", runtime, ".")))}) { |
There was a problem hiding this comment.
I assume it's OK to take a bunch of locks here, as this does not happen in the request path; is that right?
You can easily code around this if you need to, and I've been mulling over creating a new family of stats-macros for instantiating stats without taking locks.
There was a problem hiding this comment.
Typically this is done in the configure path which is coming out of the dispatcher, so same thread, but not (typically) in the request path.
2 participants
Signed-off-by: John Plevyak jplevyak@gmail.com