Ingestion clock skew detection (#279)#282
Merged
Merged
Conversation
aggregate maxPastMs/maxFutureMs/count/lastSeenAt in one query instead of a JS loop over every row. cast metadata jsonb fields to float8 before MAX to avoid a lexicographic max bug, with a regression test for it.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
Full backend run with coverage over the final state of this branch ( That is the last verification this branch was waiting on. Everything else was already reported: projects 102, metering 49, ingestion and config suites, frontend banner 7, shared 177, frontend 79, |
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.
Closes the silent failure mode behind #279.
The problem
A log ingested with a client-supplied
timefar from the server clock is stored and searchable, but threshold alert rules count logs in[now - time_window, now], so they can never see it. In #279 an Ubuntu host shipped through rsyslog into Fluent Bit, whose Lua filter built atimeabout 27 hours in the past. Ingestion worked, the rule was enabled, the webhook channel tested fine, and no alert ever fired. Nothing in the product said why. Diagnosis took a GitHub issue round trip and a manual SQL query comparingNOW()to the storedtime.What this does
Detects a
timemore than 24 hours in the past or more than 5 minutes ahead of the server clock, counts it, and surfaces it. The log is never rejected: backfilling historical data stays valid.The 24 hour threshold is derived, not chosen. It is the maximum configurable alert
time_window(alerts/routes.ts), so past that point no threshold rule in the product can match the log. That makes the warning an exact statement rather than a heuristic.Detection is always on. There is no configuration to get wrong: a feature whose whole job is to make a silent failure visible should not ship with a knob that silently switches it off.
How it works
ingestionService.ingestLogs, the single choke point for every log path (batch, Fluent Bit/NDJSON, OTLP, receivers). Cost is one subtraction per record and oneDate.now()per batch, no second pass.ingestion.timestamp_skewmetering event per skewed batch, carrying the count and the worst delta per direction. No migration:metering_eventsalready hadmetadata JSONB.GET /api/v1/projects/:id/ingestion-healthdriving a warning banner on the project overview for tenants. The banner is the one that actually solves [BUG] Las alertas de umbral (Threshold) no se disparan para logs de Ubuntu #279, since it is where the person who misconfigured the shipper looks. Its copy carries the whole diagnosis, including the fix, because there is no public ingestion doc to link to yet.An
Invalid Dateis deliberately never counted as skew:now - NaNisNaNand both comparisons are false. What the reservoir does with a malformed timestamp is a separate hole, and conflating the two would make this signal mean two different things.Also fixed
METERING_ENABLED=falsesilently suppressed everyingestion.*counter, includingingestion.pii_rejected, the safety counter for records dropped because PII masking failed. That toggle governs usage metering, while theingestion.*counters are operational health signals already excluded from tenant usage breakdowns by a prefix filter. They now bypass it. The buffer hard cap still applies: that is a memory valve, not a feature switch.Notes for review
Two bugs worth knowing about, both caught in review and fixed here:
{:else if stats}branch, reachable only whenisEmptyis false. ButisEmptyderives from clock-windowed queries, exactly the ones skewed logs fall outside of, so in the precise [BUG] Las alertas de umbral (Threshold) no se disparan para logs de Ubuntu #279 repro (a misconfigured shipper as the project's only source) the page showed "No data yet" and hid the banner explaining it. It is now hoisted above the branch chain.The per-project aggregate casts
metadata->>'maxPastMs'tofloat8beforeMAX, since the field is text and a lexicographic max is a real bug ("9000000" sorts above "97200000"). There is a regression test for exactly that.Verification
Targeted suites green: projects 102, metering 49, ingestion, config, frontend banner 7, shared 177, frontend 79.
svelte-checkclean of new errors.tsc --noEmitclean.A full backend run with coverage is still executing as this PR opens; the previous full run over this branch was green (exit 0, coverage gate met) and the changes since were the metering gate, the SQL aggregation and the threshold constants, all covered by the targeted suites above. I will report the full result on this PR.
Deferred work is tracked with its rationale, including extending skew detection to spans and metrics, which have the same exposure and are not covered here.