feat(config): add pprof global config support#729
feat(config): add pprof global config support#729clayzermk1 wants to merge 1 commit intoalexandrevilain:mainfrom
Conversation
alexandrevilain
left a comment
alexandrevilain
left a comment
There was a problem hiding this comment.
Single comment about the code logic.
Don't you think it could be cool to expose this port in the containers and in the service too ? It would make it easier to reach the pprof endpoint.
| // Port is required if the object is defined. | ||
| // Host is optional. | ||
| // Do not set a pprof config if the port is 0. | ||
| if pprofConfig.Port > 0 { |
There was a problem hiding this comment.
Is this condition mandatory ? Maybe this could be moved to the first condition.
WDYT ?
There was a problem hiding this comment.
Happy to remove it and reorder the conditionals.
There was a problem hiding this comment.
Updated, LMK if that's what you were looking for.
Let me take a look at what's involved. Since the |
clayzermk1
force-pushed
the
add-pprof-config-to-cluster-spec
branch
from
9ea6c2a to
8a3b79f
Compare
clayzermk1
force-pushed
the
add-pprof-config-to-cluster-spec
branch
from
8a3b79f to
0040110
Compare
|
|
@alexandrevilain added auto One thing I couldn't figure out was how to prevent a nil |
I figured it out. It's because the Temporal |
alexandrevilain
left a comment
alexandrevilain
left a comment
There was a problem hiding this comment.
@clayzermk1 added some comments :)
If you're wondering on default values for host and port, you can set default values in: https://github.com/alexandrevilain/temporal-operator/blob/main/api/v1beta1/temporalcluster_defaults.go#L60
| // PProfPort defines a custom pprof port for the service. | ||
| // The port is defined by the global config. | ||
| // +optional | ||
| PProfPort *int `json:"pprofPort,omitempty"` |
There was a problem hiding this comment.
IMO this field is useless. If spec.pprof != nil the container will expose spec.pprof.port.
| } | ||
|
|
||
| if b.instance.Spec.PProf != nil && b.instance.Spec.PProf.Port > 0 { | ||
| b.service.PProfPort = &b.instance.Spec.PProf.Port |
There was a problem hiding this comment.
To follow my comment on the spec, you can remove this assignment.
|
|
||
| containerPorts = append(containerPorts, corev1.ContainerPort{ | ||
| Name: "pprof", | ||
| ContainerPort: int32(*b.service.PProfPort), |
There was a problem hiding this comment.
| ContainerPort: int32(*b.service.PProfPort), | |
| ContainerPort: int32(b.instance.Spec.PProf.Port), |
|
Thanks! I'll take a look and turn this around ASAP.
I was trying to avoid setting default values since Temporal sets them itself. That way if they ever decide to change the defaults, no code change would be required. |
|
Hi @clayzermk1 ! Do you need any help ? |
|
Hi @alexandrevilain, Sorry for the slow turn, I've been reassigned to a different project. I'm not sure when I'll get back around to this. |
|
I received permission to work on this. I should be able to pick it up in a week or so 👍 |
|
Still on the radar. |
2 participants
Implements #705
I've tested both port and host as working on my cluster against a custom docker image with these changes.