Add skip_record_validation_percents_by_type indexer config for sampled backfill validation - #1315
Conversation
…validation Adds an indexer config option that skips per-record JSON schema validation for a configurable fraction of records, keyed by GraphQL type. It exists for backfills of trusted, pre-validated data, where the per-record schema walk is a meaningful ingest cost and the datastore mappings provide a coarse backstop. `skip_record_validation_for` maps a type name to a fraction in `[0.0, 1.0]`: `0.0` (or an absent key) validates every record, `1.0` skips every record, and values in between sample. The skip decision is deterministic per event id -- a stable `Zlib.crc32` of `EventID#to_s` buckets each event -- so the same event makes the same decision on retry across indexer pods (`String#hash` is unsuitable: `RUBY_HASH_SEED` is per-process). The event envelope is always validated regardless of the sampling rate. Two supporting pieces: - `RecordPreparer::UnknownTypeError` (a `KeyError` subtype) is raised when a skipped record reaches the preparer with a missing/unknown abstract-type `__typename`. `Factory#build` rescues it and returns a structured `FailedEventError` rather than letting an exception escape, with a message that omits the offending value. - Skipping a safety check is never silent: `Processor` tallies skipped records per batch and logs a single aggregate `RecordValidationSkipped` entry (with per-type counts), mirroring the batch-level `ElasticGraphIndexingLatencies` log. Per-record logging would be untenable at backfill scale.
marcdaniels-toast
left a comment
There was a problem hiding this comment.
@vermatron I'm digging into this PR. Meanwhile can you merge the latest origin/main into it?
Addresses review feedback on block#1315: the `skip_record_validation_for` description implied that a malformed record whose validation was skipped would always fail in isolation. Only a missing or unknown `__typename` on an abstract-type field actually does - that is the one error `Operation::Factory#build` rescues. Other malformations that per-record validation would have caught escape as unhandled exceptions and take their whole batch down. They surface at two layers: - during `Factory#build`, e.g. a value `IndexingPreparers::Integer` cannot coerce, or a missing `id_source` path - during `router.bulk`, from `Update#metadata` - the rollover index suffix and the custom routing key. `to_datastore_bulk` is memoized and lazy, so these are not reachable from `Factory#build`'s rescue at all. Because an exception produces no `batchItemFailures` response, SQS redelivers the entire batch and the malformed record re-poisons it on every retry until `maxReceiveCount` drains it to the DLQ, dragging the well-formed events along each time. The description now says so. Regenerated `config_schema.yaml` via `script/update_config_artifacts`. Actually isolating these failures is a larger change (it needs a seam that covers both layers, plus typed errors so a schema-artifact `KeyError` is not demoted to a per-event data failure) and is left to a follow-up.
…idation-for-config
…onfig' into add-skip-record-validation-for-config
Move the warning about skipped validation failing a whole batch to the front of the setting's description, and drop the duplicate copy that had accumulated at the end. Keeps the unique details (the document id/routing key/rollover suffix failure mode and the dead letter queue consequence) and regenerates config_schema.yaml. Generated with Claude Code
|
This looks good to me. I'll ask @myronmarston to take a look in case there are bigger picture things I didn't think of or catch. |
myronmarston
left a comment
There was a problem hiding this comment.
Nice work @vermatron! I left some suggestions.
`build_failed_result` builds the operations it attaches to a `FailedEventError` so callers can see what would have been indexed. That build can itself raise: `Widget` requires `cost`, and its derived `WidgetCurrency` update target sources its id from `cost.currency`, so a `Widget` with no `cost` fails validation, reaches `build_failed_result`, and then dies with `KeyError: key not found: ["cost"]`. The whole batch dies with it, and the malformation that was about to be reported is never reported at all. Fall back to no operations instead, and log a `FailedEventOperationBuildingFailure` warning so the discarded error stays traceable. `FailedEventError#operations` is already documented as sometimes being empty for exactly this reason, so nothing downstream is surprised. This lands ahead of the record-validation sampling work because an exception raised inside a `rescue` body is not caught by that same `rescue`: the re-validation path added next routes unvalidated records through `build_failed_result` from within a rescue, and needs it not to raise. Generated with Claude Code
Responds to the eight inline comments on block#1315. Naming. `skip_record_validation_for` did not say what its values were, and it read like a list of types when it is a map of percentages. It is now `skip_record_validation_percents_by_type`, and `BuildResult`'s `validation_skipped_for` is now `type_with_skipped_validation`, which names what it holds. Units. The values were fractions in `[0.0, 1.0]`, which nobody writes in a config file. They are now percents in `[0, 100]`. The skip decision compares `Zlib.crc32` of the event id against that percentage of the CRC32 space, with a `CRC32_SPACE_PER_PERCENT` constant so the per-event cost stays one multiply. The `<= 0` and `>= 100` guards keep both endpoints exact, so no float boundary error can make a `0` percent skip a record or a `100` percent validate one. Isolation via re-validation instead of an error taxonomy. Rescuing only `RecordPreparer::UnknownTypeError` handled the one failure mode we had traced, which is not a principle. With validation skipped there is no bounded list of ways malformed data can blow up while operations are built, so we now rescue anything and re-run the validation we skipped. If the validator faults the record, the caller gets a `FailedEventError` carrying the validator's own message, PII-sanitized, exactly what it would have gotten on the validated path, and the rest of the batch still indexes. If the validator is happy, the error was never about the data, so it is re-raised untouched rather than disguised as a data failure. `UnknownTypeError` is therefore deleted rather than moved to `errors.rb`: `Inventor` requires `__typename` and pins a `const` discriminator per concrete subtype, so both the missing and the unknown case already fail validation. The spec that covered it stays, now asserting the validator's message, since it is what proves the deletion is safe. `::Kernel.raise` is used for the re-raise because `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 bypassed deliberately, with a comment at the site and a spec that asserts the original class and message rather than merely that something raised. Documentation. The `description:` now leads with what the setting does and the `0`/`100` semantics, then the indexing-CPU tradeoff and the canary rationale, then the caveat, narrowed to say which malformations are isolated and which still fail the whole batch, with the redelivery-to-DLQ consequence spelled out rather than implying a broader guarantee. Generated with Claude Code
myronmarston
left a comment
There was a problem hiding this comment.
LGTM! Before merging, can you update the PR title and description to reflect where we landed? Currently it refers to skip_record_validation_for which is not the final API. I'll merge this with a squash-and-merge which will use the PR title/description for the commit message so it would be nice if it was accurate.
skip_record_validation_for indexer config for sampled backfill validationskip_record_validation_percents_by_type indexer config for sampled backfill validation
Hi @myronmarston , I have updated the PR description. Thanks for your help with the changes. |
Add
skip_record_validation_percents_by_typeindexer config for sampled backfill validationWhy
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_updatesbackfill knob and follows the same shape.Design notes:
skip_record_validation_percents_by_typemaps a type name to a percent in[0, 100].0(or an absent key) validates everything,100skips everything, and values in between sample. The value is the percentage skipped, so90skips 90% and validates 10%.The skip decision compares a
Zlib.crc32of 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#hashwon't do here: its seed is per-process, so two pods would disagree. The<= 0and>= 100guards keep both endpoints exact, so no float boundary error can make a0skip a record or a100validate one.The event envelope is always validated. Only the per-record schema walk gets sampled.
Skipping isn't silent.
Processorcounts skipped records per batch and logs oneRecordValidationSkippedline with per-type counts, the same way it logsElasticGraphIndexingLatencies. Logging per record would drown a100percent 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#buildrescues anything and then runs the validation it skipped. If the validator faults the record, the caller gets aFailedEventErrorcarrying 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__typenamecase:Inventorhasrequired: ["__typename"]plus aoneOfwhose branches each pin__typenamewith aconst, so a missing one failsrequiredand an unknown one fails every branch.::Kernel.raiseis used for the re-raise becauseOperation::Factoryoverridesraiseto stop the class originating errors instead of returning aBuildResult. 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 plainraise exceptionwould substitute the guard's own error and still satisfy a bareraise_error.Fixed on the way:
build_failed_resultcould itself raise while building the operations it attaches to aFailedEventError, masking the malformation it was trying to report. An exception in arescuebody isn't caught by that samerescue, 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:Widgetrequirescost, so aWidgetwithout it fails validation, reachesbuild_failed_result, and dies withKeyErrorbuilding the derivedWidgetCurrencytarget, whose id comes fromcost.currency. The batch dies with it and the malformation is never reported. It now falls back to no operations and logsFailedEventOperationBuildingFailure;FailedEventError#operationsis 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_bulkis lazy and memoized, so the rollover index suffix and custom routing value computed inUpdate#metadataare evaluated insiderouter.bulk, afterbuildhas 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:
config.rb: newskip_record_validation_percents_by_typeJSON schema property (object, per-type number in[0, 100],additionalProperties: false, default{});convert_valuescoerces percents toFloat. Thedescription:leads with what the setting does, then the indexing-CPU tradeoff, then what remains unisolated.operation/factory.rb: newskip_validation?(type, event)and theCRC32_SPACE_PER_PERCENTconstant;buildbranches on the skip decision intobuild_success_resultorbuild_success_result_isolating_malformed_records;build_failed_resultno longer lets a second failure mask the first.BuildResultgainstype_with_skipped_validation.processor.rb: aggregateRecordValidationSkippedlog per batch when any record was skipped.indexer.rb: wireconfig.skip_record_validation_percents_by_typeinto the factory.record_preparer.rb: unchanged. It carried aRecordPreparer::UnknownTypeErrorin an earlier revision of this PR; that's gone, and the file no longer appears in the diff.elasticgraph-localconfig_schema.yaml: regenerated viascript/update_config_artifacts.Verification
script/run_specs(COVERAGE=1, real Elasticsearch): 5307 examples, 0 failures.elasticgraph-indexeron 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.New tests:
config_spec.rb: integer YAML percents coerce toFloat(90to90.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 (stubbedZlib.crc32for both branches); retry stability; the derived-index path under skip; a non-coercibleamount_centsand an unknown abstract-type__typenameeach reported as aFailedEventErrorcarrying 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 oneRecordValidationSkippedwith the rightcount/counts_by_type; a batch with no skips logs none.