Pprof modularization #24114
Pprof modularization #24114
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
pippolo84
added
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
from
4702a80
to
eaf0612
Compare
|
Integration tests are failing due to a mismatch between the Go version used by the workflow and the one set in go.mod following this commit. |
pkg/defaults/defaults.go
Outdated
| // PprofAddressAPIServer is the default value for pprof in the clustermesh-apiserver | ||
| PprofAddressAPIServer = "localhost" | ||
|
|
||
| // PprofPortAgent is the default value for pprof in the agent | ||
| PprofPortAgent = 6060 | ||
|
|
||
| // PprofPortAgent is the default value for pprof in the operator | ||
| // PprofPortOperator is the default value for pprof in the operator |
There was a problem hiding this comment.
Remove and just do the defaults inline where we define the default config? Makes more sense to me that way, but happy to hear if someone has an argument for keeping pkg/defaults.
There was a problem hiding this comment.
Unfortunately those defaults are used also by bugtool. Considered that, it probably makes sense to leave them in pkg/defaults.
There was a problem hiding this comment.
As a trade-off, I ended up moving the default values from pkg/defaults to the binary specific option package (pkg/option for the agent, operator/option for the operator and so on).
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
2 times, most recently
from
89c6ca1
to
8b39491
Compare
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
from
8b39491
to
605778c
Compare
|
Uploaded a minor change (and related test) to avoid starting a net.Listener in case of pprof disabled by config. |
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
from
605778c
to
003c692
Compare
|
Updated cmdref documentation. |
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
from
003c692
to
c1d9dbd
Compare
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
2 times, most recently
from
f88383a
to
940927e
Compare
| @@ -17,6 +17,8 @@ import ( | |||
| var log = logging.DefaultLogger.WithField(logfields.LogSubsys, "pprof") | |||
There was a problem hiding this comment.
Remove and use injected logrus.FieldLogger
There was a problem hiding this comment.
This is used by pprof.Enable, that we still need for hubble-relay. Being outside of the cell lifecycle, we won't have any injected logrus.FieldLogger here.
But this should be gone once hubble-relay will be refactored to use the pprof cell.
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
from
940927e
to
fa2edbd
Compare
|
/test |
Modularize pprof related code into a cell. The cell is used by the agent, the operator and the clustermesh-apiserver. The already present pprof.Enable function is marked as deprecated. Differently from the agent and the clustermesh-apiserver, the operator prefixes the pprof related flags with `operator-`. To reuse the same cell, we use an operator specific config type, in order to map the equivalent fields to the operator-specific flag names. Also, when importing the cell, an additional call to cell.Config is requested to register the default config type (for agent and clustermesh-apiserver) or the operator-specific one. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Add two unit-style integration tests to verify the correct registration of the pprof HTTP handlers. Specifically, the first one check that no listeners is started when pprof is disabled and the other one exercises the `debug/pprof/heap` endpoint when pprof is enabled. Both tests verify that the cell is not leaking any goroutines after the hive stops. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
This commit regenerates the cmdref documentation files following the modularization of pprof. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Fix a typo (`Private` -> `Provide`) in ProvidePrivate function inline documentation. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Move pprof related options default values (address and port) from pkg/defaults to binary specific packages, like pkg/option for the agent, operator/option for the operator and clustermesh-apiserver/option for the clustermesh server. This is done to avoid polluting the global defaults package with defaults that are not really global but binary specific. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
pippolo84
force-pushed
the
pr/pippolo84/pprof-modularization
branch
from
fa2edbd
to
342998a
Compare
|
/test |
|
test-runtime hit #23495 |
|
CI is happy (minus an unrelated flake as reported in previous comment), reviews are in, marking ready to merge. |
pippolo84
added
the
ready-to-merge
Modularize the pprof related code into a cell. The cell is used by the agent, the operator and the clustermesh-apiserver to register the pprof HTTP handlers and export runtime profiling data.
The already present
pprof.Enablefunction is marked as deprecated, but not removed, since hubble-relay still depends on it and that is not yet modularized to use hive and cells.Differently from the agent and the clustermesh-apiserver, the operator prefixes the pprof related flags with
operator-. To reuse the same cell, we use an operator specific config type, in order to map the equivalent fields to the operator-specific flag names. Also, when importing the cell, an additional call to cell.Config is requested to register the default config type (for agent and clustermesh-apiserver) or the operator-specific one.The PR also adds an integration-style unit test to verify the correct registration of the pprof HTTP handlers when the cell is used.
Fixes: #24048