mongo_proxy: support configurable command list for metrics by bartle-stripe · Pull Request #13494 · envoyproxy/envoy

bartle-stripe · 2020-10-11T02:58:54Z

Commit Message: mongo_proxy: support configurable command list for metrics

Additional Description:

This additionally removes query from the default list of commands, since that's not actually a MongoDB command. I'm not sure why it was ever added, but we never see any query command metrics (not be to be confused with the separate query namespace).
Hardcoding aliases (findandmodify => findAndModify, etc...) seems reasonable, and doesn't make sense to be configurable, because these aliases only exist for historical reasons, and newer MongoDB commands (afaik) don't have such aliases.

Risk Level: Low

Testing: Updated tests and tested internally at Stripe.

Docs Changes: Updated mongo_proxy config docs to mention new commands field.

Release Notes: mongo_proxy: the list of commands to produce metrics for is now configurable.

Fixes #13448

repokitteh-read-only · 2020-10-11T02:59:02Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13494 was opened by bartle-stripe.

see: more, trace.

mattklein123

Looks great, thanks! Just a few small comments.

/wait

mattklein123 · 2020-10-11T17:24:32Z

api/envoy/config/filter/network/mongo_proxy/v2/mongo_proxy.proto

v2 is locked/deprecated, so please remove these changes.

Hm, so I originally didn't include v2 in this change but I got test failures locally.

Specifically, a segfault in what looked to be an attempt to convert v3 into v2 config.

Anyway, removed, hopefully CI passes.

Looks good, I think I might have been testing this change internally on an older git commit.

mattklein123 · 2020-10-11T17:26:47Z

api/envoy/extensions/filters/network/mongo_proxy/v3/mongo_proxy.proto

Please add a release note.

I don't see a release note? Please also ref link to this field. You will need to merge main to pick up the CI change for docs also.

/wait

Sorry, should be there now.

Fixes envoyproxy#13448 Signed-off-by: David Bartley <bartle@stripe.com>

mattklein123 · 2020-10-12T14:44:29Z

(Friendly request to please never force push. It makes reviews much more difficult. Thank you!)

Signed-off-by: David Bartley <bartle@stripe.com>

mattklein123 · 2020-10-12T17:36:26Z

@bartle-stripe please stop force pushing! Thanks!

bartle-stripe · 2020-10-12T19:40:05Z

Sorry, the instructions on the DCO check told me to force push my last commit because I didn't include Signed-off-by.

mattklein123

LGTM with small nit, thanks!

/wait

mattklein123 · 2020-10-12T21:20:52Z

source/extensions/filters/network/mongo_proxy/config.cc

+  if (proto_config.commands_size() > 0) {
+    commands =
+        std::vector<std::string>(proto_config.commands().begin(), proto_config.commands().end());
+    ;


nit: errant ';'

Good catch, fixed!

Signed-off-by: David Bartley <bartle@stripe.com>

mattklein123

Thanks!

mattklein123 · 2020-10-12T22:55:58Z

Apologies this will need another main merge.

/wait

…configurable-commands

Signed-off-by: David Bartley <bartle@stripe.com>

mattklein123

Thanks!

* master: (22 commits) http: using CONNECT_ERROR for HTTP/2 (envoyproxy#13519) listener: respect address.pipe.mode (it didn't work) (envoyproxy#13493) examples: Fix more deprecations/warnings in configs (envoyproxy#13529) overload: tcp connection refusal overload action (envoyproxy#13311) tcp: towards pluggable upstreams (envoyproxy#13331) conn_pool: fixing comments (envoyproxy#13520) Prevent SEGFAULT when disabling listener (envoyproxy#13515) Convert overload manager config literals to YAML (envoyproxy#13518) Fix runtime feature variable name (envoyproxy#13533) dependencies: refactor repository location schema utils, cleanups. (envoyproxy#13452) router: fix an invalid ASSERT when encoding metadata frames in the router. (envoyproxy#13511) http2: Proactively disconnect connections flooded when resetting stream (envoyproxy#13482) ci use azp to sync filter example (envoyproxy#13501) mongo_proxy: support configurable command list for metrics (envoyproxy#13494) http local rate limit: note token bucket is shared (envoyproxy#13525) wasm/extensions: Wasm extension policy. (envoyproxy#13526) http: removing envoy.reloadable_features.http1_flood_protection (envoyproxy#13508) build: update ppc64le CI build status shield (envoyproxy#13521) dependencies: enforce dependency shepherd sign-off via RepoKitteh. (envoyproxy#13522) Add no_traffic_healthy_interval (envoyproxy#13336) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

repokitteh-read-only bot added v2-freeze api labels Oct 11, 2020

bartle-stripe force-pushed the bartle-mongo_proxy-configurable-commands branch 3 times, most recently from 32becca to 22721a0 Compare October 11, 2020 10:13

mattklein123 self-assigned this Oct 11, 2020

mattklein123 requested changes Oct 11, 2020

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Oct 11, 2020

mongo_proxy: support configurable command list for metrics

ad9cdee

Fixes envoyproxy#13448 Signed-off-by: David Bartley <bartle@stripe.com>

bartle-stripe force-pushed the bartle-mongo_proxy-configurable-commands branch from 909674e to ad9cdee Compare October 11, 2020 21:11

repokitteh-read-only bot added the waiting label Oct 12, 2020

Add release notes.

95ed47a

Signed-off-by: David Bartley <bartle@stripe.com>

repokitteh-read-only bot removed the waiting label Oct 12, 2020

Fix wording.

fd09ca3

Signed-off-by: David Bartley <bartle@stripe.com>

bartle-stripe force-pushed the bartle-mongo_proxy-configurable-commands branch from dce2228 to fd09ca3 Compare October 12, 2020 16:59

mattklein123 requested changes Oct 12, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 12, 2020

Remove extraneous semi-colon.

0c6ebb6

Signed-off-by: David Bartley <bartle@stripe.com>

repokitteh-read-only bot removed the waiting label Oct 12, 2020

mattklein123 previously approved these changes Oct 12, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Oct 12, 2020

repokitteh-read-only bot added the waiting label Oct 12, 2020

Merge remote-tracking branch 'origin/master' into bartle-mongo_proxy-…

e8c7d95

…configurable-commands

bartle-stripe dismissed mattklein123’s stale review via e8c7d95 October 12, 2020 23:02

repokitteh-read-only bot added api and removed waiting labels Oct 12, 2020

Fix release notes ordering.

f61a3f4

Signed-off-by: David Bartley <bartle@stripe.com>

mattklein123 approved these changes Oct 13, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Oct 13, 2020

mattklein123 merged commit 7857873 into envoyproxy:master Oct 13, 2020

bartle-stripe deleted the bartle-mongo_proxy-configurable-commands branch October 13, 2020 01:31

Conversation

bartle-stripe commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 11, 2020

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!

mattklein123 commented Oct 12, 2020

Uh oh!

mattklein123 commented Oct 12, 2020

Uh oh!

bartle-stripe commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Oct 12, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bartle-stripe commented Oct 11, 2020 •

edited

Loading

bartle-stripe commented Oct 12, 2020 •

edited

Loading