direct: report why a migration failed in telemetry - #6355
Open
denik wants to merge 34 commits into
Open
Conversation
Collaborator
Integration test reportCommit: e1fcad5
Top 3 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/migrate-error
branch
from
August 25, 2026 09:25
ec7c5be to
5555ae5
Compare
denik
marked this pull request as ready for review
August 25, 2026 15:28
Contributor
Approval status: pending
|
pietern
reviewed
Aug 26, 2026
| @@ -0,0 +1,300 @@ | |||
| // Package safeerr provides errors that retain their message template: the | |||
| // format string, with values that came from the user left as verbs. Telemetry | |||
Contributor
There was a problem hiding this comment.
Verbs sounds active, more like "format specifier" or "placeholder".
Assuming you mean %s and friends here.
Contributor
Author
There was a problem hiding this comment.
"verbs" is heavily used on fmt package docs: https://pkg.go.dev/fmt
|
|
||
| type templateError struct { | ||
| // err is the fmt.Errorf result. It carries the interpolated message and | ||
| // the unwrap chain, so this type never reimplements either. |
| case safeValue: | ||
| // A value that declares a stand-in keeps it even here: the type | ||
| // knows which part of itself is user data, so it outranks a | ||
| // call-site assertion that the whole value is safe. |
Contributor
There was a problem hiding this comment.
I find this comment hard to parse.
|
|
||
| // expand walks the template, substituting safe values and chained templates and | ||
| // leaving every other verb in place. | ||
| func (e *templateError) expand(depth int) string { |
Contributor
There was a problem hiding this comment.
Why can't this process args first and then call fmt.Sprintf directly?
Contributor
Author
There was a problem hiding this comment.
I changed the approach to something similar - I prepprocess format string, escaping all format specifiers corresponding to unsafe values and the call Errorf on it + filtered args.
| // engine whose state could not be read or converted. Set alongside | ||
| // direct_migrate_error on opt-in deploys and direct_drymigrate_success on the | ||
| // dry run. | ||
| DirectMigrateSaferr string `json:"direct_migrate_saferr,omitempty"` |
Contributor
Author
There was a problem hiding this comment.
replaced with 'serror'
denik
force-pushed
the
denik/migrate-error
branch
from
August 26, 2026 09:47
58cbb04 to
9eaea8d
Compare
denik
changed the title
Deploy telemetry reports error_message, which is scrubbed by a path/PII regex deny-list and still treated as privileged, so it cannot be aggregated. Add an allow-list counterpart: an error keeps its format string, with everything the user supplied left as a verb, and that is what gets reported. libs/safeerr wraps fmt.Errorf and retains the template. Error() is byte-identical to what fmt.Errorf would have produced and the unwrap chain is unchanged, so converting a call site changes nothing for existing consumers. Templates chain through %w. Values are unsafe unless marked Safe; a value that is only partly user-authored supplies its own stand-in via SafeStringer, which is how config.ResourceKey reports resources.jobs.* without the resource name. Backend errors are unsafe: an API message echoes resource names, workspace paths and principals. SafeAPIErrorDescription reports only the structured fields, and requires error_code to look like an enum member so free text cannot ride along. The template reaches telemetry through diag.Diagnostic.ErrorTemplate, since logdiag keeps only the diagnostic and drops the error value. It lands in experimental.string_values, mirroring bool_values so later keys need no proto change. Converted the migrate path, which also records why an automatic migration to the direct engine failed rather than only that it did. The universe proto field is a follow-up; until it lands the field is dropped on ingestion. Co-authored-by: Isaac
…tests The inline `jq 'select(...)' out.requests.txt` pipelines duplicated what print_migration_telemetry and print_telemetry_string_values already print from the same recorded request, and .agents/rules/testing.md calls that pipeline out as an anti-pattern precisely because the helpers exist. Removing out.telemetry.txt also drops a synced file, hence the Files: count in the migrate golden. Co-authored-by: Isaac
…g map The string_values map was justified as mirroring bool_values so later keys would need no proto change, but adding the map is itself a proto change, so that cost is paid either way and the flexibility only covers keys that do not exist. Three known slots are better as named fields: a typo is a compile error, the struct documents what exists, and the backend gets real columns instead of a map to explode. Splitting the migration template in two also drops a join: the field names match the booleans recorded beside them, so a conversion failure is distinguishable from a commit failure without consulting bool_values. Co-authored-by: Isaac
Every resource key starts with it, so it carries no information in a template: "jobs.*" says what "resources.jobs.*" said. Co-authored-by: Isaac
The same two values were rendered in opposite orders by two functions in the same package: the user-facing summary reads "(403 INJECTED)" and the template read "INJECTED 403". Co-authored-by: Isaac
A filer error is a fixed classification plus the path it concerns: "access denied: /Workspace/...". The classification is a source literal and safe to report; the path is not. Naming the literals lets Error() and SafeString() derive from one string so they cannot drift, and leaves all 47 construction sites untouched. safeerr consults a wrapped error's stand-in under %w when it has no template of its own, and diag.ErrorTemplate consults it at the top of the chain, so this reports usefully from the call sites not yet raised through safeerr — which is most of them: before pushing direct state to workspace: %w [403 INJECTED] after pushing direct state to workspace: access denied [403 INJECTED] It matters most where the HTTP status cannot discriminate: a missing file and a missing parent directory are both 404. Co-authored-by: Isaac
Only the auto-migrate tests asserted what migration telemetry a deploy reports; the rest exercised migration scenarios while the telemetry went unobserved. Every migration test that records requests now prints it, so a change in what a scenario reports shows up as a golden diff. print_migration_telemetry collapses identical lines, since out.requests.txt can hold several deploys and repeating a line per deploy says nothing, and reports missing requests rather than letting jq complain about the file. Four tests are left out: default-python, profile_arg and var_arg set RecordRequests = false, and the two engine-config tests run only a failing "deployment migrate", which is not a deploy and emits no event. Also table-tests the conversion failures in build_state_test.go, which only covered success paths, pinning each message against its template. Co-authored-by: Isaac
…ploy Rather than collapsing repeated lines, each block is printed right after the deploy it belongs to and the recorded requests are cleared, so a block covers exactly one deploy. default-python, profile_arg and var_arg now record requests too, and remove them once the telemetry is printed, so nothing large lands in a golden. default-python needs withdir: its script cd's into the generated bundle while the requests are recorded at the test root. Three blocks come out empty on purpose. Those deploys run after the state is already direct, and MigrateToDirect is skipped entirely for a direct deploy, so the empty block is the assertion. Co-authored-by: Isaac
Deploy errors are a separate concern from migration, so error_template and the plumbing it needed are gone: diag.Diagnostic's field, logdiag's FirstErrorTemplate, Metrics.ErrorTemplate, the proto field and the deploy-error-template test. That also reverts the exhaustruct additions in load_dbalert_files.go. Only direct_migrate_error_template and direct_migrate_commit_error_template remain. Telemetry logging is reverted from the migration tests where it only showed direct_drymigrate_success true, which fifteen other tests already assert. Nothing covered the conversion failure class, only commit failures. The new auto-migrate-conversion-failure test uses an unknown secret scope permission level: a warning for the terraform deploy, but SecretScopeFixups rejects it while preparing the config for the direct engine. It asserts both populations — a failed dry run when not opted in, and a stopped migration when opted in. A conversion failure makes no API call, so unlike the commit failures it cannot be produced by injecting a fault. Co-authored-by: Isaac
The fixups ran through their mutator, so the failure arrived as a diagnostic and the migration could only report a generic "failed to apply secret scope fixups" — losing which fixup failed, and printing two warnings for one problem. ApplySecretScopeFixups is now the body, returning a real error and the offending scope key, with the mutator as a wrapper around it. The migration calls it directly, so its safeerr template survives: before direct_migrate_error_template failed to apply secret scope fixups after direct_migrate_error_template unknown permission level %q for secret scope Only the level itself is withheld; the rest of the message is a literal. Co-authored-by: Isaac
Its Ignore = [".databricks"] duplicated the parent's, and the comment above it described the databricks.yml permission level rather than the setting it sat on. That level is already explained where it appears, and the mechanism at the top of the script. Co-authored-by: Isaac
BuildStateFromTF wrapped node in config.ResourceKey at all twelve of its error sites. Retyping the loop variable would only trade those for seven string(node) casts at the places that want a plain string, so derive the key once instead. Co-authored-by: Isaac
One is a genuine failure: a terraform.tfstate whose format version the
migration does not understand. It is only reachable through "bundle deployment
migrate", since a deploy would have terraform reject the state first, so this
one records no telemetry.
The other three record that shapes which look like they should break do not:
reference-terraform-syntax ${databricks_job.src.id} deploys and migrates
cleanly; reverseInterpolate maps it back and
both resolution methods agree
reference-direct-only-resource an app env var referencing a catalog migrates
cleanly: apps.config is inline app.yaml rather
than an API field, so ExtractReferences drops
the reference before resolving it
reference-dabs-only-field a reference to a DABs-only field never reaches
the migration at all — Interpolate rewrites it
to a Terraform attribute that does not exist and
the deploy fails first
They are regression cover: each asserts the current outcome, so a change that
makes one start failing shows up here.
Co-authored-by: Isaac
A warning stops an automatic migration exactly as an error does, but carries no error to describe, so direct_migrate_warnings was a bare boolean with nothing saying which warning it was — for the outcome that is plausibly the most common blocker, since a single disagreeing field is enough. BuildStateFromTF now also returns a PII-free description of the first warning, built through safeerr from the parts that are safe: the resource type and the stage, never the resource name or the disagreeing values. It covers both warning sources, a resolution disagreement and a resource type with no adapter, and lands in direct_migrate_warning_template. Unit-tested only: neither warning is reachable from an acceptance test, since a disagreement needs a crafted terraform.tfstate and every terraform resource type currently has an adapter. Co-authored-by: Isaac
… tests The three metrics are now direct_migrate_saferr, direct_migrate_commit_saferr and direct_migrate_warning_saferr, naming the machinery that produces them rather than the shape of the value. tfstate-version-unsupported now exercises the automatic migration instead of the manual command. A post-deploy script bumps the state format version, which the deploy runs after applying and before the migration, so the deploy succeeds against the state it wrote and only the conversion fails. It reports direct_migrate_error, and now a saferr too: the version error was raised with fmt.Errorf, so nothing described it. reference-terraform-syntax now performs a real migration rather than a dry run, which exposed a defect: BuildStateFromTF derives depends_on from each reference via GetNodeAndType, which returns an empty node for a path that does not start with "resources". The empty node lands in resources.json, the migration reports success, and the next plan fails with `invalid dependency ... no such node ""`. The test records that rather than fixing it, since the fix is a behaviour change that does not belong in a telemetry PR. Also: reference-dabs-only-field asserted nothing. musterr does not extract a leading KEY=value the way trace does, so bash failed to find a command named DATABRICKS_BUNDLE_ENGINE=terraform, musterr accepted that as the expected failure, and the contains.py check never matched. Both tests also kept an out.requests.txt golden carrying per-run timings. Co-authored-by: Isaac
The defect was described in a script comment, which does not reach the golden. Using the repo's convention puts it in output.txt where a reader of the test output sees it, alongside a short note on the cause. Co-authored-by: Isaac
…inel reference-dabs-only-field only showed terraform rejecting the reference, so the conversion never ran. Both jobs now deploy with a literal and the reference is introduced afterwards, pointing a DABs-only field at another resource's DABs-only field. Neither is in terraform.tfstate, so both resolution methods fail and the conversion reports which field and which template: jobs.dst field tasks[0].new_cluster.autotermination_minutes: method A: jobs: "..." is a DABs-only field with no Terraform equivalent; method B: cannot look up "..." : jobs: "..." is a DABs-only field ... It goes through the migrate command, since a deploy carrying the reference is rejected by terraform before the conversion runs. tfstate-version-unsupported no longer gates its post-deploy script on a sentinel file. Commenting the script out in databricks.yml and enabling it with update_file.py is the idiom the other migrate tests already use, and leaves the script itself unconditional. Both tests were also keeping an out.requests.txt golden carrying per-run timings. Co-authored-by: Isaac
Using bundle.engine: direct rather than the migrate command puts this through the automatic migration, so it reports telemetry for both populations — a failed dry run before opting in, and direct_migrate_error after — each with the saferr: jobs.*: cannot resolve field %q (template %q): jobs.%s field %s: method A: %w; method B: cannot look up %q: %w It also corrects the reason the previous version went through the command: a deploy carrying this reference is not rejected by terraform. The field has no Terraform equivalent, so it is dropped on conversion along with the reference inside it, which is what leaves the conversion as the only thing that fails. No literal-then-swap is needed either; the reference can be there from the start. This is the reference-resolution failure class reaching telemetry end to end, which the unit table previously covered alone. Co-authored-by: Isaac
… error Reformatting only, except for one real gap: substitute's branch for a %w whose argument holds no error was untested. A nil error interface matches no case in templateArgs, so nothing is retained and the verb stays in the template. Errorf stays below 100% because of the vet printf marker, which is unreachable by construction. Co-authored-by: Isaac
safeerr's format scanner rejected an explicit argument index only ahead of a width or precision, so %2[2]s and %.2[2]s parsed '[' as the verb and the template desynchronized from fmt. The whole design rests on bailing out of anything the scanner does not model, and this was a hole in that. Following a template through %w was unbounded, so an error whose Unwrap reaches back to an ancestor recursed until the stack was gone. Bounded instead. An error retained for %w chaining now also contributes its stand-in under an ordinary verb, so a typed error reports its classification whether a call site writes %s or %w. ResourceKey.SafeString read the group straight out of the key without checking it, so a key of an unexpected shape would have put its second segment into a telemetry field. It now reports only a group this package defines. The secret scope fixups iterated a map while returning the offending key, which travels to a diagnostic and to telemetry, so two invalid scopes made the output depend on map order. Pre-existing, and the extraction did not change it, but sorting is what the repo's determinism rule asks for. Co-authored-by: Isaac
Nothing covered them. The strings BuildStateFromTF produces are unit-tested, but the fields carrying them to telemetry were not, so a renamed or duplicated json tag would silently stop populating a dashboard column rather than fail anything. This does not cover direct_migrate_warning_saferr end to end. Producing a conversion warning needs the two resolution methods to disagree, which needs a crafted terraform.tfstate: a real deploy stores the same value on both sides, and name_prefix does not help because it is applied before references resolve. Co-authored-by: Isaac
direct_migrate_warning_saferr had no coverage past the string BuildStateFromTF
returns. A post-deploy script edits dst's stored name in terraform.tfstate, so
Method A reads "source-drifted" from dst's own state while Method B evaluates
${resources.jobs.src.name} to "source". They disagree, the conversion warns, and
the migration stops without an error:
direct_migrate_warnings true
direct_migrate_warning_saferr jobs.%s field %q: method A and method B disagree
Editing the stored value is the point. A real deploy stores the same string on
both sides of a name-to-name reference, and name_prefix does not help because it
is applied before references resolve, so config alone cannot produce this.
Co-authored-by: Isaac
"requested in" became "selected via" and "opted in via" became "selected via". The rebase also dropped direct_migrate_commit_saferr from two goldens again, the same way it did last time: main carries those files without the field, so the auto-merge takes its line set. Co-authored-by: Isaac
The new error and warning-path tests open a write-mode DeploymentState but never reach the Finalize the success helper uses to close the resources.json.wal handle. On Windows an open handle blocks t.TempDir() cleanup, failing the test. Add openEmptyWriteState, which registers a t.Cleanup that finalizes the state (closing the WAL handle and removing the file), and use it from both sites. Co-authored-by: Isaac <no-reply@databricks.com>
SafeSprintf renders the format with only its safe arguments substituted, per the
TODO: walk the format, keep a verb that has a safe argument, and escape the
percent of every other one so it appears literally and consumes nothing. On a
construct the scanner does not model, return the format plus "(safeerrerr)" —
which argument each verb consumes is then unknown, and substituting any of them
could pair a safe value with the wrong verb.
Three things the rework left dangling:
- New assigned an error to the string field, so the package did not build.
- Errorf called SafeSprintf while the stub was named SafeErrorf. Settled on
SafeSprintf: it returns a string, and that is the name at the call site.
- parseVerb had been removed but is still the scanner primitive, so it is
restored, keeping its table test.
maxSafeErrorSize now caps the result, since a safe message is a telemetry field.
Two behaviours changed, deliberately. A safe message is a message rather than a
format string, so %% renders as a percent. And an error argument contributes its
own safe message through a proxy, so nesting composes at construction rather
than by recursing at read time — which retires the depth bound and the cyclic
chain concern entirely.
Renamed across the codebase: safeerr.ErrorTemplate and diag.ErrorTemplate are
now SafeError, with the tests and their file names following.
Co-authored-by: Isaac
Both asserted the same shape. New never formats, so the message and the safe message always equal the input, which leaves the table needing only the text. Adds a case for an escaped percent, which neither test covered. Co-authored-by: Isaac
The table already described every input and both expected representations, so the same rows now also exercise SafeSprintf, which was only reachable through Errorf, and assert that Error() renders what fmt.Errorf would. Three chain tests were format-and-args rows in disguise and became table rows; their unwrapping assertions moved to TestErrorsIs, which already owns that, and what the table cannot express — that every level keeps its own safe message — stays as one test. Co-authored-by: Isaac
testifylint wants the expected value first. I pushed the previous commit without noticing lint had failed. Co-authored-by: Isaac
Its property — that Error() renders what fmt.Errorf would — is now asserted for every row by the MatchesFmt loop, so the separate table only carried its seven inputs. Those become rows, with both representations pinned rather than compared against a value computed in the test. Two behaviours the old test did not state, now recorded: an extra safe argument is dropped from the safe message rather than reported as fmt's EXTRA marker, and a safe value rendered through the wrong verb keeps fmt's marker, where an unsafe one would have had its verb escaped. Co-authored-by: Isaac
Twelve tests were the table's shape or already covered by it, so 25 test functions become 13 while assertions go up from 114 to 152. Redundant outright: the several-wrapped-errors chain test, whose rendering became a row and whose unwrapping moved to TestErrorsIs; the safe-args test, covered by the mixed-args row; and the stability test, which guarded expand() mutating state that the construction-time design removed. Folded in as rows: the SafeStringer table and its five satellites, and the nil-wrap case. The leak property is now a loop over every row against a list of the user-data-shaped strings the fixtures use, which is stronger than the individual NotContains assertions it replaces: a new row is covered without anyone remembering to assert it. TestSafeErrorFallsBackToRawFormat stays as it is. Its loop derives the expectation from the format, so folding it would trade that for four hardcoded strings. Co-authored-by: Isaac
…eason 20 was arbitrary. The keys are sorted now, so one call is enough for current behaviour; the repetition only buys regression detection, because dropping the sort would make a single call pick "apple" half the time and pass. Ten rounds leaves a one-in-a-thousand miss, which the comment now states. Co-authored-by: Isaac
Sorted keys make one call deterministic, and dropping the sort would fail it about half the time, which is enough to notice. Co-authored-by: Isaac
acceptance/bundle/migrate/test.toml pins EnvMatrix.DMS on main; the harness materializes it into out.test.toml, which these six directories predate. Co-authored-by: Isaac
denik
force-pushed
the
denik/migrate-error
branch
from
September 1, 2026 10:49
e1fcad5 to
e3c4703
Compare
pietern
reviewed
Sep 2, 2026
| // | ||
| // The format string is considered safe. The arguments to format strings are unsafe by default | ||
| // unless wrapped with safeerr.Safe() or implement SafeStringer() interface. | ||
| package safeerr |
Contributor
There was a problem hiding this comment.
Mix of "safeerr" and "saferr", let's pick one and use it everywhere.
| // argument each verb consumes is no longer known and substituting any of | ||
| // them could pair a safe value with the wrong verb. The format is a | ||
| // source literal, so reporting it unrendered is still safe. | ||
| return truncateSafe(format + "(safeerrerr)") |
| // without scrubbing: the format string is a source literal, and nothing else | ||
| // reaches the output. | ||
| func SafeSprintf(format string, args ...any) string { | ||
| safeFormat, safe, ok := escapeUnsafeVerbs(format, safeArgs(args)) |
Contributor
There was a problem hiding this comment.
Is this safeFormat and safeArgs?
| // escapeUnsafeVerbs rewrites format so that only verbs with a safe argument | ||
| // still consume one, and returns those arguments in order. It reports false for | ||
| // a construct parseVerb does not model. | ||
| func escapeUnsafeVerbs(format string, safe []any) (string, []any, bool) { |
Contributor
There was a problem hiding this comment.
The incoming args are unsafe, so just args?
| if argIndex >= len(safe) { | ||
| // More verbs than arguments; go vet reports the call itself. | ||
| return nil, false | ||
| } |
Contributor
There was a problem hiding this comment.
This check belongs in the calling function, then this function can take just (arg any).
It shouldn't be called safe because we don't know if it is safe yet.
| out[i] = safeValue{v: ss.SafeString()} | ||
| } else { | ||
| out[i] = v | ||
| } |
Contributor
There was a problem hiding this comment.
This logic is different from the logic in safeValueFor.
I think they should be shared and this function can call safeValueFor in a loop on its args.
Alternatively, we don't call safeArgs before passing into escapeUnsafeVerbs and let its loop make the verb arguments safe.
| // which is most of them. | ||
| if ss, ok := err.(safeerr.SafeStringer); ok { | ||
| safe = ss.SafeString() | ||
| } |
Contributor
There was a problem hiding this comment.
This can be moved to the SafeError function. Looks like a fallback that should always apply.
| // and config values back to the caller — so it never appears here. What is left | ||
| // is still the most useful part for aggregation: which platform error the | ||
| // request failed with. | ||
| func SafeAPIErrorDescription(e error) string { |
Contributor
There was a problem hiding this comment.
Mixed use of e and err for error args.
| BundleMutatorExecutionTimeMs: getExecutionTimes(b), | ||
|
|
||
| // Own alignment group: these names are long enough that sharing one | ||
| // would re-align every field above them. |
| func setWarnSaferr(target *string, err error) { | ||
| if *target == "" { | ||
| *target = safeerr.SafeError(err) | ||
| } |
Contributor
There was a problem hiding this comment.
Can't we concat safe errors? E.g. an https://pkg.go.dev/errors#Join for safe errors.
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
3 participants
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.
Changes
Tests
An acceptance test per failure class: conversion, state parse, commit, and the warning-only case that stops a migration without an error.