Replace ':' with '_' in stats name. #1806

hennna · 2017-10-04T00:46:37Z

Do a character replacement (':' to '_') in ScopeImpl to remove ':' restriction in user defined names. Stats must be created in a dedicated scope.

This PR handles cases that can be resolved by removing invalid character during stats scope creation (ex: cluster name and listener addresses). This PR is not general enough to allow arbitrary naming of counters, gauges, and histograms. The PR also does not reconcile accessing stats using unsanitized stats name.

Fixes #1782

Signed-off-by: Henna Huang [email protected]

Signed-off-by: Henna Huang <[email protected]>

hennna · 2017-10-04T00:47:09Z

test/common/upstream/cluster_manager_impl_test.cc

-                            "#/properties/name\n Schema violation: pattern\n Offending document "
-                            "key: #/name");
+  create(parseBootstrapFromJson(json));
+  EXPECT_EQ(0, factory_.stats_.counter("cluster.cluster_name.lb_local_cluster_not_ok").value());


Are there any non-zero stats that I can check here?

Can't you check any cluster stat that is per-cluster, adding a : in there?

These are the cluster stats that I see: https://github.com/envoyproxy/envoy/blob/master/include/envoy/upstream/upstream.h#L177

Signed-off-by: Henna Huang <[email protected]>

htuch · 2017-10-04T13:38:44Z

docs/configuration/cluster_manager/cluster.rst

@@ -31,7 +31,7 @@ Cluster
 name
  *(required, string)* Supplies the name of the cluster which must be unique across all clusters.
  The cluster name is used when emitting :ref:`statistics <config_cluster_manager_cluster_stats>`.
-  The cluster name can be at most 60 characters long, and must **not** contain ``:``.
+  The cluster name can be at most 60 characters long.


Probably a good idea to document the s/:/_/ that happens in cluster stats output.

mattklein123 · 2017-10-04T14:50:46Z

source/common/upstream/upstream_impl.cc

@@ -81,7 +87,7 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config,
          std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))),
      per_connection_buffer_limit_bytes_(
          PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)),
-      stats_scope_(stats.createScope(fmt::format("cluster.{}.", name_))),
+      stats_scope_(stats.createScope(clusterStatsName(name_))),


Thoughts on just moving the replace into the implementation of createScope() ? This would cover LDS also and any future cases. @junr03 you should create a scope for RDS stats so that if route table name has ':' in it it would also be fixed. I realize that RDS/LDS also have potential length issues but that is orthogonal to this.

yup, was following #1782. @hennna I don't want to duplicate your efforts here, lets decide if you are going to move the replace into the implementation of createScope(). If you do, then I don't have to fix anything, if you don't then I will update #1805.

@junr03 I've updated to push the character replacement in ScopeImpl. I believe this should work for you also.

Signed-off-by: Henna Huang <[email protected]>

htuch

LGTM. This is all very-statsd specific, I hope we don't run into situations with other stats sinks which have their own formatting constraints; @mrice32.

htuch · 2017-10-04T18:50:36Z

source/server/listener_manager_impl.cc

-  std::replace(final_stat_name.begin(), final_stat_name.end(), ':', '_');
-  listener_scope_ = parent_.server_.stats().createScope(final_stat_name);
+  listener_scope_ =
+      parent_.server_.stats().createScope(fmt::format("listener.{}.", address_->asString()));


Nit: may also want to validate this in a unit test.

mattklein123 · 2017-10-04T19:02:26Z

source/common/stats/thread_local_store.h

    ~ScopeImpl();
+    // ':' is a reserved char in statsd. Do the translation here and in
+    // IsolatedStoreImpl::ScopeImpl.
+    std::string sanitizeStatsName(const std::string& name) {


Can we move this into a common utility function.

mrice32 · 2017-10-04T19:15:57Z

@htuch I think we have a few choices when it comes to future sinks having similar formatting constraints:

Accept that is a possibility and be open to handling it upstream on a case-by-case basis.
Do generalized preprocessing of stats (configured separately from sinks because sinks can be mixed and matched) in a similar fashion to our plan for tags where one would be able to apply universal name changes through the config.
Force sinks to do this transformation on each flush (or establish a mapping) which would incur a latency penalty.

Signed-off-by: Henna Huang <[email protected]>

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <[email protected]>

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

Replace ':' with '_' in stats cluster name.

bf36355

Signed-off-by: Henna Huang <[email protected]>

hennna commented Oct 4, 2017

View reviewed changes

Fix docs

ba3f83b

Signed-off-by: Henna Huang <[email protected]>

htuch reviewed Oct 4, 2017

View reviewed changes

mattklein123 reviewed Oct 4, 2017

View reviewed changes

mattklein123 mentioned this pull request Oct 4, 2017

xDS: add version gauge #1805

Merged

hennna added 2 commits October 4, 2017 14:27

Move character replacement to ScopeImpl

4b853dd

Signed-off-by: Henna Huang <[email protected]>

Format fix

17760f6

Signed-off-by: Henna Huang <[email protected]>

hennna changed the title ~~Replace ':' with '_' in stats cluster name.~~ Replace ':' with '_' in stats name. Oct 4, 2017

htuch reviewed Oct 4, 2017

View reviewed changes

mattklein123 reviewed Oct 4, 2017

View reviewed changes

hennna added 3 commits October 4, 2017 16:15

Add common utility function and update tests.

98e30fc

Signed-off-by: Henna Huang <[email protected]>

nit

3267af4

Signed-off-by: Henna Huang <[email protected]>

fix

7cb6a76

Signed-off-by: Henna Huang <[email protected]>

mattklein123 approved these changes Oct 4, 2017

View reviewed changes

junr03 merged commit e7f454c into envoyproxy:master Oct 4, 2017

junr03 mentioned this pull request Oct 16, 2017

cds: remove stale comment envoyproxy/data-plane-api#195

Merged

htuch added a commit to htuch/envoy-api that referenced this pull request Oct 30, 2017

cds: update cluster name restrictions in Cluster.

13fc9f2

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <[email protected]>

htuch mentioned this pull request Oct 30, 2017

cds: update cluster name restrictions in Cluster. envoyproxy/data-plane-api#216

Merged

htuch added a commit to envoyproxy/data-plane-api that referenced this pull request Oct 30, 2017

cds: update cluster name restrictions in Cluster. (#216)

de2537c

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <[email protected]>

hennna deleted the remove-colon-name-restriction branch December 20, 2017 18:25

jpsim pushed a commit that referenced this pull request Nov 28, 2022

envoy: update upstream to 44bcb12 (#1806)

8a7d2a5

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

envoy: update upstream to 44bcb12 (#1806)

92db440

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ':' with '_' in stats name. #1806

Replace ':' with '_' in stats name. #1806

hennna commented Oct 4, 2017 •

edited

Loading

hennna Oct 4, 2017

htuch Oct 4, 2017

hennna Oct 4, 2017

htuch Oct 4, 2017

mattklein123 Oct 4, 2017

junr03 Oct 4, 2017 •

edited

Loading

hennna Oct 4, 2017

htuch left a comment •

edited

Loading

htuch Oct 4, 2017

mattklein123 Oct 4, 2017

mrice32 commented Oct 4, 2017 •

edited

Loading

Replace ':' with '_' in stats name. #1806

Replace ':' with '_' in stats name. #1806

Conversation

hennna commented Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 commented Oct 4, 2017 • edited Loading

hennna commented Oct 4, 2017 •

edited

Loading

junr03 Oct 4, 2017 •

edited

Loading

htuch left a comment •

edited

Loading

mrice32 commented Oct 4, 2017 •

edited

Loading