feat: support disabling monitoring for RootSync and RepoSync - #2233
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to disable monitoring in Config Sync by adding a monitoring field to the RepoSync and RootSync CRD specs, and conditionally omitting the otel-agent container and its volumes when disabled. However, a critical issue was identified in the metrics registration logic across pkg/metrics/register.go, pkg/kmetrics/register.go, and pkg/resourcegroup/controllers/metrics/register.go. Returning a nil exporter when DISABLE_MONITORING is active will cause runtime panics (nil pointer dereferences) in callers that invoke defer exporter.Shutdown(ctx) or use the exporter without a nil check. To resolve this, all callers must be updated to handle a nil exporter safely, or a safe no-op exporter should be returned.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
d603b59 to
e458faf
Compare
8651415 to
1df8436
Compare
|
/retest |
|
/test kpt-config-sync-presubmit-e2e-multi-repo-test-group3 |
|
/test kpt-config-sync-presubmit-e2e-multi-repo-test-group1 |
|
|
||
| // RegisterOTelExporter creates the OTLP metrics exporter. | ||
| func RegisterOTelExporter(ctx context.Context, containerName string) (*otlpmetricgrpc.Exporter, error) { | ||
| if os.Getenv("DISABLE_MONITORING") == "true" { |
There was a problem hiding this comment.
Meant to be "!="? I wonder why the test was not catching this
There was a problem hiding this comment.
In cmd/reconciler-manager/main.go, we use if os.Getenv("DISABLE_MONITORING") != "true" because we are conditionally executing and wrapping a block of code (and persisting general initialization flow afterwards).
Here in pkg/kmetrics/register.go (and pkg/metrics/register.go), we use == "true" as an early return to guard clause. If monitoring is explicitly disabled, we initialize the safe Noop variables and return early. The rest of the function operates under the assumption that monitoring is active.
Returning nil here is by design, no exporter exists when monitoring is disabled. All four call sites in this PR already nil-check OTel Collector Exporter before Shutdown, so this can't panic.
afdb4ee to
7cc953d
Compare
This commit introduces the ability to opt-out of monitoring sidecars (specifically otel-agent) for RootSync and RepoSync objects via the spec.monitoring.enabled field in the declarative API. When monitoring is disabled, the config-sync controllers will omit the otel-agent container from the reconciler pods and set the environment variable DISABLE_MONITORING=true in the remaining vital containers to silence telemetry exporter logic. All Unit and E2E tests for this logic have been implemented accordingly. TAG=agy
7cc953d to
145627f
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tiffanny29631 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4e6e21b
into
GoogleContainerTools:main
2 participants
This commit introduces the ability to opt-out of monitoring sidecars (specifically otel-agent) for RootSync and RepoSync objects via the
spec.monitoring.enabled fieldin the declarative API.When monitoring is disabled, the config-sync controllers will omit the otel-agent container from the reconciler pods and set the environment variable
DISABLE_MONITORING=truein the remaining vital containers to silence telemetry exporter logic. All Unit and E2E tests for this logic have been implemented accordingly.