Commit 985000e
Add
Add `skip_record_validation_percents_by_type` indexer config for sampled
backfill validation
## Why
During large backfills of already-validated data, per-record JSON schema
validation is wasted work. Every record walks the full schema (regex,
enum, min/max, format, abstract-type discriminators) even though the
source has already been validated upstream. Today there's no way to
trade that cost for throughput.
This adds a config option that skips per-record validation for a chosen
percentage of records, per GraphQL type, while keeping a sampled slice
validated as a canary so schema drift still surfaces. It's a sibling of
the existing `skip_derived_indexing_type_updates` backfill knob and
follows the same shape.
Design notes:
- `skip_record_validation_percents_by_type` maps a type name to a
percent in `[0, 100]`. `0` (or an absent key) validates everything,
`100` skips everything, and values in between sample. The value is the
percentage *skipped*, so `90` skips 90% and validates 10%.
- The skip decision compares a `Zlib.crc32` of the event id
(`type:id@vversion`) against the configured percentage of the CRC32
space. Same event id, same decision, so a retry never flips a record
between validated and skipped, even across pods. `String#hash` won't do
here: its seed is per-process, so two pods would disagree. The `<= 0`
and `>= 100` guards keep both endpoints exact, so no float boundary
error can make a `0` skip a record or a `100` validate one.
- The event envelope is always validated. Only the per-record schema
walk gets sampled.
- Skipping isn't silent. `Processor` counts skipped records per batch
and logs one `RecordValidationSkipped` line with per-type counts, the
same way it logs `ElasticGraphIndexingLatencies`. Logging per record
would drown a `100` percent backfill in log lines, so the count is
aggregated per batch.
- Isolation via re-validation, not an error taxonomy: with validation
off, malformed data surfaces as an exception while building the event's
operations, and there is no bounded list of error types to enumerate. So
when validation was skipped, `Factory#build` rescues anything and then
runs the validation it skipped. If the validator faults the record, the
caller gets a `FailedEventError` carrying **the validator's own
message** - the same one it would have gotten on the validated path,
PII-sanitized - and the rest of the batch still indexes. If the
validator is happy, the error was never about the data (a schema
artifact defect, or a bug), so the original exception is re-raised
untouched. This is why no dedicated error type is needed for the
missing/unknown abstract-type `__typename` case: `Inventor` has
`required: ["__typename"]` plus a `oneOf` whose branches each pin
`__typename` with a `const`, so a missing one fails `required` and an
unknown one fails every branch.
- `::Kernel.raise` is used for the re-raise because `Operation::Factory`
overrides `raise` to stop the class *originating* errors instead of
returning a `BuildResult`. Propagating an error that already escaped a
collaborator is the opposite case, so the guard is deliberately
bypassed, with a comment at the site and a spec that asserts the
original class and message. Asserting on both matters: a plain `raise
exception` would substitute the guard's own error and still satisfy a
bare `raise_error`.
- Fixed on the way: `build_failed_result` could itself raise while
building the operations it attaches to a `FailedEventError`, masking the
malformation it was trying to report. An exception in a `rescue` body
isn't caught by that same `rescue`, so this had to hold before the
re-validation path above could route unvalidated records through it. It
turns out to be a live bug on the *validated* path too: `Widget`
requires `cost`, so a `Widget` without it fails validation, reaches
`build_failed_result`, and dies with `KeyError` building the derived
`WidgetCurrency` target, whose id comes from `cost.currency`. The batch
dies with it and the malformation is never reported. It now falls back
to no operations and logs `FailedEventOperationBuildingFailure`;
`FailedEventError#operations` is already documented as sometimes empty
for exactly this reason.
- With validation off, one class of failure is still not isolated:
malformations that surface only when an operation is serialized for the
datastore. `Update#to_datastore_bulk` is lazy and memoized, so the
rollover index suffix and custom routing value computed in
`Update#metadata` are evaluated inside `router.bulk`, after `build` has
returned - out of reach of any rescue here. Such a batch produces no
partial-failure response, so the queue redelivers all of its events and
the malformed record fails them again on each retry until it drains to
the DLQ. The config documentation says this specifically rather than
implying a broader guarantee. Happy to take it on in a follow-up.
The field defaults to `{}`, so nothing changes unless you set it.
Additive and minor-release-safe.
## What
Config:
```yaml
indexer:
skip_record_validation_percents_by_type:
Widget: 90 # skip validation for 90% of Widget records, validate 10%
Component: 100 # skip validation for all Component records
```
- `config.rb`: new `skip_record_validation_percents_by_type` JSON schema
property (object, per-type number in `[0, 100]`, `additionalProperties:
false`, default `{}`); `convert_values` coerces percents to `Float`. The
`description:` leads with what the setting does, then the indexing-CPU
tradeoff, then what remains unisolated.
- `operation/factory.rb`: new `skip_validation?(type, event)` and the
`CRC32_SPACE_PER_PERCENT` constant; `build` branches on the skip
decision into `build_success_result` or
`build_success_result_isolating_malformed_records`;
`build_failed_result` no longer lets a second failure mask the first.
`BuildResult` gains `type_with_skipped_validation`.
- `processor.rb`: aggregate `RecordValidationSkipped` log per batch when
any record was skipped.
- `indexer.rb`: wire `config.skip_record_validation_percents_by_type`
into the factory.
- `record_preparer.rb`: unchanged. It carried a
`RecordPreparer::UnknownTypeError` in an earlier revision of this PR;
that's gone, and the file no longer appears in the diff.
- RBS signatures updated for all of the above.
- `elasticgraph-local` `config_schema.yaml`: regenerated via
`script/update_config_artifacts`.
## Verification
- `script/run_specs` (COVERAGE=1, real Elasticsearch): 5307 examples, 0
failures. `elasticgraph-indexer` on its own is 264 examples, 0 failures,
at 100% line (592/592) and 100% branch (135/135).
- `script/type_check` (Steep): no type errors.
- `script/lint` (Standard Ruby): 899 files, no offenses.
- `script/spellcheck` (codespell): clean.
- `script/ci_parts/run_misc_checks`: `config_schema.yaml is up-to-date`,
so no artifact drift.
- `bundle exec rake schema_artifacts:check`: up to date (runtime config
only, no artifact changes).
- `bundle exec rake site:validate`: HTML-Proofer clean over 171 files,
149 runs, 0 failures.
- Generated configuration reference checked by hand: the new field, its
text and its examples all appear, and the generated example config still
validates against the schema.
New tests:
- `config_spec.rb`: integer YAML percents coerce to `Float` (`90` to
`90.0`), and out-of-range percents (`100.5`, `-0.1`) are rejected at
config load.
- `operation/factory_spec.rb`: a skipped type builds operations without
record validation; non-skipped types still fail on bad records; envelope
validation still runs for skipped types; partial sampling (stubbed
`Zlib.crc32` for both branches); retry stability; the derived-index path
under skip; a non-coercible `amount_cents` and an unknown abstract-type
`__typename` each reported as a `FailedEventError` carrying the
validator's message; an error the validator has no opinion about
re-raised with its class and message intact; the validated path still
propagating, so the rescue is provably gated; and a malformed event
whose operation building also fails still reporting the malformation,
with a warn log for the error it discarded.
- `processor_spec.rb`: a batch with skips logs one
`RecordValidationSkipped` with the right `count`/`counts_by_type`; a
batch with no skips logs none.
---------
Co-authored-by: Ashit Verma <ashit.kumar.verma@toasttab.com>skip_record_validation_percents_by_type indexer config for sampled backfill validation (#1315)1 parent 2b9463c commit 985000e
11 files changed
Lines changed: 484 additions & 18 deletions
File tree
- elasticgraph-indexer
- lib/elastic_graph
- indexer
- operation
- sig/elastic_graph/indexer
- operation
- spec/unit/elastic_graph/indexer
- operation
- elasticgraph-local/lib/elastic_graph/local/spec_support
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
| 88 | + | |
88 | 89 | | |
89 | 90 | | |
90 | 91 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
| 14 | + | |
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
46 | 75 | | |
47 | 76 | | |
48 | 77 | | |
49 | 78 | | |
50 | 79 | | |
51 | | - | |
| 80 | + | |
52 | 81 | | |
53 | 82 | | |
54 | 83 | | |
| 84 | + | |
55 | 85 | | |
56 | 86 | | |
57 | 87 | | |
| |||
Lines changed: 80 additions & 12 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
| 16 | + | |
16 | 17 | | |
17 | 18 | | |
18 | 19 | | |
| |||
23 | 24 | | |
24 | 25 | | |
25 | 26 | | |
| 27 | + | |
26 | 28 | | |
27 | 29 | | |
28 | 30 | | |
| |||
40 | 42 | | |
41 | 43 | | |
42 | 44 | | |
43 | | - | |
44 | | - | |
45 | | - | |
46 | | - | |
47 | | - | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
48 | 53 | | |
49 | 54 | | |
50 | 55 | | |
51 | 56 | | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
52 | 83 | | |
53 | 84 | | |
54 | 85 | | |
| |||
117 | 148 | | |
118 | 149 | | |
119 | 150 | | |
120 | | - | |
| 151 | + | |
121 | 152 | | |
122 | | - | |
123 | 153 | | |
124 | 154 | | |
125 | 155 | | |
126 | 156 | | |
127 | 157 | | |
128 | 158 | | |
129 | 159 | | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
130 | 178 | | |
131 | 179 | | |
132 | 180 | | |
133 | 181 | | |
134 | 182 | | |
135 | 183 | | |
136 | | - | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
137 | 202 | | |
138 | 203 | | |
139 | 204 | | |
| |||
192 | 257 | | |
193 | 258 | | |
194 | 259 | | |
195 | | - | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
196 | 264 | | |
197 | | - | |
198 | | - | |
| 265 | + | |
| 266 | + | |
199 | 267 | | |
200 | 268 | | |
201 | 269 | | |
202 | | - | |
| 270 | + | |
203 | 271 | | |
204 | 272 | | |
205 | 273 | | |
| |||
Lines changed: 22 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
45 | 45 | | |
46 | 46 | | |
47 | 47 | | |
| 48 | + | |
| 49 | + | |
48 | 50 | | |
49 | 51 | | |
50 | 52 | | |
| |||
62 | 64 | | |
63 | 65 | | |
64 | 66 | | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
65 | 87 | | |
66 | 88 | | |
67 | 89 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
8 | 9 | | |
9 | 10 | | |
10 | 11 | | |
11 | 12 | | |
12 | 13 | | |
| 14 | + | |
13 | 15 | | |
14 | 16 | | |
15 | 17 | | |
16 | 18 | | |
17 | 19 | | |
| 20 | + | |
18 | 21 | | |
19 | 22 | | |
20 | 23 | | |
| |||
26 | 29 | | |
27 | 30 | | |
28 | 31 | | |
| 32 | + | |
29 | 33 | | |
30 | 34 | | |
31 | 35 | | |
| |||
Lines changed: 13 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
| 14 | + | |
14 | 15 | | |
15 | 16 | | |
16 | 17 | | |
| |||
19 | 20 | | |
20 | 21 | | |
21 | 22 | | |
| 23 | + | |
22 | 24 | | |
23 | 25 | | |
24 | 26 | | |
| |||
28 | 30 | | |
29 | 31 | | |
30 | 32 | | |
| 33 | + | |
31 | 34 | | |
32 | 35 | | |
33 | 36 | | |
| |||
44 | 47 | | |
45 | 48 | | |
46 | 49 | | |
47 | | - | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
48 | 55 | | |
49 | 56 | | |
50 | 57 | | |
| |||
53 | 60 | | |
54 | 61 | | |
55 | 62 | | |
| 63 | + | |
56 | 64 | | |
57 | | - | |
| 65 | + | |
58 | 66 | | |
59 | 67 | | |
60 | 68 | | |
61 | | - | |
| 69 | + | |
| 70 | + | |
62 | 71 | | |
63 | 72 | | |
64 | | - | |
| 73 | + | |
65 | 74 | | |
66 | 75 | | |
67 | 76 | | |
| |||
Lines changed: 1 addition & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
| 23 | + | |
23 | 24 | | |
24 | 25 | | |
25 | 26 | | |
| |||
0 commit comments