feat(grpc status codes): report grpc status codes via mixer by douglas-reid · Pull Request #20036 · istio/istio

douglas-reid · 2020-01-09T17:33:05Z

This PR implements the changes approved in https://tinyurl.com/yy2cwmv5 to report gRPC status codes in metrics (and provide better alignment with logs and metrics when reporting protocol). It also cleans up unused metrics dimensions.

Other PRs will be required to support the same feature set in v2 telemetry and to update the operator/installer config templates.

Partial fix for #12846

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ X ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

kyessenov · 2020-01-09T19:50:43Z

install/kubernetes/helm/istio/charts/mixer/templates/config.yaml

why are these removed? cardinality issue?

For months I've been asking if these are used, and have yet to find anyone that is actually using these dimensions. I thought we were not carrying them forward into v2 (though the code now says otherwise).

@mandarjog objections to removing?

@liminw @yangminzhu can we remove these?

fwiw, kiali does not use these.

Yes please remove, @yangminzhu last chance to object.

Sorry I missed this PR. It should be fine to remove because the new authorization policy doesn't support the permissive mode but I'm not sure if it's used by anyone in the old RBAC policy for now. cc @liminw

kyessenov · 2020-01-09T19:52:01Z

install/kubernetes/helm/istio/charts/mixer/templates/config.yaml

is the status an int or a string? I think it's a string (by mistake)

apparently it is a string (based on the test failures). Is "0" the right default, or "unknown" ?

it's set if grpc status header is present, and not set at all otherwise (see src/envoy/http/mixer/report_data.h`)

right, but do we want the unset to be represented in metrics as "0" (like we default to "200" for http) or "unknown" ?

I think empty is better. It shouldn't really be unknown for grpc ever.

So just to confirm, the decision to have separate response_code and grpc_response_status fields is because even if request_protocol=grpc, response.code is still set?

Also, I think 0 is good for unset, just like the 200. I like a default being the same type as an actual value.

@douglas-reid I know the ship has sailed but perhaps the new field should have been "request_protocol_status" or something like that. And contained the response status for the specified request_protocol. Basically replacing request_code, which has now been relegated to effectively being "http_code". So existing consumer code could still assume http codes in request_code and write new code using the new field, which could support the two existing protocols and if necessary, additional protocols somewhere down the line. And "response_code" could have been deprecated.

@jshaughn perhaps -- that's closer to my original desire. I think we can rethink that for 1.6/1.7 when maybe we do an overhaul of the metrics (and they are controlled all via v2). however, at this point, i'd like to get the approved design implemented and into istio/istio first.

@douglas-reid yup, totally agree. And I've been coding kiali the past days in anticipation of the new grpc_request_status field, and to be back compatible.

douglas-reid · 2020-01-13T23:48:38Z

Ping. This now includes updates for the merged manifests/.

howardjohn · 2020-01-13T23:49:41Z

i am not qualified to review, lmk once a mixer owner approves and I can give it the approval

jshaughn · 2020-01-31T18:18:50Z

@douglas-reid Will this make it into 1.5?

douglas-reid · 2020-01-31T18:20:59Z

@jshaughn i hope so. i'm certainly trying to make it happen.

douglas-reid · 2020-01-31T18:21:29Z

@jshaughn the biggest issue is making sure we have equivalent functionality with v2.

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

douglas-reid · 2020-02-07T17:55:15Z

Gentle ping on a review here. This PR brings the v1 telemetry pipeline in line with changes already made to v2.

mandarjog

This is one of the last features added Mixer.

) * feat(grpc status codes): report grpc status codes via mixer * response.grpc_status is a string * force string instead of auto-detect duration * update manifest files as well * update to match istio/proxy PR Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * update golden files Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * add generated file Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * running make gen a second time produces new results Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

* DO NOT MERGE Test PR * Update proxy SHA * Debug test by printing all metrics * Check if failure repros on server side too * Add debug logging in integ tests * Add envoy debug logging * feat(grpc status codes): report grpc status codes via mixer (#20036) * feat(grpc status codes): report grpc status codes via mixer * response.grpc_status is a string * force string instead of auto-detect duration * update manifest files as well * update to match istio/proxy PR Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * update golden files Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * add generated file Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * running make gen a second time produces new results Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * More debug * Update Istio Proxy SHA (#20745) * Add TCP Stats and Metadata Filters for Istio 1.6 * Update Istio Proxy SHA * Run make gen * Use Istio generated Metadata Exchange and Stats filters in TCP Stats Test (#20818) * Add TCP Stats and Metadata Filters for Istio 1.6 * Use Istio generated Metadata Exchange and Stats filters in TCP Stats test * Run make gen * Remove debugging steps * Update to latest SHA Co-authored-by: Douglas Reid <douglas-reid@users.noreply.github.com>

) * feat(grpc status codes): report grpc status codes via mixer * response.grpc_status is a string * force string instead of auto-detect duration * update manifest files as well * update to match istio/proxy PR Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * update golden files Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * add generated file Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com> * running make gen a second time produces new results Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

israel-hdez · 2020-04-14T18:04:30Z

Perhaps, could you update the Default Metrics page? https://istio.io/docs/reference/config/policy-and-telemetry/metrics/

I also see on that page the sentence They can be found here under the section with “kind: metric” while, that kind: metric looks non existent in the current config.yaml.

douglas-reid requested review from kyessenov and mandarjog January 9, 2020 17:33

douglas-reid requested a review from a team as a code owner January 9, 2020 17:33

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 9, 2020

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

kyessenov reviewed Jan 9, 2020

View reviewed changes

douglas-reid force-pushed the grpc-for-mixer branch from dd5a1e1 to 591180e Compare January 13, 2020 23:46

douglas-reid requested review from costinm, howardjohn, linsun and rshriram as code owners January 13, 2020 23:46

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

douglas-reid mentioned this pull request Jan 14, 2020

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

Merged

4 tasks

jshaughn mentioned this pull request Jan 24, 2020

Validate Kiali against Telemetry v2 in Istio 1.5 kiali/kiali#1894

Closed

jshaughn mentioned this pull request Feb 5, 2020

Support GRPC status codes kiali/kiali#2211

Merged

douglas-reid requested a review from a team as a code owner February 6, 2020 21:10

douglas-reid force-pushed the grpc-for-mixer branch from 9d31911 to 5ab8caf Compare February 6, 2020 21:11

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 6, 2020

douglas-reid added 4 commits February 6, 2020 14:21

feat(grpc status codes): report grpc status codes via mixer

6d7d1dd

response.grpc_status is a string

3bbe4b2

force string instead of auto-detect duration

9a01ae3

update manifest files as well

71a61d2

douglas-reid added 3 commits February 6, 2020 14:21

update to match istio/proxy PR

b0b5e4a

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

update golden files

15e7097

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

add generated file

412143a

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

douglas-reid force-pushed the grpc-for-mixer branch from 9a2a26f to 412143a Compare February 6, 2020 22:30

running make gen a second time produces new results

46ed473

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

mandarjog approved these changes Feb 7, 2020

View reviewed changes

howardjohn approved these changes Feb 7, 2020

View reviewed changes

kyessenov approved these changes Feb 7, 2020

View reviewed changes

istio-testing merged commit be55403 into istio:master Feb 7, 2020

Conversation

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

Uh oh!

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!

jshaughn Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

douglas-reid commented Jan 13, 2020

Uh oh!

howardjohn commented Jan 13, 2020

Uh oh!

jshaughn commented Jan 31, 2020

Uh oh!

douglas-reid commented Jan 31, 2020

Uh oh!

douglas-reid commented Jan 31, 2020

Uh oh!

douglas-reid commented Feb 7, 2020

Uh oh!

mandarjog left a comment

Choose a reason for hiding this comment

Uh oh!

israel-hdez commented Apr 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

douglas-reid commented Jan 9, 2020 •

edited

Loading

jshaughn Jan 24, 2020 •

edited

Loading