Skip to content

Fix Mongo stat separators#8444

Closed
kb000 wants to merge 1 commit intoenvoyproxy:masterfrom
kb000:remove-mongo-prefix-dot
Closed

Fix Mongo stat separators#8444
kb000 wants to merge 1 commit intoenvoyproxy:masterfrom
kb000:remove-mongo-prefix-dot

Conversation

@kb000
Copy link
Copy Markdown
Contributor

@kb000 kb000 commented Sep 30, 2019

Removes the trailing '.' from MongoStats stat_prefix format.
Inserting the separator happens when calling the POOL_*_PREFIX macros instead.

Risk Level: Low
Testing: Brought unit test init in line with factory method createFilterFactoryFromProtoTyped
Docs Changes: -
Release Notes: -
Fixes #8442

Also, add it back in when calling the POOL_*_PREFIX macros.

Signed-off-by: Kevin Burek <kburek@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member

@jmarantz when you review can you also check to see if any of the other conversions have a similar issue? Also, is there anything that we could be doing to better test this holistically? Thank you.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 1, 2019

Can you run the format-fixer?

I'm actually trying to understand what you are fixing. I merged to master in a client and then ran a test with some assertions and I didn't see a scenario where a stat was missing its dot in unit tests.

The test I ran was this:

diff --git a/include/envoy/stats/stats_macros.h b/include/envoy/stats/stats_macros.h
index 0959ff38a..bd9523ee0 100644
--- a/include/envoy/stats/stats_macros.h
+++ b/include/envoy/stats/stats_macros.h
@@ -40,9 +40,9 @@ namespace Envoy {
 #define FINISH_STAT_DECL_(X) + std::string(#X)),
 #define FINISH_STAT_DECL_MODE_(X, MODE) + std::string(#X), Envoy::Stats::Gauge::ImportMode::MODE),
 
-#define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(PREFIX FINISH_STAT_DECL_
-#define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(PREFIX FINISH_STAT_DECL_MODE_
-#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(PREFIX FINISH_STAT_DECL_
+#define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(Stats::VerifyPrefix(PREFIX) FINISH_STAT_DECL_
+#define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(Stats::VerifyPrefix(PREFIX) FINISH_STAT_DECL_MODE_
+#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(Stats::VerifyPrefix(PREFIX) FINISH_STAT_DECL_
 
 #define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "")
 #define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "")
@@ -52,4 +52,9 @@ namespace Envoy {
 #define NULL_STAT_DECL_IGNORE_MODE_(X, MODE) std::string(#X)),
 
 #define NULL_POOL_GAUGE(POOL) (POOL).nullGauge(NULL_STAT_DECL_IGNORE_MODE_
+
+namespace Stats {
+std::string VerifyPrefix(absl::string_view a);
+}
+
 } // namespace Envoy
diff --git a/source/common/stats/metric_impl.cc b/source/common/stats/metric_impl.cc
index 950679e6b..e7bb85afa 100644
--- a/source/common/stats/metric_impl.cc
+++ b/source/common/stats/metric_impl.cc
@@ -4,9 +4,16 @@
 
 #include "common/stats/symbol_table_impl.h"
 
+#include "absl/strings/match.h"
+
 namespace Envoy {
 namespace Stats {
 
+std::string VerifyPrefix(absl::string_view a) {
+  ASSERT(a.empty() || absl::EndsWith(a, "."));
+  return std::string(a);
+}
+
 MetricHelper::~MetricHelper() {
   // The storage must be cleaned by a subclass of MetricHelper in its
   // destructor, because the symbol-table is owned by the subclass.

I'm still running the full testsuite and I haven't seen any problems yet. First I ran

bazel test //test/extensions/filters/network/mongo_proxy:proxy_test

and to my surprise it passed. So what am I missing here?

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 1, 2019

OK I see some failures now with at assert; though not in Mongo. Will follow up.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 1, 2019

I see -- the bug wasn't that we were lacking the trailing dot on the prefix used for constructing stats with macros. The bug was that we had an extra trailing dot for capturing the prefix as a StatName. I have an ASSERT that I think helps for that, in the 2 impls of SymboTable::encode:

  ASSERT(!absl::EndsWith(name, "."));

that fires for this case; we'll see how many more.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 1, 2019

I have a more comprehensive version of this in #8445

@mattklein123
Copy link
Copy Markdown
Member

@kb000 if it's OK with you can we review and cover this in #8445? Please help review that one.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Oct 2, 2019

Agreed; I think #8445 is ready to go and covers this. @kb000 I could not seem to assign you #8445 or even "@"-add you, possibly because you are not in the Envoy org?

@kb000
Copy link
Copy Markdown
Contributor Author

kb000 commented Oct 9, 2019

Thanks! 😃

@kb000 kb000 deleted the remove-mongo-prefix-dot branch October 9, 2019 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mongo stats have an extra "." after prefix

3 participants