Fix metrics-collector panic on invalid regular expression filters - #2708
Fix metrics-collector panic on invalid regular expression filters#2708AdeshDeshmukh wants to merge 1 commit into
Conversation
GetFilterRegexpList discarded the error from regexp.Compile, so an invalid user-provided filter produced a nil *regexp.Regexp that panicked the metrics-collector sidecar when matched against log lines, leaving the Trial without reported metrics. Handle the compile error and fail fast with a descriptive message identifying the invalid filter. Filters are also now compiled once at startup instead of on every log line in the watch loop. Fixes kubeflow#2707 Signed-off-by: Adesh Deshmukh <adeshkd123@gmail.com>
|
[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 |
vjkumar2756
left a comment
There was a problem hiding this comment.
@AdeshDeshmukh
This is a great fix that addresses both a stability issue (preventing potential runtime panics) and a performance bottleneck in the file metrics collector.
some Suggestions:
-
Duplicate Error Message (Bug): In main.go,
klog.Fatalf("Invalid metric filter: %v", err)duplicates the error prefix because GetFilterRegexpList already returnsinvalid metric filter %q. Change toklog.Fatalf("Failed to compile metric filters: %v", err). -
Sub-optimal Memory Allocation: In GetFilterRegexpList(),
make([]*regexp.Regexp, 0, len(filters))is called before handlinglen(filters) == 0, allocating 0 capacity. Check/set default filters before allocatingregexpList. -
Typo: In main.go comment, fix "regural expressions" -> "regular expressions".
What this PR does / why we need it:
Fixes a crash in the
file-metricscollectorsidecar when a user-provided metrics filter is not a valid regular expression.GetFilterRegexpListdiscarded the error fromregexp.Compile, so an invalid filter produced anil *regexp.Regexpentry in the returned list. The first pattern match against a log line (FindAllStringSubmatch) then dereferenced the nil regexp and panicked the metrics-collector container — either inparseLogsInTextFormator in the watch loop inmain.go— so no metrics were ever reported for the Trial, with no hint that the user's filter was the problem.With this change:
invalid metric filter "...": ...), consistent with the existing error handling inwatchMetricsFile.GetFilterRegexpListnow returns an error, propagated throughparseLogsInTextFormat/CollectObservationLog.Behavior change note: previously an invalid filter silently matched nothing; now the sidecar exits immediately with a clear error. This is intentional — silent failure is what left Trials stuck without metrics.
Tests: added
TestGetFilterRegexpListcovering invalid filter (returns descriptive error, no panic), empty filters (default filter fallback), and valid multi-filter compilation. ExistingTestCollectObservationLogcases pass unchanged.Which issue(s) this PR fixes:
Fixes #2707
Checklist: