From 959bd03dc483e6ca88ad185d90e6e632041d1f38 Mon Sep 17 00:00:00 2001 From: Slawomir Skowron <329831+szibis@users.noreply.github.com> Date: Mon, 20 Apr 2026 20:27:34 +0200 Subject: [PATCH] fix: relax drilldown metadata fallback and extract literals --- CHANGELOG.md | 9 ++++ internal/proxy/coverage_gaps_test.go | 39 +++++++++++---- internal/proxy/drilldown.go | 42 ++++++++++++---- internal/proxy/drilldown_coverage_test.go | 10 ++-- internal/translator/translator.go | 58 +++++++++++++++++++++++ internal/translator/translator_test.go | 10 ++++ 6 files changed, 145 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bee855a..5e66a1c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - release/metadata: synchronized release metadata for v1.10.2. +### Bug Fixes + +- drilldown/metadata: retry relaxed metadata candidates after successful-empty strict scans for native streams, native field names, detected fields, and detected labels so Drilldown labels/fields stay populated when strict parser filters over-constrain the sample. +- translator/patterns: translate `pattern`/`extract` stages without named captures into line filters instead of emitting invalid VictoriaLogs `extract` pipes such as `extract "Metrics"`. + +### Tests + +- drilldown/translator: update fallback coverage for empty strict scans and add regression cases for literal `extract`/`pattern` stage translation. + ## [1.10.2] - 2026-04-20 ### Bug Fixes diff --git a/internal/proxy/coverage_gaps_test.go b/internal/proxy/coverage_gaps_test.go index 3b2c69d2..c2878906 100644 --- a/internal/proxy/coverage_gaps_test.go +++ b/internal/proxy/coverage_gaps_test.go @@ -520,7 +520,7 @@ func TestDetectedFieldValues_FieldFilterFallbackKeepsValuesVisible(t *testing.T) } } -func TestDetectedFields_EmptyStrictQueryDoesNotRelaxCandidates(t *testing.T) { +func TestDetectedFields_EmptyStrictQueryRelaxesCandidates(t *testing.T) { const strictToken = "strict-only" var fieldNameQueries []string @@ -565,13 +565,23 @@ func TestDetectedFields_EmptyStrictQueryDoesNotRelaxCandidates(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } - if len(fieldNameQueries) != 1 { - t.Fatalf("expected only the strict native field-name lookup, got %v", fieldNameQueries) + if len(fieldNameQueries) < 2 { + t.Fatalf("expected strict+relaxed native field-name lookups, got %v", fieldNameQueries) } - for _, got := range scanQueries { - if !strings.Contains(got, strictToken) { - t.Fatalf("expected scan lookup to preserve strict filter, got %q", got) - } + if !strings.Contains(fieldNameQueries[0], strictToken) { + t.Fatalf("expected first native field-name lookup to stay strict, got %v", fieldNameQueries) + } + if strings.Contains(fieldNameQueries[len(fieldNameQueries)-1], strictToken) { + t.Fatalf("expected final native field-name lookup to relax whole-query filters, got %v", fieldNameQueries) + } + if len(scanQueries) < 2 { + t.Fatalf("expected strict+relaxed field scans, got %v", scanQueries) + } + if !strings.Contains(scanQueries[0], strictToken) { + t.Fatalf("expected first field scan to stay strict, got %v", scanQueries) + } + if strings.Contains(scanQueries[len(scanQueries)-1], strictToken) { + t.Fatalf("expected final field scan to relax whole-query filters, got %v", scanQueries) } var resp map[string]interface{} @@ -579,8 +589,19 @@ func TestDetectedFields_EmptyStrictQueryDoesNotRelaxCandidates(t *testing.T) { t.Fatalf("unmarshal response: %v", err) } fields, _ := resp["fields"].([]interface{}) - if len(fields) != 0 { - t.Fatalf("expected empty detected_fields payload for strict empty query, got %v", resp) + if len(fields) == 0 { + t.Fatalf("expected detected_fields payload after relaxed fallback, got %v", resp) + } + foundStatus := false + for _, raw := range fields { + item, _ := raw.(map[string]interface{}) + if item["label"] == "status" { + foundStatus = true + break + } + } + if !foundStatus { + t.Fatalf("expected relaxed detected_fields payload to include status, got %v", resp) } } diff --git a/internal/proxy/drilldown.go b/internal/proxy/drilldown.go index 362b10b8..ab8650fa 100644 --- a/internal/proxy/drilldown.go +++ b/internal/proxy/drilldown.go @@ -1133,7 +1133,7 @@ func (p *Proxy) detectFields(ctx context.Context, query, start, end string, line candidates := fieldDetectionQueryCandidates(query) hadScanFailure := false var lastErr error - for _, candidate := range candidates { + for i, candidate := range candidates { logsqlQuery, err := p.translateQuery(candidate) if err != nil { lastErr = err @@ -1183,13 +1183,10 @@ func (p *Proxy) detectFields(ctx context.Context, query, start, end string, line p.setCachedDetectedFields(ctx, query, start, end, lineLimit, fieldList, fieldValues) return fieldList, fieldValues, nil } - if len(candidates) > 1 && !hadScanFailure { - emptyFields := []map[string]interface{}{} - emptyValues := map[string][]string{} - p.setCachedDetectedFields(ctx, query, start, end, lineLimit, emptyFields, emptyValues) - return emptyFields, emptyValues, nil + if i+1 < len(candidates) { + p.observeInternalOperation(ctx, "discovery_fallback", "detected_fields_empty_primary", 0) + continue } - break } if len(nativeFields) > 0 { @@ -1203,6 +1200,12 @@ func (p *Proxy) detectFields(ctx context.Context, query, start, end string, line p.setCachedDetectedFields(ctx, query, start, end, lineLimit, fieldList, fieldValues) return fieldList, fieldValues, nil } + if !hadScanFailure && lastErr == nil { + emptyFields := []map[string]interface{}{} + emptyValues := map[string][]string{} + p.setCachedDetectedFields(ctx, query, start, end, lineLimit, emptyFields, emptyValues) + return emptyFields, emptyValues, nil + } return nil, nil, lastErr } @@ -1503,14 +1506,22 @@ func (p *Proxy) fetchNativeFieldNamesForCandidate(ctx context.Context, candidate func (p *Proxy) fetchNativeFieldNames(ctx context.Context, query, start, end string) ([]string, error) { var lastErr error - for _, candidate := range fieldDetectionQueryCandidates(query) { + candidates := fieldDetectionQueryCandidates(query) + for i, candidate := range candidates { fields, err := p.fetchNativeFieldNamesForCandidate(ctx, candidate, start, end) if err != nil { lastErr = err continue } + if len(fields) == 0 && i+1 < len(candidates) { + p.observeInternalOperation(ctx, "discovery_fallback", "native_field_names_empty_primary", 0) + continue + } return fields, nil } + if lastErr == nil { + return []string{}, nil + } return nil, lastErr } @@ -1615,6 +1626,7 @@ func (p *Proxy) resolveNativeDetectedField(ctx context.Context, query, start, en } if i+1 < len(queryCandidates) { p.observeInternalOperation(ctx, "discovery_fallback", "native_detected_field_empty_primary", 0) + continue } return "", false, nil } @@ -1654,7 +1666,8 @@ func (p *Proxy) detectNativeLabels(ctx context.Context, query, start, end string func (p *Proxy) fetchNativeStreams(ctx context.Context, query, start, end string) (*vlStreamsResponse, error) { var lastErr error - for _, candidate := range fieldDetectionQueryCandidates(query) { + candidates := fieldDetectionQueryCandidates(query) + for i, candidate := range candidates { logsqlQuery, err := p.translateQuery(candidate) if err != nil { lastErr = err @@ -1678,8 +1691,15 @@ func (p *Proxy) fetchNativeStreams(ctx context.Context, query, start, end string lastErr = err continue } + if len(parsed.Values) == 0 && i+1 < len(candidates) { + p.observeInternalOperation(ctx, "discovery_fallback", "native_streams_empty_primary", 0) + continue + } return &parsed, nil } + if lastErr == nil { + return &vlStreamsResponse{}, nil + } return &vlStreamsResponse{}, lastErr } @@ -1754,9 +1774,13 @@ func (p *Proxy) detectScannedLabels(ctx context.Context, query, start, end strin summaries := scanDetectedLabelSummaries(body, p.labelTranslator) if len(summaries) == 0 && i+1 < len(candidates) { p.observeInternalOperation(ctx, "discovery_fallback", "detected_labels_empty_primary", 0) + continue } return summaries, nil } + if lastErr == nil { + return map[string]*detectedLabelSummary{}, nil + } return nil, lastErr } diff --git a/internal/proxy/drilldown_coverage_test.go b/internal/proxy/drilldown_coverage_test.go index 355abca1..1be2e5ec 100644 --- a/internal/proxy/drilldown_coverage_test.go +++ b/internal/proxy/drilldown_coverage_test.go @@ -132,7 +132,7 @@ func TestFetchNativeFieldValues_RelaxesAfterSuccessfulEmptyPrimary(t *testing.T) } } -func TestDetectScannedLabels_DoesNotRelaxOnSuccessfulEmptyPrimary(t *testing.T) { +func TestDetectScannedLabels_RelaxesOnSuccessfulEmptyPrimary(t *testing.T) { var calls atomic.Int32 backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/select/logsql/query" { @@ -153,11 +153,11 @@ func TestDetectScannedLabels_DoesNotRelaxOnSuccessfulEmptyPrimary(t *testing.T) if err != nil { t.Fatalf("detectScannedLabels returned error: %v", err) } - if len(labels) != 0 { - t.Fatalf("expected empty primary response to stop without relaxed fallback, got %#v", labels) + if labels["service_name"] == nil { + t.Fatalf("expected relaxed fallback labels, got %#v", labels) } - if got := calls.Load(); got != 1 { - t.Fatalf("expected exactly one backend request, got %d", got) + if got := calls.Load(); got != 2 { + t.Fatalf("expected strict+relaxed backend requests, got %d", got) } } diff --git a/internal/translator/translator.go b/internal/translator/translator.go index 2850ef10..b732bf45 100644 --- a/internal/translator/translator.go +++ b/internal/translator/translator.go @@ -322,6 +322,12 @@ func translatePipelineStage(stage string, labelFn LabelTranslateFunc) string { // rejects these, so treat them as a no-op stage. return "" } + if !hasNamedExtractPlaceholder(patternExpr) { + if filter, ok := patternExpressionToLineFilter(patternExpr); ok { + return filter + } + return "" + } return "| extract " + patternExpr } if strings.HasPrefix(stage, "regexp ") { @@ -334,6 +340,12 @@ func translatePipelineStage(stage string, labelFn LabelTranslateFunc) string { if isNoopPatternExpression(patternExpr) { return "" } + if !hasNamedExtractPlaceholder(patternExpr) { + if filter, ok := patternExpressionToLineFilter(patternExpr); ok { + return filter + } + return "" + } return "| extract " + patternExpr } @@ -400,6 +412,52 @@ func isNoopPatternExpression(expr string) bool { } } +func hasNamedExtractPlaceholder(expr string) bool { + pattern, ok := extractPatternExpressionValue(expr) + if !ok { + return false + } + for { + start := strings.IndexByte(pattern, '<') + if start < 0 { + return false + } + pattern = pattern[start+1:] + end := strings.IndexByte(pattern, '>') + if end < 0 { + return false + } + token := strings.TrimSpace(pattern[:end]) + if token != "" && token != "_" { + return true + } + pattern = pattern[end+1:] + } +} + +func patternExpressionToLineFilter(expr string) (string, bool) { + pattern, ok := extractPatternExpressionValue(expr) + if !ok { + return "", false + } + return "~" + strconv.Quote(patternFilterValueToRegex(pattern)), true +} + +func extractPatternExpressionValue(expr string) (string, bool) { + expr = strings.TrimSpace(expr) + if expr == "" { + return "", false + } + if strings.HasPrefix(expr, "`") && strings.HasSuffix(expr, "`") && len(expr) >= 2 { + return expr[1 : len(expr)-1], true + } + unquoted, err := strconv.Unquote(expr) + if err != nil { + return "", false + } + return unquoted, true +} + // translateLabelFilter handles label comparison filters. func translateLabelFilter(stage string, labelFn LabelTranslateFunc) string { if chained, ok := translateLogicalLabelFilterChain(stage, labelFn); ok { diff --git a/internal/translator/translator_test.go b/internal/translator/translator_test.go index c175c9c5..51abc4eb 100644 --- a/internal/translator/translator_test.go +++ b/internal/translator/translator_test.go @@ -96,6 +96,16 @@ func TestTranslateLogQL(t *testing.T) { logql: `{app="nginx"} | pattern " - - <_>"`, want: `app:=nginx | extract " - - <_>"`, }, + { + name: "pattern parser without named field falls back to line filter", + logql: `{app="nginx"} | pattern "Metrics"`, + want: `app:=nginx ~"Metrics"`, + }, + { + name: "extract parser without named field falls back to line filter", + logql: `{app="nginx"} | extract "Metrics"`, + want: `app:=nginx ~"Metrics"`, + }, { name: "pattern wildcard no-op is dropped", logql: `{app="nginx"} | pattern "(.*)" | status="500"`,