Skip to content

stats: add histograms for request/response headers and body sizes#11559

Merged
htuch merged 47 commits intoenvoyproxy:masterfrom
ranjithkumar007:headers
Jul 21, 2020
Merged

stats: add histograms for request/response headers and body sizes#11559
htuch merged 47 commits intoenvoyproxy:masterfrom
ranjithkumar007:headers

Conversation

@ranjithkumar007
Copy link
Copy Markdown
Contributor

@ranjithkumar007 ranjithkumar007 commented Jun 11, 2020 •
edited
Loading

Signed-off-by: Ranjith Kumar [email protected]

Commit Message: stats: add histograms for request/response headers and body sizes
Additional Description: Created a new struct for optional cluster stats. Moved timeout budget stats and added request response headers and body stats in the new struct.
Risk Level: Low
Testing: Added test cases
Docs Changes: added
Release Notes: added
Fixes #10308 , Fixes #3621

@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jun 11, 2020

PTAL @nezdolik

nezdolik
nezdolik reviewed Jun 12, 2020
View reviewed changes
nezdolik
nezdolik reviewed Jun 12, 2020
View reviewed changes
nezdolik
nezdolik reviewed Jun 12, 2020
View reviewed changes
nezdolik
nezdolik reviewed Jun 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nezdolik nezdolik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from few nits. Let's proceed and add tests.

ranjithkumar007
ranjithkumar007 commented Jun 20, 2020
View reviewed changes
@ranjithkumar007 ranjithkumar007 force-pushed the headers branch 3 times, most recently from 8bad27f to 425411e Compare June 23, 2020 14:27
@ranjithkumar007 ranjithkumar007 marked this pull request as ready for review June 24, 2020 09:14
@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jun 24, 2020

All checks have passed. PTAL @nezdolik

jmarantz
jmarantz reviewed Jun 24, 2020
View reviewed changes
nezdolik
nezdolik reviewed Jun 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nezdolik nezdolik left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Updated) Nvm i see that docs have been updated

@nezdolik
Copy link
Copy Markdown
Member

nezdolik commented Jun 26, 2020

After brief discussion with @jmarantz, proposed histogram should be enabled by config flag, as it is expensive to track histogram per cluster with large number of clusters. @ranjithkumar007 here is example of histogram controlled by config flag: #9227

@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jun 26, 2020 •
edited
Loading

here is example of histogram controlled by config flag: #9227

Thanks @nezdolik . So, we need a new entry in cluster config.
I was wondering how to add such entry that would fit well for #11766 as well.
Should we have seperate entries like track_upstream_rq_headers_size, track_upstream_rq_body_size ? or a dict like track_upstream_rq_stats : {'headers_size': true, 'body_size' : false}?

@nezdolik
Copy link
Copy Markdown
Member

nezdolik commented Jun 26, 2020

Think the first approach is preferred.

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 27, 2020

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: #11559 was synchronize by ranjithkumar007.

see: more, trace.

@ranjithkumar007
Add histogram for request headers size per upstream
24bc19c 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
Add docs
3d90048 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
use read_callbacks to get clusterinfo
e4928db 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
Add tests
693297d 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
Update tests
167eb47 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
check if host is not null
dd373a5 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
Update test
006dff1 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
mock upstream host
e4fc9a5 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
increase cluster memory limit
877ff5b 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
update memory upper bound values
1bfd0f2 
Signed-off-by: Ranjith Kumar <[email protected]>
snowp
snowp suggested changes Jul 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good, just got a few style nits.

@envoyproxy/api-shepherds for API review

@ranjithkumar007
Merge branch 'master' into headers
4f10e5c 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
address comments
e984b7d 
Signed-off-by: Ranjith Kumar <[email protected]>
@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jul 19, 2020

@jmarantz There is a decline of 256 bytes in memory usage per cluster. On master it is 45003. On this PR it shows 44747.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jul 19, 2020

Awesome; one last regold away from passing release tests. Since you changed an existing large structure from absl::optional to unique_ptr, the memory reduction makes sense to me.

Nice job! Fix CI and this LGTM.

@htuch
Copy link
Copy Markdown
Member

htuch commented Jul 20, 2020

/lgtm api

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jul 20, 2020

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jul 20, 2020

Azure Pipelines successfully started running 1 pipeline(s).

jmarantz
jmarantz previously approved these changes Jul 20, 2020
View reviewed changes
@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jul 20, 2020 •
edited
Loading

hds_integration_test is timing out again on tsan. Possibly a flaky test.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jul 20, 2020

/azp run

RE hds_integration_test timing out...not clear whether this is just death by 3 seconds/test-method * lots of test methods, or something is getting stuck. Hopefully we'll catch a faster machine on this run.

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jul 20, 2020

Command 'run

RE' 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.

@nezdolik
Copy link
Copy Markdown
Member

nezdolik commented Jul 20, 2020

PR lgtm apart from the point of adding back docs for deprecated field.

@nezdolik
Copy link
Copy Markdown
Member

nezdolik commented Jul 21, 2020 •
edited
Loading

Looks like hds flaky test is the reason of failing CI, @ranjithkumar007 could you try to retrigger CI with /retest comment?

@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jul 21, 2020

Looks like hds flaky test is the reason of failing CI, @ranjithkumar007 could you try to retrigger CI with /retest comment?

Yeah, I will do it once I update the docs.

@ranjithkumar007
update docs
63e36fb 
Signed-off-by: Ranjith Kumar <[email protected]>
htuch
htuch approved these changes Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@htuch htuch merged commit a24c95e into envoyproxy:master Jul 21, 2020
@ranjithkumar007
Copy link
Copy Markdown
Contributor Author

ranjithkumar007 commented Jul 22, 2020

Thanks everyone! Good to have first contribution to envoy merged 😀 .

KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
@ranjithkumar007 @KBaichoo
stats: add histograms for request/response headers and body sizes (en…
93c977e 
…voyproxy#11559)

Created a new struct for optional cluster stats. Moved timeout budget stats and added request response headers and body stats in the new struct.

Risk Level: Low
Testing: Added test cases
Docs Changes: added
Release Notes: added

Fixes envoyproxy#10308 , Fixes envoyproxy#3621

Signed-off-by: Ranjith Kumar <[email protected]>
Signed-off-by: Kevin Baichoo <[email protected]>
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
@ranjithkumar007
stats: add histograms for request/response headers and body sizes (en…
9ff7f61 
…voyproxy#11559)

Created a new struct for optional cluster stats. Moved timeout budget stats and added request response headers and body stats in the new struct.

Risk Level: Low
Testing: Added test cases
Docs Changes: added
Release Notes: added

Fixes envoyproxy#10308 , Fixes envoyproxy#3621

Signed-off-by: Ranjith Kumar <[email protected]>
Signed-off-by: scheler <[email protected]>
@ranjithkumar007 ranjithkumar007 deleted the headers branch August 12, 2020 16:48
@rgs1 rgs1 mentioned this pull request Apr 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nezdolik nezdolik nezdolik left review comments

@ggreenway ggreenway ggreenway requested changes

@jmarantz jmarantz jmarantz approved these changes

+2 more reviewers

@snowp snowp snowp requested changes

@htuch htuch htuch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmarantz jmarantz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature request: Histogram for header sizes Feature request: statistics on request and response body size

6 participants

@ranjithkumar007 @nezdolik @jmarantz @htuch @snowp @ggreenway