Replace ':' with '_' in stats name. by hennna · Pull Request #1806 · envoyproxy/envoy

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 henna@google.com

Signed-off-by: Henna Huang <henna@google.com>

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 <henna@google.com>

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

docs/configuration/cluster_manager/cluster.rst

  *(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

      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 <henna@google.com>

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 <henna@google.com>

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

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

bf36355

Signed-off-by: Henna Huang <henna@google.com>

hennna commented Oct 4, 2017

View reviewed changes

Fix docs

ba3f83b

Signed-off-by: Henna Huang <henna@google.com>

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 <henna@google.com>

Format fix

17760f6

Signed-off-by: Henna Huang <henna@google.com>

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 <henna@google.com>

nit

3267af4

Signed-off-by: Henna Huang <henna@google.com>

fix

7cb6a76

Signed-off-by: Henna Huang <henna@google.com>

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 <htuch@google.com>

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 <htuch@google.com>

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 <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

envoy: update upstream to 44bcb12 (#1806)

92db440

Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Elite1015 pushed a commit to Elite1015/data-plane-api that referenced this pull request Feb 23, 2025

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

715273a

In response to envoyproxy/envoy#1806 and envoyproxy/envoy#1871. Signed-off-by: Harvey Tuch <htuch@google.com>

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
junr03 merged 7 commits intoenvoyproxy:masterfrom
hennna:remove-colon-name-restriction

hennna commented Oct 4, 2017 •

edited

Loading

Uh oh!

hennna Oct 4, 2017

Uh oh!

htuch Oct 4, 2017

Uh oh!

hennna Oct 4, 2017

Uh oh!

htuch Oct 4, 2017

Uh oh!

mattklein123 Oct 4, 2017

Uh oh!

junr03 Oct 4, 2017 •

edited

Loading

Uh oh!

hennna Oct 4, 2017

Uh oh!

htuch left a comment •

edited

Loading

Uh oh!

htuch Oct 4, 2017

Uh oh!

mattklein123 Oct 4, 2017

Uh oh!

mrice32 commented Oct 4, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

hennna commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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