PMM-15201 Correct status after change valkey in PMM admin. - #5714
PMM-15201 Correct status after change valkey in PMM admin.#5714JiriCtvrtka wants to merge 10 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5714 +/- ##
==========================================
- Coverage 43.59% 43.53% -0.07%
==========================================
Files 415 303 -112
Lines 43134 32669 -10465
==========================================
- Hits 18804 14222 -4582
+ Misses 22454 16965 -5489
+ Partials 1876 1482 -394
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Fixes Valkey exporter log-level argument handling so pmm-managed generates the correct flag form (--log-level=... instead of --log.level=...) and aligns pmm-admin inventory change-agent valkey-exporter log-level validation with add agent behavior, preventing valkey_exporter from exiting early and incorrectly landing the agent in DONE.
Changes:
- Introduces a shared log-level arg helper (
withLogLevelFlag) plus a Valkey-specific helper (withValkeyLogLevel) emitting--log-level=...and translating storedfatal→error. - Updates Valkey exporter config generation to use the Valkey-specific flag spelling.
- Tightens
pmm-admin inventory change-agent valkey-exporterto reject--log-level=fataland adds/extends tests for the new behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| managed/services/agents/valkey.go | Switch Valkey exporter arg generation to --log-level via withValkeyLogLevel. |
| managed/services/agents/valkey_test.go | Add a per-level matrix test ensuring --log-level is used and --log.level is never emitted. |
| managed/services/agents/log_level.go | Refactor log-level arg building into a reusable helper and add Valkey-specific behavior. |
| managed/services/agents/log_level_test.go | New unit tests for kingpin-style and Valkey-style log level arg generation and fallback behavior. |
| admin/commands/inventory/change_agent_valkey_exporter.go | Change CLI flags to reject fatal for Valkey exporter log level. |
| admin/commands/inventory/change_agent_valkey_exporter_test.go | Add parser test ensuring --log-level=fatal is rejected. |
Suppressed comments (1)
managed/services/agents/log_level_test.go:103
- Same parallel subtest capture issue here:
name/tcfrom the range loop are referenced inside at.Parallel()subtest, which can lead to using the wrong iteration’s values. Shadow both variables inside the loop before callingt.Run.
} {
t.Run(name, func(t *testing.T) {
t.Parallel()
actual := withValkeyLogLevel(nil, tc.level, supported)
assert.Equal(t, tc.expected, actual)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Keep log_level.go exporter-agnostic: the valkey flag spelling now lives at its only call site in valkey.go. Drop the test cases that duplicated the coverage already provided by TestValkeyExporterConfig and DefaultTimeoutUsesFlag.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds reusable log-level flag construction, applies the standard ChangesValkey log-level handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PMM-15201
Problem
valkey_exporterparses its command line with the standard libraryflagpackage, notkingpin like the rest of the exporters. pmm-managed was still building its args with the
shared
withLogLevel()helper, which appends--log.level=<level>. The exporter rejectedthe unknown flag, printed its usage and exited with code 2 immediately after start, so the
agent ended up reported as
DONEinstead ofRUNNING.The bug is hit whenever a log level is stored for a Valkey exporter, i.e. both
pmm-admin inventory add agent valkey-exporter --log-level=...andpmm-admin inventory change-agent valkey-exporter --log-level=....On top of that, the
change-agent valkey-exportercommand accepted--log-level=fatalwhile
add agent valkey-exporterdid not —valkey_exporterhas no fatal level andsilently falls back to
info.Solution
withLogLevel()into a sharedwithLogLevelFlag()and addwithValkeyLogLevel(),which emits
--log-level=<level>forvalkey_exporter.valkeyExporterConfig()now uses it.fatalis translated toerrorfor Valkey (same fallback the other exporterswithout a fatal level already use), so existing agents keep working after upgrade.
pmm-admin inventory change-agent valkey-exporterswitches fromLogLevelFatalChangeFlagstoLogLevelNoFatalChangeFlags, so--log-level=fatalisrejected consistently with the corresponding
add agentcommand.Tests
managed/services/agents/log_level_test.gocovering the kingpin helper (fatalfallback, empty level, pmm-agent older than 2.28, arg appending) and the Valkey helper
(flag spelling and fatal fallback).
TestValkeyExporterConfigextended with a per-level matrix asserting--log-level=isproduced and
--log.levelnever is, plus a case with no log level set.TestValkeyExporterChangeAgentextended with a case asserting--log-level=fatalisrejected by the parser.
Summary by CodeRabbit
Bug Fixes
fataltoerrorwhen necessary.Tests