[envoy] new integration #1156

ofek · 2018-02-25T22:25:10Z

Notes

The 305 metrics were taken from:

Additional Notes

I've excluded undocumented metrics
We must exclude these and these until Stat name collisions envoyproxy/envoy#2683 is resolved.
To avoid enumerating every HTTP status code, I have intentionally excluded:
- upstream_rq_<*> from here and here
- each upstream_rq_total_xxx and upstream_rq_time_xxx from here
I've excluded experimental 3rd party metrics:
- DynamoDB capacity.<operation_name>.__partition_id=<last_seven_characters_from_partition_id> from here
I've excluded deprecated metrics:
- cluster.outlier_detection.ejections_total and cluster.outlier_detection.ejections_consecutive_5xx from here

Team

In an effort to simplify things for the new testing approach:

flake8 configuration is now in tox.ini, eliminating the need for setup.cfg.
tests/requirements.txt is now requirements-dev.txt.

Review

@DataDog/croissant

and

@mattklein123 @htuch @mrice32 @jmarantz Please feel free to chime in. You would be particularly interested in:

the parser (implementation is the same as discussed in Slack)
https://github.com/DataDog/integrations-core/pull/1156/files#diff-26242bcecd44a7dd35184c58c4813ed9

our descriptions of Envoy (tell me what you'd like it to say)
https://github.com/DataDog/integrations-core/pull/1156/files#diff-80bc075ec660cedefbacfbb08a41c682R5
https://github.com/DataDog/integrations-core/pull/1156/files#diff-4d2ed940a01536f3edd5499b23f267ccR4

htuch · 2018-03-08T01:34:47Z

envoy/conf.yaml.example

+  # supply a list of tags. The admin endpoint must be accessible.
+  # https://www.envoyproxy.io/docs/envoy/latest/operations/admin
+
+  - stats_url: http://localhost:80/stats


I thought we had a stats sink for emitting to DataDog, https://github.com/envoyproxy/data-plane-api/blob/master/envoy/config/metrics/v2/stats.proto#L157, this is a push model. How come we have pull here? I'd echo the same security concerns as in envoyproxy/data-plane-api#523. CC: @taiki45

edit: fixed tag to Taiki

Hi @htuch and @taiki45,

Thanks for sharing your concerns with us! We understand them, and this is what we are doing at the moment to address them:

Until Admin endpoint security envoyproxy/envoy#2763 is resolved, we are offering our customers an alternative way to access Envoy's /stats endpoint.

Our envoy integration supports SSL verification of the Envoy endpoint (and this requires Envoy to support SSL). If the SSL verification fails for any reason when enabled, the integration will not collect metrics.

Our integration has the option to support basic authentication (but this requires Envoy to implement basic authentication).

Our integration does not need to be aware of RBAC, but when Envoy implements it, our integration will use it transparently (via basic authentication).

At the moment, we do not see the benefit of supporting the dogstatsd connector, because that still does not prevent the admin endpoint on Envoy from being exposed to the entire trusted network. A customer could use that to be more secure in some network scenarios, but we would also like to provide our customers with a working agent integration for Envoy in its current form.

We also need to do some massaging of metrics in the integration for our backend, which the dogstatsd connector currently does not allow. Additionally, we are finding our parser a bit more resilient to stat name collisions. @jmarantz mentioned that he has a similar implementation for which he’ll issue a PR to Envoy soon.

We would be happy to enforce authentication by default, provided that Envoy implements it. We hope this addresses your concerns. Thanks again for sharing your thoughts with us!

Regards,
The Agent team at Datadog

Yeah, this all seems very reasonable. I'd add a couple of points for consideration:

With your example at https://github.com/DataDog/integrations-core/tree/ofek/envoy/envoy#secured-stats-endpoint, should you have the admin bind only to 127.0.0.1 to avoid incoming connections from NICs?

I probably worded the above a little strongly; until Admin endpoint security envoyproxy/envoy#2763 is resolved we don't have a great story for access outside a trusted network (although I like your example as a temporary workaround, you could post it to Admin endpoint security envoyproxy/envoy#2763 once the above fix is made). We've recently been discussing this in Documentation for hystrix dashboard support feature (issue #1244) envoyproxy/data-plane-api#523 for Hystrix and agreed to separate out the admin security concerns from pull-based stats.

@htuch That's an interesting idea. Would I simply change "address": "tcp://0.0.0.0:8001" to "address": "tcp://127.0.0.1:8001"?

@ofek Yes, that's right. And I assume that prevents the entire admin endpoints from being exposed.

I agree to move to pull-based stats because it can be easily scaled than push-based one, but I'd leave some comments to clarify the points.

At the moment, we do not see the benefit of supporting the dogstatsd connector, because that still does not prevent the admin endpoint on Envoy from being exposed to the entire trusted network. A customer could use that to be more secure in some network scenarios, but we would also like to provide our customers with a working agent integration for Envoy in its current form.

AFAIK, the datadog agent will be running in the customer's host (the same host as running Envoy process). If it's correct, we might be able to send stats to datadog agent without opening any Envoy admin endpoints. The scenario is:

Configure Envoy admin endpoint to bind local loopback address (127.0.0.1 in IPv4).

Send Envoy stats to datadog agent running on the same host using UDP with dog_statsd sink.

We also need to do some massaging of metrics in the integration for our backend, which the dogstatsd connector currently does not allow.

I suppose you can do that modifications with writing a statsd relay component which receives stats and modifies the stats then re-pushes to somewhere (probably datadog agent?).

IIUC, the current admin's /stats endpoint is missing histogram metrics. envoyproxy/envoy#1947 This can be resolved with envoyproxy/envoy#2736 which will implement pull-based stats with stats sink (I didn't fully take a look the PR yet).

gmmeyer

I have a lot more to review, it'll probably come in stages, but these are the first two things that need to be fixed

gmmeyer · 2018-03-08T18:18:32Z

envoy/requirements.in

@@ -0,0 +1 @@
+requests==2.18.4


you need to generate the requirements.txt through the general mechanism we use for this: pip-compile --generate-hashes --output-file requirements.txt requirements.in

this is a requirement conflict with datadog-agent-base and a lot of other checks. Please use the version we're already using. If you want to upgrade, you can do that in another PR as it requires a good bit of work to ensure there aren't any conflicts.

masci

Left few comments and a couple of questions but overall this is extremely good material! Tests are 💯with high coverage, 🍰

masci · 2018-03-12T13:19:06Z

tasks.py

+    'disk',
+    'kubelet',
+    'vsphere',
+    'envoy',


nit: let's try to keep the list alphabetically sorted for better readability

masci · 2018-03-12T13:21:47Z

envoy/datadog_checks/envoy/envoy.py

+            self.log.warning(msg)
+            return
+
+        get_method = getattr


just curious: why this alias?

To avoid a global lookup many times during the loop.

Is it that much slower?

Ok for me, mind to add a comment above that line? So it doesn't get wiped by mistake in the future.

masci · 2018-03-12T13:26:35Z

envoy/datadog_checks/envoy/errors.py

@@ -0,0 +1,2 @@
+class UnknownMetric(Exception):


nit: can we add the license header?

masci · 2018-03-12T13:26:49Z

envoy/datadog_checks/envoy/metrics.py

@@ -0,0 +1,1228 @@
+from .utils import make_metric_tree


nit: license header

masci · 2018-03-12T13:28:21Z

envoy/datadog_checks/envoy/parser.py

+from .metrics import METRIC_PREFIX, METRIC_TREE, METRICS
+
+
+def parse_metric(metric):


can we add a docstring roughly describing what this function does?

masci · 2018-03-12T13:29:46Z

envoy/datadog_checks/envoy/parser.py

+    return METRIC_PREFIX + metric, tags, METRICS[metric]['method']
+
+
+def reassemble_addresses(seq):


can we add a docstring roughly describing what this function does?

masci · 2018-03-12T13:32:04Z

envoy/datadog_checks/envoy/utils.py

+
+    for metric in metrics:
+        parts = metric.split('.')
+        mapping = metric_tree


why the copy?

We make mapping reference the root of the tree at every iteration to create the new branches.

gmmeyer

I think everything I was worried about has been addressed now! Once @masci's comments are addressed feel free to merge!

taiki45 · 2018-03-13T01:29:01Z

envoy/conf.yaml.example

+  - stats_url: http://localhost:80/stats
+
+  #   tags:
+  #     - instance:foo


In case you missed, probably you can use fixed string tags to distinguish Envoy instances. https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/metrics/v2/stats.proto#config-metrics-v2-tagspecifier I don't know the tag can work with admin /stats, but I'm using the fixed tag with dog_statsd sink. Please note the fixed tag feature is only available with Envoy >= v1.6.0.

Thanks! We actually allow users to specify any tag they wish, which, as you said, is especially useful for distinguishing between different instances of an integration for metric aggregation.

ofek added do-not-merge/WIP kind/new-integration labels Feb 25, 2018

add Envoy integration

bb281ac

ofek force-pushed the ofek/envoy branch from 9c56ee7 to bb281ac Compare March 7, 2018 06:59

add to gitlab ci

a6e3d7f

ofek changed the title ~~add Envoy integration~~ [envoy] new integration Mar 7, 2018

ofek requested review from masci, hush-hush, truthbk and gmmeyer March 7, 2018 07:38

ofek removed the do-not-merge/WIP label Mar 7, 2018

ofek added this to the 6.1 milestone Mar 7, 2018

ofek requested a review from a team March 7, 2018 16:53

htuch reviewed Mar 8, 2018

View reviewed changes

htuch mentioned this pull request Mar 8, 2018

Documentation for hystrix dashboard support feature (issue #1244) envoyproxy/data-plane-api#523

Closed

gmmeyer suggested changes Mar 8, 2018

View reviewed changes

ofek added 8 commits March 8, 2018 15:18

update reqs

6c60a2e

only log one debug per unknown metric

6b37fc2

add test

440f683

add more config info

7df7799

fix constructor...

a1aae8b

add note

9c6bcd2

add secured stats endpoint option

099b15f

add example config to readme

c0ede9e

masci reviewed Mar 12, 2018

View reviewed changes

ofek added 4 commits March 12, 2018 10:59

alphabetical

352d30c

add license headers

5240eac

add docstring

c89dac8

add docstring

8685c92

gmmeyer previously approved these changes Mar 12, 2018

View reviewed changes

add comment

a26832d

ofek dismissed gmmeyer’s stale review via a26832d March 12, 2018 20:54

gmmeyer approved these changes Mar 13, 2018

View reviewed changes

ofek merged commit 02d8d55 into master Mar 13, 2018

ofek deleted the ofek/envoy branch March 13, 2018 00:03

taiki45 reviewed Mar 13, 2018

View reviewed changes

ofek mentioned this pull request Mar 13, 2018

Admin endpoint security envoyproxy/envoy#2763

Open

masci modified the milestones: 6.1, 6.1/5.23 Mar 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[envoy] new integration #1156

[envoy] new integration #1156

ofek commented Feb 25, 2018 •

edited

Loading

htuch Mar 8, 2018 •

edited by ofek

Loading

ofek Mar 12, 2018 •

edited

Loading

htuch Mar 12, 2018

ofek Mar 12, 2018

taiki45 Mar 13, 2018

htuch Mar 13, 2018

gmmeyer left a comment

gmmeyer Mar 8, 2018

masci left a comment

masci Mar 12, 2018

masci Mar 12, 2018

ofek Mar 12, 2018

gmmeyer Mar 12, 2018

masci Mar 12, 2018

masci Mar 12, 2018

ofek Mar 12, 2018

masci Mar 12, 2018

ofek Mar 12, 2018

masci Mar 12, 2018

ofek Mar 12, 2018

masci Mar 12, 2018

ofek Mar 12, 2018

masci Mar 12, 2018

ofek Mar 12, 2018

gmmeyer left a comment

taiki45 Mar 13, 2018

ofek Mar 13, 2018 •

edited

Loading

		from .metrics import METRIC_PREFIX, METRIC_TREE, METRICS


		def parse_metric(metric):

		return METRIC_PREFIX + metric, tags, METRICS[metric]['method']


		def reassemble_addresses(seq):

		@@ -0,0 +1 @@
		requests==2.18.4

[envoy] new integration #1156

[envoy] new integration #1156

Conversation

ofek commented Feb 25, 2018 • edited Loading

Notes

Additional Notes

Team

Review

htuch Mar 8, 2018 • edited by ofek Loading

Choose a reason for hiding this comment

ofek Mar 12, 2018 • edited Loading

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

gmmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masci 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

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

gmmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofek Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

ofek commented Feb 25, 2018 •

edited

Loading

htuch Mar 8, 2018 •

edited by ofek

Loading

ofek Mar 12, 2018 •

edited

Loading

ofek Mar 13, 2018 •

edited

Loading