labstack / labstack/fanout

fix(ingest): unvalidated client event times create permanent future rollup buckets

Open
#216 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
HTML
Stars
4
Forks
0
Avg merge
4h 4m
Merged PRs (30d)
57

Description

Summary

Fanout stores client-supplied OTLP event timestamps without any sanity bound, and three rollup queries filter buckets with a lower bound only. A single source with a wrong clock — a device, a VM with broken NTP, a container — can write service_rollup buckets arbitrarily far into the future. Those buckets then match every windowed query forever, and the retention sweep never removes them because it only prunes the past.

This is not a mobile-SDK feature request. #172 settled that boundary and the docs reflect it (docs/operations.md, "Mobile boundary"). This is a data-integrity gap in the topology that #172 approved: mobile SDK -> customer-controlled Collector/gateway -> private Fanout. A gateway forwards client spans verbatim, so the device clock becomes Fanout's bucket key. Backend services with drifting clocks hit the same path.

Evidence

Ingest copies the client timestamp straight onto the row, unvalidated:

  • internal/ingest/server.go:93StartUnixNanos: int64(sp.StartTimeUnixNano)
  • internal/ingest/server.go:156TimeUnixNanos: int64(lr.TimeUnixNano)
  • internal/ingest/server.go:207,231,255,282,305 — metric data-point timestamps

Span duration is clamped (internal/ingest/server.go:338), but the absolute timestamp is not, and there is no other clamp, skew check, or drift correction in internal/ingest/.

Rollups bucket on that event time:

  • internal/query/duck.go:1232date_trunc('minute', start_time) AS bucket

Six query helpers bound their window below but not above. Three read rollups, three read raw tables — so this is not confined to the rollup path:

Site Function Table Filter
duck.go:1684 LatencyOverview service_rollup bucket >= now() - INTERVAL n MINUTE
duck.go:1723 LogsSamples logs time >= now() - INTERVAL n MINUTE
duck.go:1756 Throughput service_rollup bucket >= now() - INTERVAL n MINUTE
duck.go:1787 ServiceThroughput service_rollup bucket >= now() - INTERVAL n MINUTE
duck.go:1819 ErrorRoutes logs time >= now() - INTERVAL n MINUTE
duck.go:1855 ErrorRouteDetails spans start_time >= now() - INTERVAL n MINUTE

A repo-wide search for now() - INTERVAL finds no upper bound on any of them; the only < now() in non-test code is the retention sweep.

Retention only prunes the past:

  • internal/query/duck.go:551DELETE FROM %s WHERE bucket < now() - INTERVAL %d DAY

This prunes the three rollup tables only, and it prunes them by event time (bucket). Raw spans/logs are pruned separately by PruneParquetPass (internal/query/duck.go:529), which passes a cutoff of now - RetentionDays and compares it against batch.metadata.MaxIngestedNanos (internal/telemetry/parquet.go:578) — i.e. ingest time.

That asymmetry decides how bad each half is:

Store Pruned by Future-dated row
service_rollup / edge_rollup / endpoint_rollup event time (bucket) never pruned — permanent
raw spans / logs (Parquet) ingest time (MaxIngestedNanos) pruned RetentionDays after ingest

So the rollup-backed helpers are permanently wrong; the raw-table helpers are wrong for up to RetentionDays and then self-heal. Both still need the upper bound — the rollup case is just the one that never recovers on its own.

Failure scenario

A device (or host) with a clock set to 2035 sends 10k spans through the customer's gateway.

  1. Ingest accepts them; rows carry start_time in 2035.
  2. The rollup pass creates service_rollup buckets in 2035.
  3. All six helpers above match those rows, because a lower bound alone is satisfied by any future timestamp. So does every other window size, permanently. The raw-table helpers (LogsSamples, ErrorRoutes, ErrorRouteDetails) are hit directly and do not need the rollup pass to have run.
  4. ServiceThroughput divides the summed span count by windowMinutes, so it reports a spans-per-minute rate that includes ten years of spans that did not happen in that window.
  5. The retention sweep never deletes the rows — bucket < now() - N DAY does not match a future bucket. The distortion is permanent until someone deletes rows by hand.

The mirror case is the same asymmetry running the other way. A span arriving with an event time older than RetentionDays is accepted and rolled up, and the next maintenance pass deletes its rollup row (bucket < now() - N DAY matches immediately) while the raw Parquet batch survives, because that batch's MaxIngestedNanos is the receipt time. The row is then visible to raw SQL but absent from every rollup-backed view — the two halves of the product disagree about whether the data exists, indefinitely.

Note that internal/observability/* is not affected — it passes explicit scope.Start/scope.End bounds (overview.go:66, performance.go:234, logs.go:51). The gap is confined to the six duck.go helpers above.

Related, lower priority: internal/query/sql.go:350 and internal/query/schema.go:47 tell users and the AI chat to write start_time/time > now() - INTERVAL ..., propagating the same lower-bound-only pattern into ad-hoc and generated SQL. Worth updating the guidance text alongside this, but it is not the bug.

Proposed fix

Two independent changes; either alone is an improvement, both are cheap.

1. Bound the queries (defensive, no behavior change for healthy data).

Add an upper bound to all six helpers, so a future-dated row cannot leak into a window that does not contain it:

WHERE bucket >= now() - INTERVAL %d MINUTE
  AND bucket <= now()

and the equivalent AND time <= now() / AND start_time <= now() for the raw-table helpers.

2. Reject or clamp implausible event times at ingest (root cause).

In internal/ingest/server.go, bound StartTimeUnixNano / TimeUnixNano against receipt time — which is already computed as now at server.go:73 — with a configurable tolerance. Suggested defaults, matching what other receivers do:

  • future tolerance: a few minutes (reject or clamp anything beyond)
  • past tolerance: RetentionDays (rows older than retention will be pruned anyway; accepting them is a lie)

Open question for whoever picks this up: clamp or drop? Clamping preserves the row but fabricates a timestamp; dropping loses data but keeps the time axis honest. A third option is to keep the row at its original time and exclude out-of-bounds rows from rollups only. Recommend deciding this explicitly rather than by implementation accident, and emitting a counter either way so operators can see it happening.

A follow-on worth considering separately: OTLP has no equivalent of Sentry's sent_at envelope header ("used for clock drift correction of the event timestamp" — https://develop.sentry.dev/sdk/data-model/envelopes/), so Fanout cannot compute a real per-source drift correction today. Bounding is the pragmatic fix; drift correction would need a non-standard header and should not block this.

Non-goals

This issue deliberately stays inside the boundary set by #172. It does not propose:

  • a Fanout mobile SDK, or direct mobile-to-Fanout ingestion
  • per-app or public-client credentials, attestation, or CORS
  • RUM sessions, session.id as a first-class dimension, or crash/symbolication pipelines
  • rate limiting or quotas on the OTLP listeners

Verification

  • A span or log with an event time far in the future does not appear in any of the six helpers for a window that does not contain it — rollup-backed and raw-table-backed alike.
  • ServiceThroughput reports a rate consistent with the rows actually inside the window.
  • A span with an event time older than RetentionDays is handled per the decision above, and the outcome is observable (counter or log), not silent.
  • Rows with plausible timestamps are unaffected; existing rollup and retention tests stay green.
  • A future-dated row no longer creates a permanently unprunable rollup bucket.
  • Raw and rollup views agree on whether an out-of-bounds row exists, rather than one pruning it and the other retaining it.
  • Both OTLP/gRPC and OTLP/HTTP go through the same validation — no transport-specific behavior (per #172 §3).
  • just check and just test-race pass.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the timestamp assignments and receipt time in internal/ingest/server.go, then inspect the six windowed helpers and retention paths in internal/query/duck.go. Review the raw-data pruning logic in internal/telemetry/parquet.go and decide the out-of-bounds timestamp behavior before implementing it. Done means future and overly old events are handled consistently across OTLP transports, query views, rollups, retention, and the listed verification checks; run just check and just test-race.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, observability
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.