config/stats: add udp statds address as config option #1019

hennna · 2017-05-25T21:57:30Z

This PR allows the statsd UDP server address to be set as a JSON config parameter.

To allow for unit testing, the udp statsd writer_ has been changed from a static thread_local object to being derived from the ThreadLocal object class.

In this PR, statsd_local_udp_port has been DEPRECATED and replaced with statsd_udp_ip_address.

Fixes #948 . Fixes part of #979 .

hennna · 2017-05-25T21:59:51Z

test/common/stats/udp_statsd_test.cc

+    socklen_t sock_len = sizeof(sockaddress);
+
+    EXPECT_EQ(0, getsockname(fd, reinterpret_cast<struct sockaddr*>(&sockaddress), &sock_len));
+    EXPECT_EQ("127.0.0.1", Network::Address::addressFromSockAddr(sockaddress, sizeof(sockaddr_in))


Is it expected that this is 127.0.0.1 and not 0.0.0.0?

I notice above we create the address as new Network::Address::Ipv4Instance(port) (which it was also doing before). This actually sets the destination IP as 0.0.0.0, which doesn't seem to make that sense to me (unless I'm missing something about UDP and local interfaces). The fact that this passes though seems to indicate it does get rebound? Maybe someone with more insight can comment here.

If you ping 0.0.0.0 you get

PING 0.0.0.0 (127.0.0.1) 56(84) bytes of data. 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.056 ms

So somehow it's getting converted.

htuch

Looks great @hennna, thanks.

htuch · 2017-05-26T03:10:08Z

include/envoy/server/configuration.h

@@ -130,12 +130,18 @@ class Main {
   */
  virtual Optional<std::string> statsdTcpClusterName() PURE;

+  // TODO(hennna): Deprecate in release: 1.4.0.


Nit: it's deprecated now, will be removed in 1.4.0.

htuch · 2017-05-26T03:10:39Z

DEPERECATED.md

@@ -0,0 +1,3 @@
+# DEPRECATED starting from 1.3.0


Maybe DEPRECATED (will be removed in 1.4.0). @RomanDzhabarov thoughts?

Maybe put a top level description of what this file is for and what rules are (feature is removed starting from the specified version, etc)

And then having a list of features that will be removed in a specific version.

Description

Version 1.4.0:

item 1

item 2

Version 1.5.0:

..

etc

Update the file name DEPERECATED.md -> DEPRECATED.md now, otherwise we will have to deprecate the DEPERECATED.md at a later date!

htuch · 2017-05-26T03:10:56Z

include/envoy/server/configuration.h

  /**
   * @return Optional<uint32_t> the optional local UDP statsd port to write to.
   */
  virtual Optional<uint32_t> statsdUdpPort() PURE;

  /**
+   * @return Optional<uint32_t> the optional UDP statsd address to write to.


Stale comment type.

htuch · 2017-05-26T03:11:49Z

include/envoy/server/configuration.h

  /**
   * @return Optional<uint32_t> the optional local UDP statsd port to write to.
   */
  virtual Optional<uint32_t> statsdUdpPort() PURE;

  /**
+   * @return Optional<uint32_t> the optional UDP statsd address to write to.
+   */
+  virtual Optional<std::string> statsdUdpIpAddress() PURE;


Should this be a string or Network::Address::InstanceConstSharedPtr?

This is string and has to do with parsing the JSON config file.

htuch · 2017-05-26T03:13:21Z

source/common/stats/statsd.cc

+void Writer::shutdown() {
+  shutdown_ = true;
+  if (fd_ != -1) {
+    ASSERT(close(fd_) == 0);


This should be RELEASE_ASSERT or it will get compiled out in release builds.

htuch · 2017-05-26T03:17:09Z

source/server/server.cc

    log().info("statsd UDP port: {}", config_->statsdUdpPort().value());
-    stat_sinks_.emplace_back(new Stats::Statsd::UdpStatsdSink(config_->statsdUdpPort().value()));
+    stat_sinks_.emplace_back(
+        new Stats::Statsd::UdpStatsdSink(thread_local_, config_->statsdUdpPort().value()));


You could localize the deprecation to this method if you switch the UdpStatsdSink constructor to just take an Address.

htuch · 2017-05-26T03:18:42Z

test/common/stats/udp_statsd_test.cc

+    socklen_t sock_len = sizeof(sockaddress);
+
+    EXPECT_EQ(0, getsockname(fd, reinterpret_cast<struct sockaddr*>(&sockaddress), &sock_len));
+    EXPECT_EQ("127.0.0.1", Network::Address::addressFromSockAddr(sockaddress, sizeof(sockaddr_in))


I notice above we create the address as new Network::Address::Ipv4Instance(port) (which it was also doing before). This actually sets the destination IP as 0.0.0.0, which doesn't seem to make that sense to me (unless I'm missing something about UDP and local interfaces). The fact that this passes though seems to indicate it does get rebound? Maybe someone with more insight can comment here.

RomanDzhabarov · 2017-05-26T18:26:25Z

DEPRECATED.md

+The following features have been DEPRECATED and will be removed in the specified release cycle.
+
+* Version 1.4.0
+  * Config option `statsd_local_udp_port` has been deprecated and has been replaced with


thanks @hennna I'll update this with 3a466c3
once you merge it

htuch

LGTM

htuch · 2017-05-26T18:51:23Z

source/common/stats/statsd.h

  ~Writer();

  void writeCounter(const std::string& name, uint64_t increment);
  void writeGauge(const std::string& name, uint64_t value);
  void writeTimer(const std::string& name, const std::chrono::milliseconds& ms);
+  void shutdown() override;
+  // Called in unit test to validate address.
+  int getFdForTests() { return fd_; };


htuch · 2017-05-26T18:52:53Z

test/common/stats/udp_statsd_test.cc

+    EXPECT_EQ("127.0.0.1", Network::Address::addressFromFd(fd)->ip()->addressAsString());
+  } else {
+    EXPECT_EQ("::1", Network::Address::addressFromFd(fd)->ip()->addressAsString());
+  }


What about checking expected port?

This need to be updated to getpeername. Will fix.

htuch · 2017-05-26T18:53:26Z

test/config/integration/server.json

@@ -232,7 +232,7 @@

  "admin": { "access_log_path": "/dev/null", "profile_path": "{{ test_tmpdir }}/envoy.prof", "address": "tcp://127.0.0.1:0" },
  "flags_path": "/invalid_flags",
-  "statsd_local_udp_port": 8125,
+  "statsd_udp_ip_address": "127.0.0.1:0",


It's a bit weird specifying a 0 send port, but sure, why not, it at least points out this is distinguished.

RomanDzhabarov · 2017-05-26T18:59:24Z

docs/configuration/overview/overview.rst

@@ -45,9 +45,11 @@ flags_path
  *(optional, string)* The file system path to search for :ref:`startup flag files
  <operations_file_system_flags>`.

-statsd_local_udp_port


shall we keep this until it's removed from the code? (and add deprecation warning)

Sure. Will add it back.

RomanDzhabarov · 2017-05-26T19:23:54Z

source/common/stats/statsd.h

-
-  uint32_t port_;
+  ThreadLocal::Instance& tls_;
+  uint32_t tls_slot_;


RomanDzhabarov · 2017-05-26T19:28:53Z

docs/configuration/overview/overview.rst

-statsd_local_udp_port
-  *(optional, integer)* The UDP port of a locally running statsd compliant listener. If specified,
-  :ref:`statistics <arch_overview_statistics>` will be flushed to this port.
+statsd_udp_ip_address


what happens if both specified?

It will choose ip_address over port in server.cc

wondering if we just should throw to avoid confusion when we checked whether one or another is set.
https://github.com/lyft/envoy/pull/1019/files#diff-3eda0ff865e056f19775ad626a5c2920R321

RomanDzhabarov · 2017-05-26T19:40:10Z

source/common/stats/statsd.h


 private:
  void send(const std::string& message);

  int fd_;
+  bool shutdown_ = false;


for consistency, bool shutdown_{};

RomanDzhabarov · 2017-05-26T21:54:28Z

+1

jylin · 2017-05-26T22:02:29Z

Does this patch support a DNS address instead of an IP? If so, maybe just statsd_udp_remote_address is a better option name.

htuch · 2017-05-26T22:04:57Z

This doesn't support a DNS address, only IP:port. In general, Envoy only supports DNS resolutions today when redirecting via cluster resolution.

jylin · 2017-05-30T14:21:42Z

OK, although then using this option sort of forces you into a bad practice of hard-coding IP addresses. Would it make sense to try to make the "statsd_tcp_cluster_name" then support UDP too (i.e. a "statsd_udp_cluster_name")?

htuch · 2017-05-30T14:30:39Z

This seems reasonable from a config/API perspective, but will involve a bit of work to make work. Currently the cluster implementations such as LogicalDnsCluster are assuming TCP connections. So, we can leave open to track this but I don't think anyone is actively working on it. PRs are welcome.

htuch · 2017-05-30T14:34:02Z

This is also related to #492 - if there was general support for UDP proxying we would be able to leverage that to do have UDP addressable clusters for statsd.

htuch · 2017-05-30T14:38:12Z

Tracking in #1028.

Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>

Add udp statds address as config option

a4762be

hennna commented May 25, 2017

View reviewed changes

htuch reviewed May 26, 2017

View reviewed changes

hennna added 4 commits May 26, 2017 11:41

Response to comments

3b35c4b

Update docs

5c62e0d

Update

b22e78a

Use addressFromFd

9e7fda9

RomanDzhabarov reviewed May 26, 2017

View reviewed changes

hennna added 2 commits May 26, 2017 14:40

Fix

e897281

Fix

e35a479

htuch reviewed May 26, 2017

View reviewed changes

RomanDzhabarov reviewed May 26, 2017

View reviewed changes

source/common/stats/statsd.h Outdated

uint32_t port_;

ThreadLocal::Instance& tls_;

uint32_t tls_slot_;

Copy link

Member

RomanDzhabarov May 26, 2017

Choose a reason for hiding this comment

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

const

RomanDzhabarov reviewed May 26, 2017

View reviewed changes

RomanDzhabarov self-assigned this May 26, 2017

Response to comments

a64fad2

htuch approved these changes May 26, 2017

View reviewed changes

RomanDzhabarov merged commit a8c446a into envoyproxy:master May 26, 2017

RomanDzhabarov mentioned this pull request May 30, 2017

Create formal deprecation process #1012

Closed

htuch mentioned this pull request May 30, 2017

UDP cluster support for statsd addresses #1028

Open

hennna deleted the udp-stats-address branch May 31, 2017 16:26

mattklein123 unassigned RomanDzhabarov Sep 14, 2017

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

release: tag 0.3.1.08112020 (#1019)

81e7a21

Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>

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

release: tag 0.3.1.08112020 (#1019)

8a1eb13

Signed-off-by: Michael Rebello <[email protected]> Signed-off-by: JP Simard <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config/stats: add udp statds address as config option #1019

config/stats: add udp statds address as config option #1019

hennna commented May 25, 2017 •

edited

Loading

hennna May 25, 2017

htuch May 26, 2017

hennna May 26, 2017

htuch left a comment

htuch May 26, 2017

htuch May 26, 2017

RomanDzhabarov May 26, 2017

dnoe May 26, 2017

htuch May 26, 2017

htuch May 26, 2017

hennna May 26, 2017

htuch May 26, 2017

htuch May 26, 2017

htuch May 26, 2017

RomanDzhabarov May 26, 2017

htuch left a comment

htuch May 26, 2017

htuch May 26, 2017

hennna May 26, 2017

htuch May 26, 2017

RomanDzhabarov May 26, 2017

hennna May 26, 2017

RomanDzhabarov May 26, 2017

RomanDzhabarov May 26, 2017

hennna May 26, 2017

RomanDzhabarov May 26, 2017

RomanDzhabarov May 26, 2017

RomanDzhabarov commented May 26, 2017

jylin commented May 26, 2017

htuch commented May 26, 2017

jylin commented May 30, 2017

htuch commented May 30, 2017

htuch commented May 30, 2017

htuch commented May 30, 2017

config/stats: add udp statds address as config option #1019

config/stats: add udp statds address as config option #1019

Conversation

hennna commented May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanDzhabarov commented May 26, 2017

jylin commented May 26, 2017

htuch commented May 26, 2017

jylin commented May 30, 2017

htuch commented May 30, 2017

htuch commented May 30, 2017

htuch commented May 30, 2017

hennna commented May 25, 2017 •

edited

Loading