fix(workers): guard metricViewConfig before dereferencing queryConfigs in telemetry monitor evaluation - #3112
Open
Nedjagang wants to merge 1 commit into
Conversation
…s in telemetry monitor evaluation Every monitor-type handler in MonitorTelemetryMonitor.ts (Metric, Kubernetes, Docker, Host, Podman, Proxmox, IoT, Docker Swarm, Ceph) null-checks the top-level monitor step config but then dereferences `config.metricViewConfig.queryConfigs` without checking that `metricViewConfig` itself is present. A monitor whose step data is missing metricViewConfig (e.g. saved before the form/template flow finished populating it) throws an unhandled TypeError on every evaluation attempt. Since the evaluation job fails instead of completing, it keeps getting re-queued, which can inflate queue-size-based autoscaling metrics and drive worker replicas up indefinitely chasing a job that can never succeed. Add the same missing-config guard already used for the top-level config to metricViewConfig in each of the 9 handlers, so a malformed monitor throws a clean BadDataException instead of crashing.
Contributor
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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.
Summary
MonitorTelemetryMonitor.ts(Metric, Kubernetes, Docker, Host, Podman, Proxmox, IoT, Docker Swarm, Ceph) null-checks the top-level monitor step config, then immediately dereferencesconfig.metricViewConfig.queryConfigswithout checking thatmetricViewConfigitself is present.metricViewConfig(e.g. saved mid-flow before a template/form finished populating it) throws an unhandledTypeError: Cannot read properties of undefined (reading 'queryConfigs')on every evaluation attempt.metricViewConfigin all 9 affected handlers, so a malformed monitor throws a cleanBadDataExceptioninstead of crashing and retrying forever.Root cause (observed in production)
We hit this with monitor records shaped like
{ "cephMonitor": { "clusterIdentifier": "..." } }— noresourceFilters,metricViewConfig, orrollingTime— created via a template/onboarding flow that appears to save the step data before the rest of the form is filled in. Every scheduled evaluation of that monitor crashed identically, and since the worker treats a thrown job as "needs retry," it never stopped retrying. This happened simultaneously across all 9 vulnerable monitor types (each project had one[Template] X Monitorper type), producing a sustained, artificial load signal that scaled workers to near their configured ceiling.Fix
Minimal, mechanical, no behavior change for well-formed monitors — just fails fast with a clear error instead of throwing a raw
TypeErrorfor malformed ones.Test plan
npx tsc --noEmiton theAppworkspace shows no new errors introduced by this change (pre-existing unrelated errors in the workspace are untouched)MonitorTelemetryMonitor.tsthis should extend — happy to add a case per handler exercising a monitor step withmetricViewConfigunset if pointed at the right test file.