update monitoring config struct#1227
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the CCV monitoring configuration to remove the old Enabled/Type switch and replace it with typed sub-configs (Monitoring.Beholder, Monitoring.Pyroscope) plus a top-level Monitoring.LogLevel. It also centralizes Pyroscope lifecycle management in the bootstrapper and updates service wiring + devenv TOML templates accordingly.
Changes:
- Refactor
common/monitoring.Configand validation to supportBeholder.Enabled,Pyroscope.Enabled/URL, andLogLevel. - Add
SetupPyroscopehelper and wire Pyroscope setup/teardown intoBootstrapperstartup/shutdown. - Update call sites, tests, and devenv TOML configs/templates to match the new monitoring config shape.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| indexer/pkg/config/config.go | Switch indexer config validation to delegate to Monitoring.Validate(). |
| indexer/config.example.toml | Update example TOML to new Monitoring.{LogLevel,Pyroscope,Beholder} layout. |
| indexer/cmd/oapigen/go.sum | Add Pyroscope module checksums for codegen module. |
| indexer/cmd/main.go | Gate beholder monitoring initialization on Monitoring.Beholder.Enabled. |
| executor/config_test.go | Adjust config validation test inputs to new monitoring semantics. |
| deployment/changesets/generate_token_verifier_config.go | Update token-verifier config generation to match new monitoring model (and related inputs). |
| common/monitoring/setup.go | Refactor SetupBeholder signature and add SetupPyroscope helper. |
| common/monitoring/logging/logging.go | Change logger init to accept string log level and key off Beholder.Enabled. |
| common/monitoring/config.go | Implement new monitoring config schema and validation (log levels + pyroscope URL). |
| cmd/verifier/common.go | Update monitoring enablement check to config.Beholder.Enabled. |
| cmd/executor/service_test.go | Consolidate monitoring-disabled test case with new config structure. |
| cmd/executor/common.go | Update executor monitoring enablement check to config.Beholder.Enabled. |
| build/devenv/services/tokenVerifier.template.toml | Remove standalone pyroscope_url template entry. |
| build/devenv/services/tokenVerifier.go | Remove embedded template usage and template-derived config helper. |
| build/devenv/services/aggregator.template.toml | Update devenv aggregator template to new monitoring sections and remove standalone pyroscope_url. |
| build/devenv/fakes/go.sum | Add Pyroscope module checksums for devenv fakes module. |
| build/devenv/fakes/go.mod | Add Pyroscope indirect dependencies for devenv fakes module. |
| build/devenv/environment_monolith.go | Remove token-verifier template-config extraction for Pyroscope/Monitoring. |
| build/devenv/env.toml | Update monolith devenv config keys/sections to new monitoring structure. |
| build/devenv/env-phased.toml | Update phased devenv config keys/sections to new monitoring structure. |
| build/devenv/env-cl.toml | Update CL devenv config keys/sections to new monitoring structure. |
| build/devenv/env-cl-16.toml | Update CL-16 devenv config keys/sections to new monitoring structure. |
| build/devenv/config.go | Adjust strict TOML decode error reporting path. |
| build/devenv/components/tokenverifier/component.go | Remove template-config extraction in tokenverifier component wiring. |
| bootstrap/config_test.go | Update bootstrap monitoring validation tests to new config shape. |
| bootstrap/bootstrap.go | Wire beholder setup via BeholderConfig, init logger via Monitoring.LogLevel, and add Pyroscope lifecycle management. |
| aggregator/tests/utils.go | Update test configs to new monitoring struct defaults (empty struct). |
| aggregator/pkg/server.go | Update configuration logging keys to reflect Beholder.Enabled. |
| aggregator/cmd/main.go | Update monitoring init condition and noop monitoring construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bukata-sa
force-pushed
the
fix/monitoring-config-struct
branch
from
2abc31f to
8b8ff02
Compare
makramkd
reviewed
Jul 3, 2026
| // SetupPyroscope starts the Pyroscope profiler. | ||
| func SetupPyroscope(lggr logger.Logger, name string, config PyroscopeConfig) (*pyroscope.Profiler, error) { | ||
| if !config.Enabled { | ||
| return nil, nil |
Collaborator
There was a problem hiding this comment.
question: is returning nil for profiler safer than returning some no-op object?
bukata-sa
force-pushed
the
fix/monitoring-config-struct
branch
from
c622d5c to
8070223
Compare
|
Code coverage report:
|
makramkd
approved these changes
Jul 3, 2026
carte7000
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
monitoring.Configstruct: replaceEnabled/Typefields with typed sub-configs (Beholder.Enabled,Pyroscope.Enabled,PyroscopeConfig.URL), add top-levelLogLevel.SetupPyroscopehelper incommon/monitoring/setup.go;SetupBeholdernow takesBeholderConfigdirectly.Bootstrapper(NewBootstrapper/Stop).setup.go,cmd/verifier/common.go,cmd/executor/common.go,aggregator/cmd/main.go,aggregator/pkg/server.go,indexer/cmd/main.go,indexer/pkg/config/config.go.pyroscope_urltop-level fields.GenerateTemplateConfigfrom token verifier devenv service (no longer needed to extract pyroscope URL).LogLevelandLogStreamingLevelvalidation inConfig.ValidateandBeholderConfig.Validate.Testing
just rebuild-allccv obs upcheck logs and metrics (
ccip_service_started)Checklist
changelogdirectory)just lint fix- no new lint errorsjust generate- mocks and protobufs are up to date