stats: add config and docs for dog statsd sink by taiki45 · Pull Request #325 · envoyproxy/data-plane-api

taiki45 · 2017-12-05T14:59:18Z

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

Review Note:
I'm not a fluent English speaker.

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

htuch · 2017-12-05T15:12:49Z

api/bootstrap.proto

+// Stats configuration proto schema for built-in *envoy.dog_statsd* sink.
+// The sink emits stats with `DogStatsD <https://docs.datadoghq.com/guides/dogstatsd/>`_
+// compatible tags. Tags are configurable via :ref:`StatsConfig <envoy_api_msg_StatsConfig>`.
+message DogStatsdSink {


We probably should have a api/stats directory and move the pluggable stats to their own protos there. Same goes for api/trace. Doesn't have to be done in this PR, but if there is agreement amongst maintainers, we can file an issue.

+1. Probably in api/stats or api/stats_sink?

+1, do you mind just doing that in this PR?

Sure. Done in 2acb78b. If it's OK, I'll do the same thing for api/trace in another PR.

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

mattklein123

nice, very cool.

mattklein123 · 2017-12-05T17:05:21Z

api/bootstrap.proto

+// Stats configuration proto schema for built-in *envoy.dog_statsd* sink.
+// The sink emits stats with `DogStatsD <https://docs.datadoghq.com/guides/dogstatsd/>`_
+// compatible tags. Tags are configurable via :ref:`StatsConfig <envoy_api_msg_StatsConfig>`.
+message DogStatsdSink {


+1, do you mind just doing that in this PR?

mattklein123 · 2017-12-05T17:05:34Z

api/bootstrap.proto

+// The sink emits stats with `DogStatsD <https://docs.datadoghq.com/guides/dogstatsd/>`_
+// compatible tags. Tags are configurable via :ref:`StatsConfig <envoy_api_msg_StatsConfig>`.
+message DogStatsdSink {
+  // The UDP address of a running DogStatsD compliant listener. If specified,


Does datadog support TCP also?

It seems dd-agent (the datadog's agent) doesn't support TCP: https://github.com/DataDog/dd-agent/wiki/Agent-Architecture#supervision-privileges-and-network-ports

statsd_exporter does: https://github.com/prometheus/statsd_exporter#building-and-running

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

Since both messages are related with each other, need to gather into one place. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch · 2017-12-05T21:31:24Z

api/bootstrap.proto

+  // can be found in :ref:`Stats sinks<envoy_api_file_api/stats.proto>` or
+  // `well_known_names.h
+  // <https://github.com/envoyproxy/envoy/blob/master/source/common/config/well_known_names.h>`_
+  // in the Envoy repository.


We can move this proto as well into stats.proto.

Got it. Do you think StatsConfig can move into stats.proto as well?

Yeah I would move it also.

Done 7ee4f1e.

htuch · 2017-12-05T21:32:26Z

api/stats.proto

+message DogStatsdSink {
+  // The UDP address of a running DogStatsD compliant listener. If specified,
+  // statistics will be flushed to this address.
+  Address address = 1 [(validate.rules).message.required = true];


Out of curiosity, can DogStatsD do TCP as well?

We can flush DogStatsD messages via TCP as well as UDP. statsd_exporter which suports DogStatsD extension listens both TCP and UDP port. But original datadog's dd-agent doesn't listen TCP port: https://github.com/DataDog/dd-agent/wiki/Agent-Architecture#supervision-privileges-and-network-ports

I would consider making this a oneof, so that this can grow in future to include TCP..

+1 3c8e694

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

htuch · 2017-12-05T22:33:25Z

api/stats.proto

@@ -0,0 +1,150 @@
+// [#protodoc-title: Stats sinks]


Nit: not just sinks now.

Right. 0fbe7b3

htuch · 2017-12-05T22:34:27Z

api/stats.proto

+message DogStatsdSink {
+  // The UDP address of a running DogStatsD compliant listener. If specified,
+  // statistics will be flushed to this address.
+  Address address = 1 [(validate.rules).message.required = true];


I would consider making this a oneof, so that this can grow in future to include TCP..

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

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

htuch · 2017-12-06T02:24:12Z

Thanks @taiki45, LGTM. Will let @mattklein123 merge if he's happy with the final docs.

taiki45 · 2017-12-06T04:34:22Z

@htuch Thank you for reviewing this!

mattklein123 · 2017-12-07T14:10:31Z

LGTM, thank you! Sorry for the late review.

taiki45 · 2017-12-08T09:53:59Z

No problem, thanks!

This is a follow-up PR of #325. I mistook to add an unnecessary field here. What actually we need is just making dog_statsd_specifier oneof field, not adding tcp_cluster_name field. We can safely drop this field because users has ended up with initializing errors if they had specified this field. [critical][main] source/server/server.cc:71] error initializing configuration '/envoy.yaml': Address must be a socket or pipe This field has never been used in envoy repo: https://github.com/envoyproxy/envoy/blob/7d03b231c7e8e086956096f6ca961f82c684ed0b/source/server/config/stats/dog_statsd.cc#L19-L23 Signed-off-by: Taiki Ono <taiki-ono@cookpad.com>

stats: add config and docs for dog statsd sink

003d8a9

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

taiki45 mentioned this pull request Dec 5, 2017

stats: native DogStatsD support envoyproxy/envoy#2158

Merged

htuch reviewed Dec 5, 2017

View reviewed changes

Fix format with ./tools/check_format.py

2d4b900

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

mattklein123 reviewed Dec 5, 2017

View reviewed changes

taiki45 added 2 commits December 6, 2017 05:39

Extract stats sink proto

2acb78b

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

Move StatsConfig right after StatsSink

2ffc033

Since both messages are related with each other, need to gather into one place. Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

htuch reviewed Dec 5, 2017

View reviewed changes

Move all stats related messages into stats.proto

7ee4f1e

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:48

Now we have not only stats sinks

0fbe7b3

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

Use oneof in DogStatsdSink

3c8e694

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

htuch approved these changes Dec 6, 2017

View reviewed changes

htuch merged commit ae65079 into envoyproxy:master Dec 7, 2017

taiki45 deleted the dog-statsd branch December 8, 2017 09:53

taiki45 mentioned this pull request Dec 13, 2017

api: extract tracing protos from bootstrap #342

Merged

taiki45 mentioned this pull request Mar 14, 2018

docs: drop unnecessary field in DogStatsdSink message #545

Merged

Conversation

taiki45 commented Dec 5, 2017

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!

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!

htuch commented Dec 6, 2017

Uh oh!

taiki45 commented Dec 6, 2017

Uh oh!

mattklein123 commented Dec 7, 2017

Uh oh!

taiki45 commented Dec 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants