Fix model serving telemetry_config drift and dropped updates - #6106
Draft
radakam wants to merge 12 commits into
Draft
Fix model serving telemetry_config drift and dropped updates#6106radakam wants to merge 12 commits into
radakam wants to merge 12 commits into
Conversation
…ping updates telemetry_config is only carried by CreateServingEndpoint. The serving endpoints GET API never echoes it back, and no update API accepts it, so classify it as input_only for remote changes and no_update_api for local ones.
radakam
marked this pull request as ready for review
July 30, 2026 13:18
Contributor
Approval status: pending
|
radakam
marked this pull request as draft
July 30, 2026 13:41
Collaborator
Integration test reportCommit: 155748b
10 interesting tests: 4 RECOVERED, 4 SKIP, 2 flaky
Top 30 slowest tests (at least 2 minutes):
|
The previous approach classified telemetry_config as input_only/no_update_api on the premise that GET never returns it and no API updates it. Neither holds: ServingEndpointDetailed carries the field, and ServingEndpoints.PatchTelemetryConfig updates it. Suppressing the whole subtree converged only by making genuine sampling_fraction edits invisible and silently dropping them. The actual remote-only difference is telemetry_profile_id, which the backend assigns and echoes back, so suppress just that as a backend default and wire the PATCH API into DoUpdate. Edits now converge and are applied. The testserver fake dropped telemetry_config on create, which is why the earlier suppression looked correct locally; it now echoes the field and populates the fields the backend assigns.
radakam
changed the title
The backend only supports telemetry configuration on endpoints with custom served models. The invariant suite deploys a config-less endpoint, so the create silently dropped telemetry_config and every subsequent plan reported an update that the telemetry-config API then rejected with "Telemetry configuration is not supported for endpoint type 'NO_CONFIG'". Serving a custom model needs a registered model version and real serving compute, which is too slow for this suite, and the drift/telemetry_config test already asserts the same convergence locally. The terraform truncation note moves to that test, which is where the engine matrix is now restricted.
The backend default was declared at telemetry_config.telemetry_profile_id, but when telemetry_config is absent from the bundle, structdiff reports the populated remote as a single change on the parent telemetry_config path. Neither the rule prefix nor the map fallback in shouldSkipBackendDefault matches a struct at that path, so the change was planned as an update and DoUpdate sent a PATCH with no telemetry_config, deleting telemetry from an endpoint the bundle never configured. Adopting an endpoint that already has telemetry reaches this. Declaring the rule on the whole subtree covers the parent path. A genuine removal is unaffected because it carries a non-nil old value, which shouldSkipBackendDefault requires to be nil. The new telemetry_config_unmanaged test covers it. With the rule back at the child path it fails with the endpoint's telemetry_config reading back as null and a second PATCH carrying an empty body.
Leftover from adding and then removing the telemetry_config invariant config.
The telemetry_config_unmanaged test reads the endpoint back with `api get` and a leading-slash path, which MSYS rewrites to C:\Program Files\Git\api\... before the CLI sees it, so the request missed the testserver stub and the test failed on Windows only. The out-of-band `api patch` above it already guarded against this. Setting MSYS_NO_PATHCONV for the whole script via test.toml would instead break the print_requests.py invocation on the last line, the same reason script.prepare unsets it around its Python helpers.
Nothing exercised the removal path, which is the least certain part of this change: dropping telemetry_config from the bundle makes DoUpdate PATCH with the field omitted, and whether that deletes the configuration or is a no-op is the backend's call. The drift test now removes it, asserts the endpoint reads back null, and records the empty PATCH body in the golden file so the assumption is visible rather than buried in the fake. The fake also minted a fresh telemetry_profile_id on every PATCH instead of keeping the endpoint's profile. UUID masking hides that in the output, so it could only ever mislead a future test; the patch now carries the existing ID forward. The changelog gained the user-visible regression: an endpoint the backend types as NO_CONFIG that declares telemetry_config used to deploy because the field was dropped, and now fails on every deploy.
Wiring the telemetry-config API into DoUpdate turned a silent no-op into a hard failure for endpoints the backend types as NO_CONFIG. Create drops telemetry_config on such an endpoint instead of failing, so the bundle deployed once and then failed on every later deploy, when the plan applied the field the create had thrown away. The field can never be honored there, so keep failing, but fail at validation time naming the field and the fix instead of surfacing a 400 from the second deploy on.
The classification here had been rewritten twice on differing assumptions about
what the telemetry API accepts and returns, with the testserver fake reshaped to
match each one, so the acceptance tests could not tell whether any of them held.
Checked against a workspace instead:
- A telemetry_config naming neither table_names nor telemetry_profile_id is
discarded; create and the telemetry API both report success and apply nothing.
- GET echoes telemetry_profile_id, and inference_table_config once the config
was accepted. table_names is never echoed.
- Telemetry is rejected on endpoints typed NO_CONFIG and EXTERNAL_MODELS, and
create rejects a foundation model outright.
- An empty PATCH body removes the configuration.
- The telemetry API refuses to run while the endpoint is still applying an
earlier update, and every update call starts one.
So table_names is input_only but inference_table_config is not: sampling_fraction
round trips and needs ordinary drift detection. Suppressing the subtree would
silently swallow the edits users actually make.
DoUpdate now waits for the endpoint to settle before patching telemetry when an
earlier call in the same pass ran. WaitAfterUpdate only runs once DoUpdate has
returned, so without this a deploy touching both config and telemetry_config
fails on the telemetry call.
Validation covers both forms the API discards, and the fake models the
endpoint-type rejection so a fixture the backend would refuse cannot pass.
The drift fixture moves off an external model, which the API rejects, onto a
custom served model and gains a sampling_fraction edit. Both drift tests stay
local: telemetry needs a registered UC model and roughly ten minutes to
provision, and each PATCH locks the endpoint for another two to three.
They had accumulated the reasoning behind each revision, which belongs in the commit messages. Each one now states the API behaviour the code depends on and stops there.
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
2 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
Classify
telemetry_configas a backend default and apply changes to it throughServingEndpoints.PatchTelemetryConfig.Why
The backend assigns a
telemetry_profile_idand echoes it back on GET. Nothing suppressed it, so everybundle planafter a deploy showed a perpetual update. Separately, editingtelemetry_configplanned an update thatDoUpdatehad no branch for, so the deploy silently did nothing.The rule covers the whole subtree, not just
telemetry_profile_id: with no localtelemetry_config, the remote-only value arrives as a single change on the parent path, which a child rule doesn't match. That planned an update and sent a PATCH with notelemetry_config, deleting telemetry from an endpoint the bundle never configured. Genuine removals still apply, since they carry a non-nil old value.Behavior change
The backend rejects telemetry on endpoints without custom served models. Such an endpoint with
telemetry_configin the bundle used to deploy fine, because the backend dropped the field; now every deploy fails withTelemetry configuration is not supported for endpoint type 'NO_CONFIG'. Erroring beats silently ignoring the field, but it surfaces as a raw 400 rather than a plan-time validation. This is also whytelemetry_configleft the invariant configs.Tests
drift/telemetry_configcovers convergence and that asampling_fractionedit is applied.drift/telemetry_config_unmanagedcovers out-of-band telemetry on an endpoint that doesn't declare it; with the rule at the child path it fails, withtelemetry_configreading back asnull. Direct engine only: the terraform provider typessampling_fractionas an integer and truncates0.5to0.