cluster: support ng-monitoring config via server_configs and per-instance overrides#2685
cluster: support ng-monitoring config via server_configs and per-instance overrides#2685boltandrke wants to merge 2 commits intopingcap:masterfrom
Conversation
…ance overrides Add ng_monitoring to ServerConfigs and NgMonitoringConfig to PrometheusSpec so that ng-monitoring configuration (e.g. storage.type, storage.path, continuous_profiling.*) can be set through the topology YAML. Global config is set under server_configs.ng_monitoring, and per-instance overrides under monitoring_servers[].ng_monitoring_config, following the same merge pattern as PD/TiKV/TiDB. Ref pingcap#2325
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @boltandrke! It looks like this is your first PR to pingcap/tiup 🎉 |
|
|
ti-chi-bot
bot
added
the
size/L
| pds := []string{} | ||
| pdAddrs := []string{} | ||
| if servers, found := topoHasField("PDServers"); found { | ||
| for i := 0; i < servers.Len(); i++ { | ||
| pd := servers.Index(i).Interface().(*PDSpec) | ||
| pds = append(pds, fmt.Sprintf("\"%s\"", utils.JoinHostPort(pd.Host, pd.ClientPort))) | ||
| pdAddrs = append(pdAddrs, utils.JoinHostPort(pd.Host, pd.ClientPort)) |
There was a problem hiding this comment.
So this renames pds to pdAddrs and adds quotes around it? Is that needed?
| "advertise-address": utils.JoinHostPort(i.GetHost(), spec.NgPort), | ||
| "log.path": paths.Log, | ||
| "log.level": "INFO", | ||
| "pd.endpoints": pdAddrs, |
There was a problem hiding this comment.
| "pd.endpoints": pdAddrs, | |
| "pd.endpoints": strconv.Quote(pdAddrs), |
Maybe adding a quote here and using strconv.Quote() might be better
|
/cc @xhebox |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2685 +/- ##
==========================================
- Coverage 42.37% 42.32% -0.05%
==========================================
Files 424 424
Lines 47043 47052 +9
==========================================
- Hits 19932 19911 -21
- Misses 24425 24452 +27
- Partials 2686 2689 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
Add explicit test assertion verifying that pd.endpoints with multiple PD addresses serializes to a correct TOML array: endpoints = ["10.0.1.10:2379", "10.0.1.11:2379"] This confirms the TOML encoder handles []string correctly, matching the old template-based output format.
|
Thanks for the review @dveeden! Regarding the
[pd]
endpoints = ["10.0.1.10:2379", "10.0.1.11:2379"]This matches the output produced by the old template. I have pushed a new commit adding an explicit test assertion that verifies this behavior with multiple PD addresses, confirming the serialization is correct. |
@boltandrke did you sign the CLA? |
Now I did! |
3 participants
What problem does this PR solve
Adds support for configuring ng-monitoring via topology YAML, addressing
the gap described in #2325.
Currently there is no way to set ng-monitoring configuration (e.g.
storage.type,storage.path,continuous_profiling.*) throughthe topology YAML. The
NgMonitoringConfigtemplate is hardcoded withno user-config merge step, unlike other components (PD/TiKV/TiDB) which
support
server_configs+ per-instanceconfigoverrides.What is changed and how it works
ServerConfigs.NGMonitoring— addedng_monitoring map[string]anytoServerConfigsfor global ng-monitoring configPrometheusSpec.NgMonitoringConfig— addedng_monitoring_config map[string]anyfor per-instance overridesMonitorInstance.InitConfig— replaced template-based ngmonitoring.toml generation with map-based approach usingMerge2Toml, following the same pattern as PD/TiKV/TiDBng_monitoringandng_monitoring_configsectionsUsage
Check List
go build ./...)go test ./pkg/cluster/spec/...)TestNGMonitoringServerConfig)make check— 0 issues)Ref #2325