Add pprof for galley #10047

clyang82 · 2018-11-19T06:29:37Z

Enable pprof for galley only. the pprof will be started in 9094 by default.

Signed-off-by: Chun Lin Yang clyang@cn.ibm.com

istio-testing · 2018-11-19T06:29:51Z

Hi @clyang82. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ayj · 2018-11-19T19:11:58Z

Thanks @clyang82 for added this. Could we expose the pprof endpoint on a different port than metrics? Metrics are exposed outside the pod (via container port / service), whereas pprof can be accessed via kubectl port-foward when we need to.

ayj · 2018-11-19T19:12:03Z

/ok-to-test

clyang82 · 2018-11-20T01:11:28Z

@ayj Thanks for your comments. Right now, mixer, pilot, citadel and galley expose the pprof endpoint on 9093. I think we should keep consistence, consider that they are for self-monitoring. What do you think?

hzxuzhonghu · 2018-11-20T01:19:37Z

/ok-to-test

ayj · 2018-11-20T01:27:20Z

I thought Pilot exposed pprof on port 8080 along with the other legacy discovery services (see

istio/pilot/pkg/proxy/envoy/discovery.go

Line 115 in 5a6cca3

    
           func NewDiscoveryService(environment *model.Environment, o DiscoveryServiceOptions) (*DiscoveryService, error) {

).

clyang82 · 2018-11-20T07:01:34Z

@ayj Yes. you are right. But 9093 is also working for pilot. they are 2 http ports to expose the same content /metrics, /pprof, /debug, /ready, /version. We need to remove one of them. I suggest to keep 9093 and remove 8080. What is your idea?

ymesika · 2018-11-20T08:16:19Z

galley/cmd/galley/cmd/root.go

@@ -123,6 +124,8 @@ func GetRootCmd(args []string, printf, fatalf shared.FormatFn) *cobra.Command {
 		"Interval of updating file for the Galley readiness probe.")
 	rootCmd.PersistentFlags().UintVar(&monitoringPort, "monitoringPort", 9093,
 		"Port to use for exposing self-monitoring information")
+	rootCmd.PersistentFlags().BoolVar(&enableProfiling, "enableProfiling", true,


Maybe false for default?

pilot, mixer are true by default, while citadel is false. Do we need to make them to false for default?

Not sure. Let's wait for community feedback on this because it doesn't seem right for me that profiling is on by default.

Thanks for your response. Let us wait for the feedback from community.

Is there a reason for having them on by default?

I don't know. It is on in the initial version. @kyessenov any thoughts?

Let's keep it off by default. We originally made this default=true in Pilot ~2 years ago to help with early debugging before Istio was used in production.

make it to default=false for mixer, pilot, galley and citadel. right?

ayj · 2018-11-21T17:55:12Z

I suggest to keep 9093 and remove 8080. What is your idea?

It seems like exposing telemetry and internal diagnostic/debug information on the same port might be risky. If they are separate ports, we can more easily limit pprof exposure by not adding its port to the k8s pod/svc.

clyang82 · 2018-11-22T09:11:17Z

I suggest to keep 9093 and remove 8080. What is your idea?

It seems like exposing telemetry and internal diagnostic/debug information on the same port might be risky. If they are separate ports, we can more easily limit pprof exposure by not adding its port to the k8s pod/svc.

@ayj sounds good. how about?

9093 for telemetry - it can reduce the changes. many documents mention that.
9094 for debug information - not adding to pod/svc
8080 pilot only for service discover - List all known services in /v1/registration

ayj · 2018-11-27T01:30:17Z

Sounds good in general. We need to be careful about renumbering any Pilot ports that are used by istioctl.

cc @rshriram (pilot), @douglas-reid (mixer), @liamawhite (istioctl)

codecov · 2018-11-27T12:50:50Z

Codecov Report

Merging #10047 into release-1.1 will increase coverage by 2%.
The diff coverage is 13%.

@@              Coverage Diff               @@
##           release-1.1   #10047     +/-   ##
==============================================
+ Coverage           68%      70%     +2%     
==============================================
  Files              571      442    -129     
  Lines            48706    41362   -7344     
==============================================
- Hits             32982    28830   -4152     
+ Misses           13946    11122   -2824     
+ Partials          1778     1410    -368

Impacted Files	Coverage Δ
galley/pkg/server/monitoring.go	`0% <0%> (ø)`	⬆️
mixer/pkg/server/args.go	`100% <100%> (ø)`	⬆️
mixer/adapter/inventory.gen.go	`0% <0%> (-100%)`	⬇️
pilot/pkg/networking/plugin/mixer/mixer.go	`1% <0%> (-77%)`	⬇️
mixer/adapter/rbac/rbac.go	`0% <0%> (-70%)`	⬇️
galley/pkg/meshconfig/cache.go	`0% <0%> (-63%)`	⬇️
pkg/mcp/server/monitoring.go	`3% <0%> (-57%)`	⬇️
mixer/pkg/runtime/config/queries.go	`64% <0%> (-36%)`	⬇️
mixer/pkg/runtime/config/snapshot.go	`58% <0%> (-36%)`	⬇️
pilot/pkg/serviceregistry/memory/discovery.go	`57% <0%> (-29%)`	⬇️
... and 275 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c24863b...0acc8bd. Read the comment docs.

clyang82 · 2018-11-27T12:55:27Z

Thanks @ayj . I would like to use this PR to add pprof for galley and disable pprof for pilot/mixer/citadel/istioctl by default. Will have a separated PR to handle:

9093 for telemetry - it can reduce the changes. many documents mention that.
9094 for debug information - not adding to pod/svc
8080 pilot only for service discover - List all known services in /v1/registration

Would you please help review this PR firstly? Thanks.

clyang82 · 2018-11-27T14:03:08Z

/test e2e-dashboard

liamawhite · 2018-11-27T14:14:48Z

@clyang82 if you're changing the debug port in Pilot to 9094 you will need to make changes so that istioctl works. This is the line that needs changing -> https://github.com/istio/istio/blob/master/pilot/cmd/pilot-discovery/request.go#L33

ayj · 2018-11-27T18:23:04Z

galley/cmd/galley/cmd/root.go

@@ -123,6 +124,8 @@ func GetRootCmd(args []string, printf, fatalf shared.FormatFn) *cobra.Command {
 		"Interval of updating file for the Galley readiness probe.")
 	rootCmd.PersistentFlags().UintVar(&monitoringPort, "monitoringPort", 9093,
 		"Port to use for exposing self-monitoring information")
+	rootCmd.PersistentFlags().BoolVar(&enableProfiling, "enableProfiling", false,
+		"Enable pprof for Galley")


Can you add the pprofPort flag now and plumb that through StartSelfMonitoring? That way we can be done with the galley specific changes and address Pilot and Mixer individually.

@ayj sorry for late. I just add pprof in 9094. please review again. Let us use this ticket to handle galley specific changes. Thanks.

douglas-reid · 2018-11-27T19:37:37Z

/hold

@mandarjog wants mixer to have pprof on by default.

clyang82 · 2018-11-28T14:49:49Z

@mandarjog could you share your point why you want to have pprof on by default for mixer? Thanks.

costinm · 2018-11-29T00:58:52Z

/hold - please don't change pilot pprof or ports without a strong reason.

costinm · 2018-11-29T01:00:35Z

Also: please use the PR description or comments to explain why a change is made - it makes no sense to me to make gratuitous backward incompatible changes like renaming ports.
We can be careful for new ports to make sure they use the 150xx range.

clyang82 · 2018-12-03T14:16:31Z

/hold - please don't change pilot pprof or ports without a strong reason.

@costinm right now, for pilot, they are 2 http ports (8080 & 9093) to expose the same content /metrics, /pprof, /debug, /ready, /version. that is obvious duplication we should keep one for it.
if there is no strong reason to enable the debug, it should be disabled by default to reduce the performance impact and unnecessary noise. Thanks.

liamawhite · 2018-12-03T15:45:58Z

@clyang82 /debug is required for a couple of istioctl commands. IMO we should leave debug on by default but allow users to turn it off. The people who really need the perf will be a much fewer number of people and they are more likely to understand what happens when you turn off the debug.

stale · 2018-12-17T17:10:42Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

istio-testing · 2018-12-26T03:50:51Z

@clyang82: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-mixer-no_auth-mcp.sh	a61a98e7dabbca82a244937c55f028cada198f1f	link	`/test e2e-mixer-no_auth-mcp`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

clyang82 · 2018-12-27T10:42:41Z

/retest

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

clyang82 · 2019-01-03T01:11:13Z

/assign @ayj
I would like to use this ticket to handle galley specific changes. Thanks.

ayj

/lgtm

istio-testing · 2019-01-03T18:08:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayj, clyang82

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~galley/OWNERS~~ [ayj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes label Nov 19, 2018

istio-testing requested review from ayj and ozevren November 19, 2018 06:29

istio-testing added the needs-ok-to-test label Nov 19, 2018

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 19, 2018

ymesika reviewed Nov 20, 2018

View reviewed changes

clyang82 mentioned this pull request Nov 20, 2018

REQUEST: New membership for clyang82 istio/community#102

Closed

7 tasks

clyang82 changed the title ~~Enable pprof for galley~~ Add pprof for galley and disable pprof for pilot/mixer/citadel/istioctl Nov 27, 2018

ayj reviewed Nov 27, 2018

View reviewed changes

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Nov 27, 2018

stale bot added the stale label Dec 17, 2018

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 22, 2018

clyang82 force-pushed the enable_pprof_galley branch from a61a98e to 1548826 Compare December 26, 2018 01:59

stale bot removed the stale label Dec 26, 2018

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 26, 2018

clyang82 changed the title ~~Add pprof for galley and disable pprof for pilot/mixer/citadel/istioctl~~ Add pprof for galley Dec 26, 2018

clyang82 added 2 commits December 28, 2018 13:51

Enable pprof to galley

a35c1c7

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

start pprofile in 9094

0acc8bd

Signed-off-by: Chun Lin Yang <clyang@cn.ibm.com>

clyang82 force-pushed the enable_pprof_galley branch from 0d78643 to 0acc8bd Compare December 28, 2018 05:52

istio-testing assigned ayj Jan 3, 2019

ayj approved these changes Jan 3, 2019

View reviewed changes

istio-testing added the lgtm label Jan 3, 2019

istio-testing added the approved label Jan 3, 2019

clyang82 removed the do-not-merge/hold Block automatic merging of a PR. label Jan 6, 2019

istio-testing merged commit ee30c79 into istio:release-1.1 Jan 6, 2019

clyang82 deleted the enable_pprof_galley branch January 11, 2019 07:11

Add pprof for galley #10047

Add pprof for galley #10047

Conversation

clyang82 commented Nov 19, 2018 • edited

istio-testing commented Nov 19, 2018

ayj commented Nov 19, 2018

ayj commented Nov 19, 2018

clyang82 commented Nov 20, 2018

hzxuzhonghu commented Nov 20, 2018

ayj commented Nov 20, 2018

clyang82 commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayj commented Nov 21, 2018

clyang82 commented Nov 22, 2018 • edited

ayj commented Nov 27, 2018

codecov bot commented Nov 27, 2018 • edited

Codecov Report

clyang82 commented Nov 27, 2018

clyang82 commented Nov 27, 2018

liamawhite commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglas-reid commented Nov 27, 2018

clyang82 commented Nov 28, 2018

costinm commented Nov 29, 2018

costinm commented Nov 29, 2018

clyang82 commented Dec 3, 2018

liamawhite commented Dec 3, 2018

stale bot commented Dec 17, 2018

istio-testing commented Dec 26, 2018 • edited

clyang82 commented Dec 27, 2018

clyang82 commented Jan 3, 2019

ayj left a comment

Choose a reason for hiding this comment

istio-testing commented Jan 3, 2019

clyang82 commented Nov 19, 2018 •

edited

clyang82 commented Nov 22, 2018 •

edited

codecov bot commented Nov 27, 2018 •

edited

istio-testing commented Dec 26, 2018 •

edited