Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: add stats_matcher for envoy's stats #1761

Conversation

tmsnan
Copy link
Contributor

@tmsnan tmsnan commented Aug 3, 2023

api: add stats_matcher for envoy's stats
Relates to #1669

@tmsnan tmsnan requested a review from a team as a code owner August 3, 2023 07:57
@tmsnan tmsnan force-pushed the dev-add-envoy-stats-matcher-and-histogrambucketsettings branch from 71ed779 to 21d1e0e Compare August 3, 2023 07:57
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #1761 (bdfa889) into main (3fbaa78) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1761      +/-   ##
==========================================
+ Coverage   65.22%   65.27%   +0.04%     
==========================================
  Files          86       86              
  Lines       12318    12318              
==========================================
+ Hits         8034     8040       +6     
+ Misses       3772     3766       -6     
  Partials      512      512              

see 2 files with indirect coverage changes

@tmsnan tmsnan marked this pull request as draft August 3, 2023 08:16
@zirain
Copy link
Contributor

zirain commented Aug 3, 2023

please add api first

@tmsnan tmsnan force-pushed the dev-add-envoy-stats-matcher-and-histogrambucketsettings branch from 21d1e0e to 5c148d1 Compare August 4, 2023 07:11
@tmsnan tmsnan marked this pull request as ready for review August 4, 2023 07:13
@tmsnan tmsnan force-pushed the dev-add-envoy-stats-matcher-and-histogrambucketsettings branch from 5c148d1 to 2f6f0b7 Compare August 4, 2023 07:15
@tmsnan tmsnan changed the title followup: add stats_matcher and histogramBucketSettings for envoy's stats api: add stats_matcher and histogramBucketSettings for envoy's stats Aug 4, 2023
@tmsnan
Copy link
Contributor Author

tmsnan commented Aug 4, 2023

please add api first

done

@tmsnan tmsnan force-pushed the dev-add-envoy-stats-matcher-and-histogrambucketsettings branch from 3e4dfa3 to 6cf8fa7 Compare August 7, 2023 02:26
@tmsnan
Copy link
Contributor Author

tmsnan commented Aug 7, 2023

/retest

zhaonan added 4 commits August 10, 2023 19:14
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
@tmsnan tmsnan force-pushed the dev-add-envoy-stats-matcher-and-histogrambucketsettings branch from e5da312 to 45bc763 Compare August 10, 2023 12:02
zhaonan added 3 commits August 11, 2023 11:22
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
@tmsnan tmsnan changed the title api: add stats_matcher and histogramBucketSettings for envoy's stats api: add stats_matcher for envoy's stats Aug 11, 2023
@arkodg
Copy link
Contributor

arkodg commented Aug 22, 2023

thanks for incorporating the suggestions @tmsnan, had some more minor suggestions around comments and doc string, overall LGTM

zhaonan added 2 commits August 22, 2023 20:20
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
@@ -10,6 +10,14 @@ type ProxyMetrics struct {
Prometheus *PrometheusProvider `json:"prometheus,omitempty"`
// Sinks defines the metric sinks where metrics are sent to.
Sinks []MetricSink `json:"sinks,omitempty"`
// Matches defines configuration for selecting specific metrics instead of generating all metrics stats
// that are enabled by default. This helps reduce CPU and memory overhead in Envoy.
// When Matches is nil(not set), gateway by default create and expose only a subset of
Copy link
Contributor

Choose a reason for hiding this comment

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

lines 15-19 can be removed, imo
also because its hard to define in docs what exactly the default stats are, for e.g. we currently also output http stats

zirain and others added 2 commits August 23, 2023 09:25
Signed-off-by: zhaonan <zhaonan06@corp.netease.com>
@tmsnan
Copy link
Contributor Author

tmsnan commented Aug 23, 2023

/retest

Copy link
Contributor

@arkodg arkodg 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 !

@arkodg arkodg merged commit 6ff0d37 into envoyproxy:main Aug 23, 2023
20 checks passed
@tmsnan tmsnan deleted the dev-add-envoy-stats-matcher-and-histogrambucketsettings branch December 6, 2023 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants