feat(stats): support grpc status codes in metrics by douglas-reid · Pull Request #2624 · istio/proxy

douglas-reid · 2020-01-14T01:02:35Z

Companion PR to istio/istio#20036.

This PR implements the changes approved in https://tinyurl.com/yy2cwmv5 to report gRPC status codes in metrics.

TODO:

update envoywasm to provide response.grpc_status (https://sourcegraph.com/github.com/envoyproxy/envoy-wasm@542558a722e23225cb1f6e2c8194c093f993f9c8/-/blob/source/extensions/filters/common/expr/context.cc#L124-144)
update logs to match other PR
add grpc-echo client/server for testing
update tags in istio/istio to match

PiotrSikora · 2020-01-24T19:03:00Z

@douglas-reid do you want to redo it now on top of #2631 in order to get it into 1.5?

douglas-reid · 2020-01-24T19:07:38Z

yeah, i'll need to rebase, etc.

PiotrSikora · 2020-01-24T21:55:14Z

@douglas-reid it's merged now.

douglas-reid · 2020-01-30T17:35:40Z

@kyessenov @mandarjog please take a look at this PR now.

There are a few important parts, but mostly, a decision is needed regarding the default behavior of grpc_response_status label for non-gRPC requests.

The original proposal design doc was under-specified in this regard.

In this PR, if the protocol is not gRPC, then the fallback will be the UNKNOWN grpc status code (2). This matches the behavior for translating HTTP OK -> gRPC codes (doc).

From http-grpc-status-mapping:

200 is UNKNOWN because there should be a grpc-status in case of truly OK response.

The spec for UNKNOWN, includes:

Unknown error. For example, this error may be returned when a Status value received from another address space belongs to an error space that is not known in this address space. Also errors raised by APIs that do not return enough error information may be converted to this error.

as well as

Error parsing returned status | UNKNOWN | Client

We had previously discussed the following alternatives:

Returning 1 (gRPC OK)
Returning an empty label.

I rejected returning gRPC OK, as that conflicts with the mapping.

Returning an empty label is slightly clunky, as the code gets much more complicated, as do the regexes for generating prometheus labels and the weirdness of having non-populated labels for non-gRPC metrics.

Thoughts?

Note: this PR is blocked on a cherry-pick of @kyessenov's hardening PR of upstream Envoy.

douglas-reid · 2020-01-30T18:59:33Z

Update: After thinking about this again, I switched to grpc_response_status: "" (empty string) for non-gRPC protocol cases. Please let me know what you think.

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid · 2020-02-04T21:04:08Z

/test test-asan_proxy

douglas-reid · 2020-02-04T21:04:18Z

/test test_proxy

douglas-reid · 2020-02-04T21:04:28Z

/test test-tsan_proxy

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

kyessenov · 2020-02-05T20:16:08Z

src/envoy/http/alpn/alpn_test.cc

    FilterConfig_AlpnOverride;
 using testing::NiceMock;
 using testing::Return;
+using testing::ReturnPointee;


nit: not used

kyessenov

LG. I think we need to de-duplicate all the templates at some point. It's PITA to update them once the deprecated fields stop working.

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid · 2020-02-05T22:21:19Z

@kyessenov agreed on the templates. deprecation will hit us hard.

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

* feat(stats): support grpc status codes in metrics (#2624) * fix(stats): remove policy-related dimensions (#2647) * feat(stats): add support for canonical service labels (#2658) * update istio/envoy SHA Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * add missing deps for cel Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * buildifier lint fix WORKSPACE respositories.bzl Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * restore file test path * commenting out RBE stuff Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * skip http mxc test Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid added the do-not-merge Block automatic merging of a PR. label Jan 14, 2020

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 14, 2020

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 14, 2020

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 14, 2020

This was referenced Jan 14, 2020

feat(context): add suppport for getting the grpc_status code envoyproxy/envoy-wasm#372

Closed

feat(context): add response.grpc_status envoyproxy/envoy#9693

Merged

douglas-reid force-pushed the grpc-status-codes branch from 0c91848 to 8ac4ea3 Compare January 30, 2020 17:16

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 30, 2020

douglas-reid force-pushed the grpc-status-codes branch 2 times, most recently from d9fcd65 to 89c156e Compare January 30, 2020 18:58

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 4, 2020

douglas-reid added 5 commits February 4, 2020 10:24

feat(stats): support grpc status codes in metrics

6f11657

wip

f68e1c8

add tests and fix up context

69cd580

set empty grpc_response_code

ef2a6c3

use latest envoyproxy/envoy-wasm

24341dc

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid force-pushed the grpc-status-codes branch from fbf4f8a to 24341dc Compare February 4, 2020 18:29

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 4, 2020

douglas-reid added 2 commits February 4, 2020 10:45

add license/copyright banner

ced4d85

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

fix lint / format / malign issues

75524d6

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

fix up alpn_test.cc

2418eb6

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2020

douglas-reid added 2 commits February 5, 2020 09:39

fix lint

82c719f

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

more tests needed updating with envoy update

c208bf8

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid marked this pull request as ready for review February 5, 2020 18:42

douglas-reid requested a review from a team February 5, 2020 18:42

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 5, 2020

stackdriver fix

3eb9466

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

kyessenov reviewed Feb 5, 2020

View reviewed changes

kyessenov self-requested a review February 5, 2020 20:21

kyessenov reviewed Feb 5, 2020

View reviewed changes

kyessenov approved these changes Feb 5, 2020

View reviewed changes

douglas-reid added 2 commits February 5, 2020 14:18

fix stackdriver onConfigure

584e8f7

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

remove unused using clause

38d1908

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

clang-format

9e82a96

Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

douglas-reid removed the do-not-merge Block automatic merging of a PR. label Feb 6, 2020

istio-testing merged commit cdf3692 into istio:master Feb 6, 2020

douglas-reid added a commit to douglas-reid/proxy that referenced this pull request Feb 7, 2020

feat(stats): support grpc status codes in metrics (istio#2624)

5d061b6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stats): support grpc status codes in metrics#2624

feat(stats): support grpc status codes in metrics#2624
istio-testing merged 14 commits intoistio:masterfrom
douglas-reid:grpc-status-codes

douglas-reid commented Jan 14, 2020 •

edited

Loading

Uh oh!

PiotrSikora commented Jan 24, 2020

Uh oh!

douglas-reid commented Jan 24, 2020

Uh oh!

PiotrSikora commented Jan 24, 2020

Uh oh!

douglas-reid commented Jan 30, 2020

Uh oh!

douglas-reid commented Jan 30, 2020

Uh oh!

douglas-reid commented Feb 4, 2020

Uh oh!

douglas-reid commented Feb 4, 2020

Uh oh!

douglas-reid commented Feb 4, 2020

Uh oh!

kyessenov Feb 5, 2020

Uh oh!

kyessenov left a comment

Uh oh!

douglas-reid commented Feb 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

douglas-reid commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrSikora commented Jan 24, 2020

Uh oh!

douglas-reid commented Jan 24, 2020

Uh oh!

PiotrSikora commented Jan 24, 2020

Uh oh!

douglas-reid commented Jan 30, 2020

Uh oh!

douglas-reid commented Jan 30, 2020

Uh oh!

douglas-reid commented Feb 4, 2020

Uh oh!

douglas-reid commented Feb 4, 2020

Uh oh!

douglas-reid commented Feb 4, 2020

Uh oh!

kyessenov Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

douglas-reid commented Feb 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

douglas-reid commented Jan 14, 2020 •

edited

Loading