filter: add grpc upstream stats by Sh4d1 · Pull Request #10748 · envoyproxy/envoy

Sh4d1 · 2020-04-13T11:34:50Z

Description: Add upstream_rq_time histogram to grpc stats
Risk Level: Medium
Testing: unit test + manual test
Docs Changes: add note about enable_upstream_stats
Release Notes: added
Parially Fixes #9169

repokitteh-read-only · 2020-04-13T11:34:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10748 was opened by Sh4d1.

see: more, trace.

Sh4d1 · 2020-04-13T11:49:25Z

cc @kyessenov

api/envoy/config/filter/http/grpc_stats/v2alpha/config.proto

api/envoy/extensions/filters/http/grpc_stats/v3/config.proto

ggreenway · 2020-04-13T18:26:09Z

source/common/grpc/context_impl.cc

change type to std::chrono::duration

Would miliseconds work? duration is a template, pretty sure I'll need to modify the class as well 🤔

Sh4d1 · 2020-04-14T08:41:32Z

Not sure why the docs are failing 🤔

jmarantz

Looks good generally; one minor nit.

@envoyproxy/senior-maintainers

source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

Sh4d1 · 2020-04-16T06:25:49Z

All good for me I think

ggreenway · 2020-04-16T16:12:14Z

@Sh4d1 Do not force-push to PR branches please. It makes reviewing changes much more difficult.

ggreenway · 2020-04-16T16:15:03Z

/retest

repokitteh-read-only · 2020-04-16T16:15:06Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #10748 (comment) was created by @ggreenway.

see: more, trace.

ggreenway · 2020-04-16T16:15:30Z

/azp retest

azure-pipelines · 2020-04-16T16:15:35Z

Command 'retest' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

Sh4d1 · 2020-04-16T16:16:16Z

@ggreenway it was only a rebase 😄

ggreenway · 2020-04-16T16:17:24Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-16T16:17:34Z

Azure Pipelines successfully started running 1 pipeline(s).

ggreenway · 2020-04-16T16:20:48Z

@Sh4d1 Don't rebase. Please merge instead. We squash the commits when the PR is merged so the extra history doesn't hurt anything. But when people force-push, there's not a way to automatically see that it is only a rebase. As described in https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md:

Once you submit a PR, please do not rebase it. It's much easier to review if subsequent commits are new commits and/or merges. We squash rebase the final merged commit so the number of commits you have in the PR don't matter.

ggreenway · 2020-04-16T16:22:54Z

/azp run envoy-presubmit (macOS)

azure-pipelines · 2020-04-16T16:23:17Z

No pipelines are associated with this pull request.

Sh4d1 · 2020-04-16T16:30:10Z

@ggreenway okay! Sorry I missed that part, my bad 🙄

ggreenway · 2020-04-16T19:38:14Z

@envoyproxy/api-shepherds PTAL

htuch

/lgtm api

mattklein123 · 2020-04-18T21:36:48Z

source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc

+      if (config_->enable_upstream_stats_ &&
+          decoder_callbacks_->streamInfo().lastUpstreamTxByteSent().has_value() &&
+          decoder_callbacks_->streamInfo().lastUpstreamRxByteReceived().has_value()) {
+        std::chrono::milliseconds chrono_duration =
+            std::chrono::duration_cast<std::chrono::milliseconds>(
+                decoder_callbacks_->streamInfo().lastUpstreamRxByteReceived().value() -
+                decoder_callbacks_->streamInfo().lastUpstreamTxByteSent().value());
+        config_->context_.chargeUpstreamStat(*cluster_, request_names_, chrono_duration);
+      }


@ggreenway @Sh4d1 post review drive by: this would not include "trailers only" responses (headers only responses with an error). I think this is probably not what we want and we should include error responses also? Can we fix/clarify this in a follow up? Thank you.

@mattklein123 good catch! I think adding the same logic in encodeHeaders if end_stream is true should be enough right? I'll send the fix tomorrow I think 😄

Or move the whole logic there 🤔

* stats: add upstream_rq_time by grpc methods Signed-off-by: Patrik Cyvoct <patrik@ptrk.io> Signed-off-by: pengg <pengg@google.com>

Sh4d1 requested a review from lizan as a code owner April 13, 2020 11:34

repokitteh-read-only bot added the api label Apr 13, 2020

Sh4d1 force-pushed the grpc_upstream_stats branch 2 times, most recently from 6b5a4cc to 53b3640 Compare April 13, 2020 12:51

ggreenway requested changes Apr 13, 2020

View reviewed changes

ggreenway assigned ggreenway and jmarantz Apr 13, 2020

ggreenway added the waiting label Apr 13, 2020

Sh4d1 force-pushed the grpc_upstream_stats branch from 53b3640 to dfa06e4 Compare April 14, 2020 07:35

repokitteh-read-only bot removed the waiting label Apr 14, 2020

jmarantz previously approved these changes Apr 14, 2020

View reviewed changes

source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc Outdated Show resolved Hide resolved

Sh4d1 dismissed jmarantz’s stale review via 6dcb4e8 April 16, 2020 06:22

Sh4d1 added 10 commits April 16, 2020 08:24

stats: add upstream_rq_time by grpc methods

7e36097

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

generated apis

084495f

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

add release note

e45b7dc

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

fix format

b2e7758

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

fix review

3aeef8a

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

generated apis

b76747a

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

fix doc

7c3bb66

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

fix doc

a3ac442

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

fix format

c11814b

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

use const as suggested

f7c833d

Signed-off-by: Patrik Cyvoct <patrik@ptrk.io>

Sh4d1 force-pushed the grpc_upstream_stats branch from 6dcb4e8 to f7c833d Compare April 16, 2020 06:25

ggreenway approved these changes Apr 16, 2020

View reviewed changes

htuch approved these changes Apr 17, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Apr 17, 2020

ggreenway merged commit 4a70533 into envoyproxy:master Apr 17, 2020

mattklein123 reviewed Apr 18, 2020

View reviewed changes

Sh4d1 mentioned this pull request Apr 19, 2020

fix: upstream grpc stats on trailers only #10842

Merged

penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020

filter: add grpc upstream stats (envoyproxy#10748)

3170823

* stats: add upstream_rq_time by grpc methods Signed-off-by: Patrik Cyvoct <patrik@ptrk.io> Signed-off-by: pengg <pengg@google.com>

Conversation

Sh4d1 commented Apr 13, 2020

Uh oh!

repokitteh-read-only bot commented Apr 13, 2020

Uh oh!

Sh4d1 commented Apr 13, 2020

Uh oh!

Uh oh!

Uh oh!

ggreenway Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

Sh4d1 Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

Sh4d1 commented Apr 14, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sh4d1 commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

repokitteh-read-only bot commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

azure-pipelines bot commented Apr 16, 2020

Uh oh!

Sh4d1 commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

azure-pipelines bot commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

azure-pipelines bot commented Apr 16, 2020

Uh oh!

Sh4d1 commented Apr 16, 2020

Uh oh!

ggreenway commented Apr 16, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Apr 18, 2020

Choose a reason for hiding this comment

Uh oh!

Sh4d1 Apr 18, 2020

Choose a reason for hiding this comment

Uh oh!

Sh4d1 Apr 18, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants