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.

Sign up for GitHub

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

Jump to bottom

Enable proxy prometheus by default #2015

Merged
merged 11 commits into from
Oct 22, 2023
Merged

Enable proxy prometheus by default #2015

merged 11 commits into from
Oct 22, 2023

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Oct 19, 2023

fixes: #1998

@zirain
change API
e15749a 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
implement
7efd616 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
fix test
c9d3515 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain requested a review from a team as a code owner October 19, 2023 07:32
@zirain zirain added the release-note Indicates a required release note label Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023 •
edited
Loading

Codecov Report

Merging #2015 (ba14e48) into main (930592c) will increase coverage by 0.24%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2015      +/-   ##
==========================================
+ Coverage   65.30%   65.55%   +0.24%     
==========================================
  Files          93       93              
  Lines       13727    13737      +10     
==========================================
+ Hits         8965     9005      +40     
+ Misses       4206     4183      -23     
+ Partials      556      549       -7
Files Coverage Δ
api/v1alpha1/validation/envoyproxy_validate.go 73.85% <100.00%> (+0.34%) ⬆️
internal/gatewayapi/listener.go 97.79% <100.00%> (+0.06%) ⬆️
...ternal/infrastructure/kubernetes/proxy/resource.go 91.79% <100.00%> (+0.39%) ⬆️
...frastructure/kubernetes/proxy/resource_provider.go 86.14% <100.00%> (-0.41%) ⬇️
internal/xds/bootstrap/bootstrap.go 85.50% <100.00%> (-0.21%) ⬇️

... and 2 files with indirect coverage changes

@zirain
doc & examples
284974f 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
use pointer and rename field
969d6b4 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
nit
f95109c 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
Merge branch 'main' into enable-proxy-prometheus
7a9dc17
@zirain zirain requested a review from arkodg October 19, 2023 20:27
arkodg
arkodg reviewed Oct 19, 2023
View reviewed changes
internal/infrastructure/kubernetes/proxy/resource.go
}

if proxyMetrics != nil && proxyMetrics.Prometheus != nil {
if enablePrometheus(infra) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this port always exist, since its the readiness port as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then we need to rename container name.
Keep it the same as before, will do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and think about #1787

@arkodg
Copy link
Contributor

arkodg commented Oct 19, 2023

thanks @zirain ! added some minor comments,
can you also update https://gateway.envoyproxy.io/latest/user/proxy-observability/ & https://gateway.envoyproxy.io/latest/user/grafana-integration/ to reflect this change (guessing we just need to delete existing lines of text)

@zirain
update doc
23f7e22 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
make optional
e46c34f 
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain zirain mentioned this pull request Oct 20, 2023
Closed
arkodg
arkodg previously approved these changes Oct 20, 2023
View reviewed changes
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 requested review from a team, zhaohuabing, qicz and LanceEa and removed request for a team October 20, 2023 19:07
Alice-Lilith
Alice-Lilith previously approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@Alice-Lilith Alice-Lilith left a comment

Choose a reason for hiding this comment

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

lgtm, I like prometheus enabled by default

@zirain
Copy link
Contributor Author

zirain commented Oct 21, 2023

@arkodg @AliceProxy fix the conflict.

@arkodg
Copy link
Contributor

arkodg commented Oct 21, 2023

@zirain can you run make generate again

@zirain
fix test
ba14e48 
Signed-off-by: zirain <zirain2009@gmail.com>
@arkodg arkodg merged commit c12c14c into envoyproxy:main Oct 22, 2023
18 checks passed
@zirain zirain deleted the enable-proxy-prometheus branch October 22, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkodg arkodg arkodg approved these changes

@zhaohuabing zhaohuabing Awaiting requested review from zhaohuabing zhaohuabing was automatically assigned from envoyproxy/gateway-reviewers

@qicz qicz Awaiting requested review from qicz qicz was automatically assigned from envoyproxy/gateway-reviewers

@LanceEa LanceEa Awaiting requested review from LanceEa LanceEa was automatically assigned from envoyproxy/gateway-reviewers

@Alice-Lilith Alice-Lilith Awaiting requested review from Alice-Lilith

Assignees
No one assigned
Labels
release-note Indicates a required release note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decision: Enable Prometheus Endpoint by default for EnvoyProxy
3 participants
@zirain @arkodg @Alice-Lilith