stats: add support for custom prefixes by athampy · Pull Request #3029 · envoyproxy/envoy

athampy · 2018-04-09T16:08:36Z

stats: Add support for specifying a custom prefix

Description:
This PR adds support for specifying custom prefixes for StatsdSinks.

Testing:
Ran unit tests locally and added the following unit tests:

config_test.cc:UdpSinkCustomPrefix
config_test.cc:UdpSinkDefaultPrefix
config_test.cc:TcpSinkCustomPrefix
config_test.cc:TcpSinkDefaultPrefix

Release Notes: TODO(?)

Fixes #2897

Data Plane PR

TODO

revert changes to repository_locations.bzl

athampy · 2018-04-10T22:05:49Z

@alyssawilk this is ready for review

mattklein123

Thanks, some comments to get started.

mattklein123 · 2018-04-10T22:48:23Z

bazel/repository_locations.bzl

please update now that your change has been merged

mattklein123 · 2018-04-10T22:48:39Z

source/extensions/stat_sinks/common/statsd/statsd.cc

const std::string& prefix

mattklein123 · 2018-04-10T22:50:25Z

source/extensions/stat_sinks/common/statsd/statsd.h

const std::string prefix_. Just initialize directly in the initializer list using the ternary operator.

mattklein123 · 2018-04-10T22:50:34Z

source/extensions/stat_sinks/common/statsd/statsd.h

const std::string&

mattklein123 · 2018-04-10T22:51:42Z

source/extensions/stat_sinks/common/statsd/statsd.cc

prefix.empty(), initialize in init list per below.

Changed initialization to init list for both UdpStatsdSink and TcpStatsdSink

mattklein123 · 2018-04-10T22:51:59Z

source/extensions/stat_sinks/common/statsd/statsd.h

init in init list

mattklein123 · 2018-04-10T22:53:13Z

source/extensions/stat_sinks/common/statsd/statsd.h

This should use CONSTRUCT_ON_FIRST_USE so should be a function

Replaced with static const std::string& getDefaultPrefix()

dnoe · 2018-04-12T16:07:49Z

@mrice32 might also want to review since this deals with stats

athampy · 2018-04-16T23:40:49Z

source/extensions/stat_sinks/common/statsd/statsd.cc

The assert below could fail if the prefix is really long.

mrice32

Looks great! Just a few small implementation comments and one interface question.

mrice32 · 2018-04-17T18:32:43Z

source/extensions/stat_sinks/common/statsd/statsd.h

Is there a reason we want to disallow an empty prefix since the user already has the option to not provide the argument and get the default? (same for TCP)

An empty prefix doesn't make sense, imo. If an empty prefix is passed in, we just use the default instead. Also, if a prefix isn't specified statsd_sink.prefix() in createStatsSink will be an empty string. We can avoid multiple checks for an empty string by doing it here.

SGTM - I didn't realize this was coming directly from the proto, so there is no way to specify a default.

mrice32 · 2018-04-17T18:39:03Z

source/extensions/stat_sinks/common/statsd/statsd.h

Is there a particular reason that this needs to be a reference? It's usually assumed that when a const reference is passed in (via the constructor above, for instance) that it is only guaranteed to live for the duration of that function call - meaning it shouldn't be stored and used later (same for TCP).

You're right. This shouldn't be a reference. I've changed this to const std::string The getPrefix() methods for both Udp & Tcp sinks have been changed to return const std::string

mrice32 · 2018-04-17T18:41:29Z

test/extensions/stats_sinks/statsd/config_test.cc

Two comments:

Same as above, I think this should be ASSERT_NE since you'll get a segfault below if it's nullptr.

nit: this is a little hard to read - can we just dynamic cast once, save to a local variable, and then use the resulting pointer to do both checks?

(Same comments for other test cases)

Fixed in all four tests.

mrice32 · 2018-04-17T18:54:19Z

test/extensions/stats_sinks/statsd/config_test.cc

I think this should be ASSERT_NE since if it's nullptr, you'll get a segfault below (same for other tests below).

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

…d bump data-plane-api SHA Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

mrice32

LGTM. Thanks for adding this!

mrice32 · 2018-04-22T21:45:47Z

source/extensions/stat_sinks/common/statsd/statsd.h

SGTM - I didn't realize this was coming directly from the proto, so there is no way to specify a default.

mattklein123

LGTM, thanks. Just a couple of small comments.

mattklein123 · 2018-04-23T15:59:56Z

source/extensions/stat_sinks/common/statsd/statsd.h

  // Called in unit test to validate writer construction and address.
  int getFdForTests() { return tls_->getTyped<Writer>().getFdForTests(); }
  bool getUseTagForTest() { return use_tag_; }
+  const std::string getPrefix() { return prefix_; }


const std::string&

mattklein123 · 2018-04-23T16:00:02Z

source/extensions/stat_sinks/common/statsd/statsd.h

                                                 std::chrono::milliseconds(value));
  }

+  const std::string getPrefix() { return prefix_; }


const std::string&

mattklein123 · 2018-04-23T16:02:10Z

source/extensions/stat_sinks/common/statsd/statsd.cc

-  // 40 > 6 (prefix) + 4 (random chars) + 30 for number (bigger than it will ever be)
-  const uint32_t max_size = name.size() + 40;
+  // 34 >  4 (random chars) + 30 for number (bigger than it will ever be)
+  const uint32_t max_size = name.size() + parent_.getPrefix().size() + 40;


Shouldn't this be + 34 now?

Yep, updated.

mattklein123 · 2018-04-23T16:03:25Z

source/extensions/stat_sinks/common/statsd/statsd.cc

  ASSERT(current_slice_mem_ != nullptr);
-  // 40 > 6 (prefix) + 4 (random chars) + 30 for number (bigger than it will ever be)
-  const uint32_t max_size = name.size() + 40;
+  // 34 >  4 (random chars) + 30 for number (bigger than it will ever be)


Can you update this comment to:
34 > 4 (postfix chars, e.g., "|ms\n") + 30 for number (bigger than it will ever be)

alyssawilk

LGTM modulo Matt's comments

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

mattklein123

LGTM, small nit I missed before. Please also add a release not and unhide any docs that may be hidden. Thanks!

mattklein123 · 2018-04-23T21:04:04Z

source/extensions/stat_sinks/common/statsd/statsd.cc

-  current_slice_mem_ += sizeof(STAT_PREFIX) - 1;
+  memcpy(current_slice_mem_, parent_.getPrefix().c_str(), parent_.getPrefix().size());
+  current_slice_mem_ += parent_.getPrefix().size();
+  *current_slice_mem_ = '.';


nit: simplify to *current_slice_mem_++ = '.' like below. Also, technically you should modify the above space reservation comment to account for this dot.

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

mattklein123

Thanks!

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com> Signed-off-by: Rama <rama.rao@salesforce.com>

This seems to have been implemented in envoyproxy#3029. Signed-off-by: Snow Pettersen <snowp@squareup.com>

This seems to have been implemented in #3029. Signed-off-by: Snow Pettersen <snowp@squareup.com>

This seems to have been implemented in envoyproxy/envoy#3029. Signed-off-by: Snow Pettersen <snowp@squareup.com> Mirrored from https://github.com/envoyproxy/envoy @ 45d3e3f64074338fb8ac4e2e41226180b9c245b9

mattklein123 reviewed Apr 10, 2018

View reviewed changes

dnoe assigned mrice32 and alyssawilk Apr 12, 2018

athampy force-pushed the gh-2897 branch from 7349279 to 88937a4 Compare April 16, 2018 19:56

athampy commented Apr 16, 2018

View reviewed changes

source/extensions/stat_sinks/common/statsd/statsd.cc Outdated

Copy link

Member Author

athampy Apr 16, 2018

Choose a reason for hiding this comment

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

The assert below could fail if the prefix is really long.

mrice32 reviewed Apr 17, 2018

View reviewed changes

athampy added 8 commits April 20, 2018 16:10

stats: Update createStatsSink to pass in prefix to sink constructor

55fd426

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: add support for custom prefixes in statsd sinks

7c51e0c

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: add tests for custom prefix

a75b05d

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: fix format

2e0cd2c

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: fix tests

2a021a8

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: use CONSTRUCT_ON_FIRST_USE, clean up prefix initialization, an…

f472f78

…d bump data-plane-api SHA Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: fix namespaces in config_test after repo reorg

40d503b

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

stats: address review comments

807f7bf

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

athampy force-pushed the gh-2897 branch from 5269bac to 807f7bf Compare April 20, 2018 22:40

mrice32 reviewed Apr 22, 2018

View reviewed changes

mrice32 approved these changes Apr 22, 2018

View reviewed changes

mattklein123 reviewed Apr 23, 2018

View reviewed changes

alyssawilk previously approved these changes Apr 23, 2018

View reviewed changes

Address review comments

34bc646

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

athampy dismissed alyssawilk’s stale review via 34bc646 April 23, 2018 17:11

mattklein123 reviewed Apr 23, 2018

View reviewed changes

address nits

1399b2b

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com>

mattklein123 approved these changes Apr 24, 2018

View reviewed changes

mattklein123 merged commit 51663a1 into envoyproxy:master Apr 24, 2018

ramaraochavali pushed a commit to ramaraochavali/envoy that referenced this pull request May 3, 2018

stats: add support for custom prefixes (envoyproxy#3029)

5353782

Signed-off-by: Akhil Thampy <akhil@akhilthampy.com> Signed-off-by: Rama <rama.rao@salesforce.com>

snowp pushed a commit to snowp/envoy that referenced this pull request May 11, 2018

docs: remove not-implemented-hide for statsd sink prefix

6a0f891

This seems to have been implemented in envoyproxy#3029. Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp mentioned this pull request May 11, 2018

docs: remove not-implemented-hide for statsd sink prefix #3358

Merged

mattklein123 pushed a commit that referenced this pull request May 11, 2018

docs: remove not-implemented-hide for statsd sink prefix (#3358)

45d3e3f

This seems to have been implemented in #3029. Signed-off-by: Snow Pettersen <snowp@squareup.com>

taiki45 mentioned this pull request Sep 23, 2018

stats: DogStatsd sink supports custom metric name prefix #4506

Merged

Conversation

athampy commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athampy commented Apr 10, 2018

Uh oh!

mattklein123 left a comment

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!

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!

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!

dnoe commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

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!

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!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

athampy commented Apr 9, 2018 •

edited

Loading