stats: native DogStatsD support by taiki45 · Pull Request #2158 · envoyproxy/envoy

taiki45 · 2017-12-05T14:58:57Z

Description:
Add new stats sink which support tagged metrics with DogStatsD format.
This feature is configurable by specifying envoy.dog_statsd in Bootstrap.stats_sinks.
Config and documentation changes are here: envoyproxy/data-plane-api#325.
Original issue is #2024.

Risk Level:
Medium?: New features that are not enabled by default.

Testing:
unit test, and manual testing with statsd_exporter which supports DogStatsD extension and Prometheus.

Docs Changes:
envoyproxy/data-plane-api#325

Release Notes:
Added, but I'm not sure about format. I referred https://github.com/envoyproxy/data-plane-api/blob/master/docs/root/intro/version_history.rst.

Fixes #2024

API Changes:
envoyproxy/data-plane-api#325

Review Note:
I'm not a fluent English speaker. Please feel free to point out any bit of difference, thanks.

Add new stats sink which supports tagged metrics with DogStatsD format. At the first moment, the sink only supports UDP output. Tested with stastd-exporter v0.5.0 and Prometheus v2.0.0. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch · 2017-12-05T16:31:20Z

source/common/stats/dog_statsd.cc

+Writer::Writer(Network::Address::InstanceConstSharedPtr address) : Statsd::Writer(address) {}
+
+void Writer::writeCounter(const std::string& name, const std::string& tag_str, uint64_t increment) {
+  std::string message(fmt::format("envoy.{}:{}|c{}\n", name, increment, tag_str));


At a high-level, wondering if you could comment on the tradeoff between doing this as a separate sink vs. making this a configurable option on the existing statsd sink? It seems the formats are fairly similar, with regular `statsd a subset. Don't mind a separate sink if it makes more sense.

Right, it's another option to implement this. In my opinion, pros of a separate sink: dedicated sink confiuration brings future extensibility on configuration around tagging. Then, pros of exteding existing statsd sink: compact code base and configuration.

I think we have to think which is more user-friendly idea not from point of code-base. In my current opinion, since the original issue #2024 starts from a new sink with dedicated configuration, the new sink seems more human natural idea. What do you think?

For code-base or design points, I'm welcome to any advices and will make improvements.

@taiki45 I think having a dedicated configuration makes sense from the user perspective. I think the main question is whether to have a shared base class for the implementation (or just one implementation with some functionality variable built in). I don't really have that strong of an opinion either way as the code is pretty self contained.

Aha, now I got the question. In this case, for code parspective, I agree to change current implementation because inheritting from concrete class (Envoy::Stats::Statsd::UdpStatsdSink class here) slightly smells bad for me.

I'm thinking about using and extending existing Envoy::Stats::Statsd::UdpStatsdSink instead of having a shared base class. Any advices?

Sure that SGTM.

Done in 5937ac4.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 · 2017-12-06T10:18:05Z

hmm... continuous-integration/travis-ci seems not working? It's waiting forever...

mattklein123 · 2017-12-06T13:48:53Z

@taiki45 sometimes CI is just broken. You need to merge master anyway so just push a new commit and it will probably fix itself.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 · 2017-12-06T14:47:55Z

OK, I've resolved the conflicts and pushed the merge commit.

htuch · 2017-12-06T19:50:31Z

Looks like //test/integration:integration_test is timing out on OS X, but I'd hazard a guess that this is not your PR and is general flake, I will kick it again.

htuch

Refactored approach looks good.

htuch · 2017-12-06T19:54:14Z

source/common/stats/statsd.cc

+    if (i == 0) {
+      tagStr.append(fmt::format("{}:{}", tag.name_, tag.value_));
+    } else {
+      tagStr.append(fmt::format(",{}:{}", tag.name_, tag.value_));


I'd recommend using https://github.com/envoyproxy/envoy/blob/master/source/common/common/utility.h#L140 here to simplify.

Good. be26f9f

Also, I'm curious about better ways to join std::vector<Tag>& tags with "," into single string, joining each tag with ":". I did some googling but I can't found the way...

htuch · 2017-12-06T19:54:51Z

source/common/stats/statsd.cc

+    return {};
+  }
+
+  std::string tagStr = "|#";


Snake rather than camel case for locals, i.e. tag_str.

htuch · 2017-12-06T19:56:45Z

source/common/stats/statsd.cc

+}
+
+const std::string buildTagStr(const std::vector<Tag>& tags) {
+  if (tags.empty()) {


Should this be use_tag_ || tags.empty() to preserve existing behavior?

Agreed. 80560ce

htuch · 2017-12-06T19:57:59Z

source/common/stats/statsd.h

+
  ThreadLocal::SlotPtr tls_;
  Network::Address::InstanceConstSharedPtr server_address_;
+  bool useTag_;


Snake rather than camel for member variables, i.e. use_tag_.

Also, this can be const bool.

htuch · 2017-12-06T20:06:03Z

test/common/stats/udp_statsd_test.cc

+  ON_CALL(counter, tagExtractedName()).WillByDefault(ReturnRef(name));
+  ON_CALL(counter, tags()).WillByDefault(ReturnRef(tags));
+
+  sink.flushCounter(counter, 1);


I don't see the actual expected statsd raw output in any of these tests? Shouldn't we have something like this?

I think this is an issue with the existing tests; there's no validation that we're actually building the correct string.

Yes, agree to add the validation test. I'll try it in this PR.

Thanks, this would be a nice improvement to the existing tests.

It was a little bit difficult for me, but I tried with changing Writer's responsibility.
How about this one: e3487df

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Also use `const` for bool. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

For envoyproxy/envoy#2158. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch · 2017-12-07T11:55:07Z

source/common/stats/statsd.cc

-void Writer::writeCounter(const std::string& name, uint64_t increment) {
-  std::string message(fmt::format("envoy.{}:{}|c", name, increment));
+void Writer::writeCounter(const std::string& name, const std::string& tag_str, uint64_t increment) {
+  std::string message(fmt::format("envoy.{}:{}|c{}\n", name, increment, tag_str));


Should we avoid the \n for regular statsd? i.e. only add it in buildTagStr() if use_tag_?

Nice point. Actually we don't need trailing newline, so just dropped it. b664048

htuch · 2017-12-07T11:58:51Z

source/common/stats/statsd.cc

+  for (const Tag& tag : tags) {
+    if (i == 0) {
+      std::vector<std::string> v = {tag.name_, tag.value_};
+      tag_str.append(StringUtil::join(v, ":"));


The idea with join is you do something like:

std::vector<std::string> tag_strs; for (const Tag& tag : tags) { tag_strs.emplace_back(tag._name_ + ":" + tag._value); } return "|#" + StringUtil::join(tag_strs, ",");

This is probably less performant than what you have, but more concise. Since this is only at statsd flush time, it's probably fine; if we hit scaling issues then we can micro-optimize.

Since this is only at statsd flush time, it's probably fine

Agreed. Refine with 3c38668.

htuch · 2017-12-07T11:59:09Z

test/common/stats/udp_statsd_test.cc

+  ON_CALL(counter, tagExtractedName()).WillByDefault(ReturnRef(name));
+  ON_CALL(counter, tags()).WillByDefault(ReturnRef(tags));
+
+  sink.flushCounter(counter, 1);


Thanks, this would be a nice improvement to the existing tests.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Remove process of building stats string from Writer and it now only holds a fd. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

mattklein123

Generally looks good. A bunch of nits and a little extra test coverage needed. Thank you!

mattklein123 · 2017-12-11T03:07:06Z

source/common/stats/statsd.cc


 void UdpStatsdSink::flushCounter(const Counter& counter, uint64_t delta) {
-  tls_->getTyped<Writer>().writeCounter(counter.name(), delta);
+  std::string message(


nit: const here and similar places, or just kill local variable and do inline as var not needed.

Yep, use const d3fb9ef.

mattklein123 · 2017-12-11T03:08:11Z

source/common/stats/statsd.cc

+
+const std::string UdpStatsdSink::buildTagStr(const std::vector<Tag>& tags) {
+  if (!use_tag_ || tags.empty()) {
+    return {};


nit: I would probably use "" here as that is more idiomatic.

Agreed: 0109362

mattklein123 · 2017-12-11T03:08:45Z

source/common/stats/statsd.cc

+    return {};
+  }
+
+  std::vector<std::string> tag_strs;


nit: s/tag_strs/tag_strings

mattklein123 · 2017-12-11T03:09:01Z

source/common/stats/statsd.h

-  void writeGauge(const std::string& name, uint64_t value);
-  void writeTimer(const std::string& name, const std::chrono::milliseconds& ms);
+  // For testing.
+  Writer() : fd_(-1){};


nit: del ';' at end of line

mattklein123 · 2017-12-11T03:09:48Z

source/common/stats/statsd.h

+  UdpStatsdSink(ThreadLocal::SlotAllocator& tls, Network::Address::InstanceConstSharedPtr address,
+                const bool use_tag);
+  // For testing.
+  UdpStatsdSink(ThreadLocal::SlotAllocator& tls, std::shared_ptr<Writer> w, const bool use_tag)


nit: s/w/writer

mattklein123 · 2017-12-11T03:10:57Z

test/common/stats/udp_statsd_test.cc


+class MockWriter : public Writer {
+public:
+  MOCK_METHOD1(write, void(const std::string& m));


nit: s/m/message

mattklein123 · 2017-12-11T03:11:58Z

test/common/stats/udp_statsd_test.cc

  EXPECT_NE(fd, -1);

  // Check that fd has not changed.
+  std::vector<Tag> tags;


I would recommend moving this into the base mock class, and then putting the ON_CALL WillByDefault into the constructor, so that it can be use by others later.

Cool, how about this one? 85dab7f

mattklein123 · 2017-12-11T03:12:51Z

test/common/stats/udp_statsd_test.cc

+  ON_CALL(counter, tags()).WillByDefault(ReturnRef(tags));
+  EXPECT_CALL(*std::dynamic_pointer_cast<NiceMock<MockWriter>>(writer_ptr),
+              write("envoy.test_counter:1|c"));
+  ;


nit: remove ';' on lines by itself in this file.

mattklein123 · 2017-12-11T03:14:50Z

source/server/config/stats/dog_statsd.cc

+namespace Server {
+namespace Configuration {
+
+Stats::SinkPtr DogStatsdSinkFactory::createStatsSink(const Protobuf::Message& config,


Please make sure that the actual registration system has coverage. You could either add this to one of the v2 integration test configurations or add dedicated config loading tests here: https://github.com/envoyproxy/envoy/blob/master/test/server/config/stats/config_test.cc. Note that you can check your coverage in CI by opening up the CI coverage build and looking at the build artifacts in CircleCI.

Done fc4efda

Note that you can check your coverage in CI by opening up the CI coverage build and looking at the build artifacts in CircleCI.

I just didn't know, thanks!

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

mattklein123

LGTM, thanks!

taiki45 · 2017-12-12T05:07:26Z

Thank you all too!

* update envoy to latest Signed-off-by: Lizan Zhou <lizan@tetrate.io> * update envoy with latest build fixes (envoyproxy#2147) * fix build Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix build Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix formatting * fix status match Signed-off-by: Lizan Zhou <lizan@tetrate.io>

For envoyproxy/envoy#2158. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 added 3 commits December 5, 2017 22:59

stats: native DogStatsD support

816f4e6

Add new stats sink which supports tagged metrics with DogStatsD format. At the first moment, the sink only supports UDP output. Tested with stastd-exporter v0.5.0 and Prometheus v2.0.0. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

stats: point to temporary fork

22dc377

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

docs: add release note

15dcc1b

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 mentioned this pull request Dec 5, 2017

stats: add config and docs for dog statsd sink envoyproxy/data-plane-api#325

Merged

taiki45 added 2 commits December 6, 2017 00:07

stats: format with clang-format-5.0

d21429d

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Fix test class name

6781c05

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch reviewed Dec 5, 2017

View reviewed changes

taiki45 added 2 commits December 6, 2017 07:54

Use and extend existing statsd sink class

5937ac4

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Update SHA of data-plane-api

bca6d22

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

mattklein123 closed this Dec 6, 2017

mattklein123 reopened this Dec 6, 2017

Merge remote-tracking branch 'origin/master' into dog-statsd

ce479fc

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch reviewed Dec 6, 2017

View reviewed changes

taiki45 added 5 commits December 7, 2017 17:31

Use snake_case for local var name

ea076af

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Don't build tag string unless using tag

80560ce

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Use StringUtil::join

be26f9f

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Use snake_case for member variables

5c3dc9b

Also use `const` for bool. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

format

9c8675c

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch pushed a commit to envoyproxy/data-plane-api that referenced this pull request Dec 7, 2017

stats: add config and docs for dog statsd sink (#325)

ae65079

For envoyproxy/envoy#2158. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch reviewed Dec 7, 2017

View reviewed changes

taiki45 added 5 commits December 8, 2017 18:56

Pin to original data-plane-api

3db0bd7

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Omit unnecessary newline character

b664048

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Add test to check actual sent values to writer

e3487df

Remove process of building stats string from Writer and it now only holds a fd. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Use snake_case and const modifier

3338694

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Simplify buildTagStr()

3c38668

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 added 2 commits December 9, 2017 06:26

Merge remote-tracking branch 'origin/master' into dog-statsd

a948d09

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Add a comment

1b10cee

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

mattklein123 reviewed Dec 11, 2017

View reviewed changes

taiki45 added 8 commits December 11, 2017 12:39

Use const if available

d3fb9ef

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Use "" for more readability

0109362

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

s/tag_strs/tag_strings/

538e3a3

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Remove unnecessary end of ";"

75c51ab

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

clang-format

0d2a721

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Use more clear name

cd7592e

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Move default method mocks to constructor

85dab7f

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Tests for DogStatsdSinkFactory

fc4efda

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

mattklein123 approved these changes Dec 11, 2017

View reviewed changes

mattklein123 merged commit 112f6c6 into envoyproxy:master Dec 11, 2017

taiki45 deleted the dog-statsd branch December 12, 2017 05:07

taiki45 mentioned this pull request Dec 13, 2017

data-plane-api: extract trace protos #2202

Merged

jmarantz mentioned this pull request Jul 15, 2023

Envoy stats sink - flush time or max_bytes_per_datagram #28316

Closed

jmarantz mentioned this pull request Dec 29, 2024

DogStatsdSink lowers performance at higher CPU + concurrency #37831

Closed

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

stats: add config and docs for dog statsd sink (#325)

373069b

For envoyproxy/envoy#2158. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

Conversation

taiki45 commented Dec 5, 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!

taiki45 Dec 5, 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!

Choose a reason for hiding this comment

Uh oh!

taiki45 commented Dec 6, 2017

Uh oh!

mattklein123 commented Dec 6, 2017

Uh oh!

taiki45 commented Dec 6, 2017

Uh oh!

htuch commented Dec 6, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taiki45 Dec 7, 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!

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!

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!

taiki45 commented Dec 5, 2017 •

edited

Loading

taiki45 Dec 5, 2017 •

edited

Loading

taiki45 Dec 7, 2017 •

edited

Loading