Repository navigation
fix: line filters whose text is not UTF-8 answer like Loki - #1236
Merged
Merged
Conversation
…lid-utf8-case # Conflicts: # conformance/registry/generated/census.json # conformance/registry/query-plan.json # conformance/reports/census.md
A |= with a text that is not UTF-8 keeps no line, a != keeps every line, and an or chain keeps its valid alternatives, since no stored line holds such bytes; VictoriaLogs rejected the regexp, so the proxy answered 400. Loki's text error body is written as valid UTF-8, one U+FFFD per invalid byte, as Loki writes it.
One translate node for _stream_id:in(), shared by the detected_level matcher and the line filters, and a live capability test that it keeps no row for log and stats queries, run by the version matrix.
github-actions
Bot
added
size/XL
szibis
changed the title
Contributor
PR Quality ReportCompared with the PR base commit; measurements below are scoped to their linked run. Base dddbe51 → PR c5a9bfb; tested merge b6a5b59. Focus signals:
Additional affected checks are listed below. Main-versus-PR whole-process CPU, peak RSS, physical SSD throughput/IOPS and per-layer evictions are not measured here. Synthetic allocations and fleet cache occupancy do not substitute for them. Evidence sources and revision status
Pending, stale, unavailable and skipped sources contribute no measurements. Query semantics catalog: base versus PRDeclared query cases and parser paths in the base and PR source. These counts do not claim execution or parity; the Loki compatibility check supplies live proof.
The matrix is the query catalog for quality and later performance scenarios. Visual evidence remains separate.
Failing case IDs: none. Broader API and Grafana execution scopesExisting CI job logs, verified against the tested revision. Candidate only: these scopes have no paired base run here. Go counts include top-level cases and subtests; repeated identities count once and a failed repeat stays failed. Playwright invocations remain separate. Do not sum profiles as independent API coverage. Failed jobs can contain only partial observed populations.
Showing 5/5 scopes; source links are above. Real workload A/B: base, PR and LokiSelected shape × range × Loki profile rows; not the complete workload baseline.
Showing 20/448 rows; full details remain in the linked A/B artifact/comment. † Loki's cache answered cold; not a computed cold comparison. Not-like-for-like rows are not speedup proof.
Docker CPU percentages sampled within ±1.5s of each request, maximum across that row's runs. Overlapping work and background activity can contribute; these are not query-attributable CPU seconds. This summary's zero may also mean missing samples. Visual proofs and regressionsFailures: none CI regression checks
Showing 60/79 checks; failed and pending checks first. Full job lists remain in the linked runs. No data (sections omitted): Coverage and tests (test count and coverage were not collected); API contract coverage and regression changes (no test outcomes, coverage or declared inventory in either snapshot); Synthetic handler microbenchmarks (no paired base/PR measurements). Required checks decide merge safety. This report summarizes selected evidence; rendering starts no tests or heavy workloads. |
Contributor
✅ Visual smoke: passed with warningsBase
Gate: a base-vs-PR data difference passes when the PR is closer to Loki, and fails when the PR diverges from Loki or Loki cannot decide it (at a range Loki does not hold, a difference passes when the same page proved that query to answer like Loki at a shorter range; otherwise label the pull request dd-landing-1h: pixel diff 10.7%
Core set montages (8): base | PR | Lokidd-fields-1h
dd-service-logs-1h
explore-logs-plain-1h
explore-metric-plain-1h
explore-sum-by-over-binary-1h
explore-topk-by-per-step-1h
explore-unwrap-sum-1h
lvl-explore-rate-errors-by-service-1h
Published inline montages: |
Contributor
⚪ Performance A/B: no change beyond noise448 shape×range — 448 ⚪ same · results vs Loki: 422 same, 0 new difference(s), 23 pre-existing Loki profiles compared (like for like):
Compared with the above-2× baseline: 4 rows (cold 4, warm 0, refresh 0; aligned 1, loki-defaults 3); a new or worsened row fails a pull request. Every shape is within noise of the base build (±30% and 100 ms). 448 unchanged shape×range (within noise)
Cold (first-run) timings — what a user opening a panel sees
† Loki's results cache answered this cold run (its own counters, or identical answers for windows a minute apart): not a computed time, so it is in no ratio. ↺ Loki cold taken from the first set that ran the same query; Loki's cache answered this one. 8 shape×range above 2× Loki already (listed, or not moved by this PR: as slow on the base build)
6 shape×range×mode where Loki did not answer 200: not compared (not a proxy defect). Why these shapes
base |
…rites them The oracle returns Loki's tree as JSON, which writes each byte that is not UTF-8 as U+FFFD, so the proxy's value is compared in that form.
github-actions
Bot
added
size/XL
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
A
|=or!=line filter whose text is not UTF-8 (|= "\xff\xfe") now answers like Loki instead of 400.|=/!=as bytes, so it accepts these queries. The proxy wrote the substring as a VictoriaLogs regexp, which VictoriaLogs refuses to compile ("invalid UTF-8"), so log and metric queries answered 400.|=with such a text keeps no line (_stream_id:in(), the proxy's existing match-no-rows filter);!=keeps every line (no filter);orchain keeps its valid alternatives.translate.TextLineFilter) serves the translator and the plan lowering. Valid texts keep their exact LogsQL.|~ "\xff"was already 400 on both sides, but the proxy echoed the raw bytes._stream_id:in()is now one translate node, shared with the detected_level matcher. A live capability test (TestVLCapability_EmptyInMatchesNothing, run by the version matrix) gates it: it must keep no row for log and stats queries.Proof vs Loki (compose stack, Loki 3.7, VictoriaLogs v1.53.0)
{probe} |= "\xff\xfe"{probe} != "\xff"{probe} |= "\xff" or " b"{probe} |~ "\xff"sum(count_over_time({probe} |= "\xff\xfe" [5m]))instantTestCompat_LineFilterInvalidUTF8LikeLoki,TestCompat_IPLineFilterLikeLokiandTestCompat_LineFilterOrAlternativespass against Loki on the stack.TestVLCapability_EmptyInMatchesNothingpasses on VictoriaLogs v1.53.0 (stack) and v1.49.0 (a standalone container).API data-to-data match vs Loki on this case: 0% → 100% (status, lines, metric value, error bytes). Open differences from Loki: 86 → 85 (regenerated README).
Visual proof: none. Grafana cannot show the case, because no stored line holds non-UTF-8 bytes, so both panels show the same empty or full set (
visual_reasonin the case).Plan / oracles
TestPlanShadow: native 624 of 22,842 rows × 12 profiles, 0 mismatches. The lowering equals the translator on the new rows.TestVLOracleon the 28,663-row corpus, v1.49.0 and v1.53.0: passes with the invalid-UTF-8 skip removed. The new rows read asapp:="…" _stream_id:in()andapp:="…".Performance
Apple M5 Pro, GOMAXPROCS=4,
-count 6, benchstat, base = origin/main with the same benchmark files.|=/!=/|~)WriteLokiErrorBody/invalid_utf8is the rare path where an error text quotes invalid bytes: one extra 212 ns and one allocation per such 400.go test -short ./internal/proxypeak RSS 3.6 GB under GOMEMLIMIT=3GiB.Registry
New and changed rows (
pending: trueuntil owner review):conformance/registry/cases/semantics/line-filter-invalid-utf8.yaml(open → fixed):conformance/registry/vl/capabilities/vl-empty-in-matches-nothing.yaml(new):Decision in
conformance/registry/behaviours/architecture-syntax-package-layering.yaml(new):Security
!=no-filter case drops only that line filter. The stream selector, the tenant (AccountID,extra_stream_filters) and every other stage still apply; the e2e!=returned only the probe stream's lines.|=uses the existing match-no-rows filter, which is now gated on every supported VictoriaLogs version by a live test. A release where_stream_id:in()matched rows would fail CI rather than return lines.Tests
TestTranslateLineFilterInvalidUTF8(log, metric, chains);TestInvalidUTF8RegexpErrorTextLikeLoki(exact Loki bytes through the handler);TestValidUTF8Text;FuzzErrorBodyShapes, whose frontend shape now expects U+FFFD per byte (20 s fuzz clean).TestCompat_LineFilterInvalidUTF8LikeLoki(core group);TestVLCapability_EmptyInMatchesNothing(vl-pinned and the version matrix).go test -shortpasses for internal/proxy, translate, translator, queryplan/... and querycorpus.preflight --quick: 16 of 16 passed. The conformance gate is OK.