Skip to content

change: emit the JumpStart model ID in ModelBuilder telemetry - #6234

Open
evakravi wants to merge 3 commits into
aws:masterfrom
evakravi:change/jumpstart-telemetry-flag
Open

change: emit the JumpStart model ID in ModelBuilder telemetry#6234
evakravi wants to merge 3 commits into
aws:masterfrom
evakravi:change/jumpstart-telemetry-flag

Conversation

@evakravi

@evakravi evakravi commented Sep 1, 2026

Copy link
Copy Markdown
Member

Problem

ModelBuilder telemetry for build() and deploy() cannot identify JumpStart usage. The telemetry URL carries no JumpStart field. Analytics cannot separate JumpStart traffic or rank JumpStart models by build count and deployment count.

Solution

Add a _jumpstart_model_id helper to the build() and deploy() telemetry param lists. The helper returns the JumpStart model ID string for a JumpStart source, and returns None for another source. The telemetry extractor omits ATTR_CALL params when a helper returns None.

JumpStart events thus carry &x-jumpstartModelId=<model-id>, and other events carry no JumpStart param. The presence of the param identifies JumpStart traffic, and its value names the model, so one field answers the two questions.

Tests

The focused command reports 36 passed. The tests cover the model ID param for build() and deploy(), and confirm the omission of the param for another model source. The tests also confirm the absence of a separate boolean param. The same command reports 3 failed, 38 passed before the model ID change.

The regression command reports 100 passed, 2 skipped. The 2 skips are existing @unittest.skip marks on TestConstructUrl.

This contribution uses the repository Apache 2.0 license.

Record whether ModelBuilder build and deploy calls use a JumpStart model ID. This flag lets downstream analytics identify JumpStart usage.

---
X-AI-Prompt: Can you add a JumpStart identity flag to ModelBuilder telemetry and raise a pull request?
X-AI-Tool: claude-code
@evakravi
evakravi deployed to auto-approve September 1, 2026 19:10 — with GitHub Actions Active
@evakravi
evakravi deployed to auto-approve September 1, 2026 19:10 — with GitHub Actions Active
@evakravi
evakravi deployed to auto-approve September 1, 2026 19:11 — with GitHub Actions Active
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 1m 8s —— View job


PR Review — JumpStart telemetry flag

  • Read PR diff
  • Verify _is_jumpstart_model_id helper & telemetry decorator behavior
  • Check param ordering / naming vs existing patterns
  • Review tests

Summary

The change looks clean and correct. It adds ("_is_jumpstart_model_id", TelemetryParamType.ATTR_CALL) to the telemetry_params of both build() and deploy(), mirroring the existing _is_nova_model_for_telemetry / _is_model_customization entries. No production behavior beyond telemetry is affected, and there are no public-API or backward-compatibility concerns.

Correctness — verified against the source:

  • The helper _is_jumpstart_model_id (sagemaker-serve/src/sagemaker/serve/model_builder_utils.py:2907) returns a bool and memoizes via _cached_is_jumpstart, so it's safe and cheap to invoke a second time during telemetry extraction.
  • ATTR_CALL handling (telemetry_logging.py:245-251) wraps the call in try/except, so a failure in the helper can't break build()/deploy().
  • Param name mapping is correct: to_lower_camel_case("is_jumpstart_model_id")isJumpstartModelId, emitted as &x-isJumpstartModelId=<True|False>, matching the tests.
  • The ordering assertion is valid: _extract_telemetry_params is appended (telemetry_logging.py:381) before &x-latency= (line 389), so the flag reliably precedes latency.
  • Because it's ATTR_CALL (not ATTR_VALUE), the flag is always emitted — including the False case — which is the intended analytics behavior.

Tests: Good coverage of both True/False for build() and deploy() plus the ordering guarantee. Scope is appropriate — the helper itself is exercised elsewhere, so mocking it here to test the wiring is the right call.

Minor (non-blocking) observations — no change required:

  • The tests mock _is_jumpstart_model_id directly, so they validate the telemetry wiring rather than JumpStart detection itself. That's the correct scope for this PR; just noting the real helper's behavior isn't re-verified here.
  • Consider mirroring one assertion for the build() path's before-x-latency ordering too, for parity with the deploy() test — optional.

No security or performance concerns. LGTM. ✅

The isJumpstartModelId flag separates JumpStart traffic from other traffic,
but it does not name the model. Analytics cannot rank JumpStart models by
build count or deployment count from a boolean.

Add a jumpstartModelId param to the build and deploy telemetry param lists.
The param emits the model ID string when the model source is a JumpStart
model ID, and emits nothing for another model source. The ATTR_CALL branch
of the param extractor drops a None return, so a param that has no value
stays out of the beacon.

---
X-AI-Prompt: Can we track the JumpStart model ID itself in telemetry instead of only a boolean flag?
X-AI-Tool: claude-code
@evakravi evakravi changed the title change: emit a JumpStart flag in ModelBuilder telemetry change: emit JumpStart identity in ModelBuilder telemetry Sep 1, 2026
@evakravi
evakravi deployed to auto-approve September 1, 2026 21:05 — with GitHub Actions Active
@evakravi
evakravi deployed to auto-approve September 1, 2026 21:06 — with GitHub Actions Active
@evakravi
evakravi deployed to auto-approve September 1, 2026 21:07 — with GitHub Actions Active
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 1m 7s —— View job


PR Review — JumpStart telemetry flag

  • Read PR diff
  • Review telemetry_logging.py None-handling change
  • Review _jumpstart_model_id / _is_jumpstart_model_id helpers
  • Review model_builder.py telemetry_params wiring
  • Review tests

Summary

The change is clean, correct, and well-tested. Beyond the flag described in the PR body, it now also emits the actual JumpStart model ID (&x-jumpstartModelId=<id>) and makes ATTR_CALL skip emission when the helper returns None — which is what lets _jumpstart_model_id() emit the ID only for JumpStart models. Verified against the source:

Correctness ✅

  • ATTR_CALL None-skip is backward-safe. All pre-existing ATTR_CALL params (_is_fine_tuned, _is_model_customization, _is_nova_model_for_telemetry) return bool, never None, so none of them change behavior. Previously a None return would have emitted the literal string &x-...=None; skipping it is strictly better. The docstring update at telemetry_logging.py:195 documents this accurately.
  • Both flags emit as intended. _is_jumpstart_model_id() returns a bool, so value is not None is truthy for both True and False&x-isJumpstartModelId= is always emitted. _jumpstart_model_id() returns None for non-JumpStart models → &x-jumpstartModelId= is emitted only for JumpStart. This matches the tests.
  • Key naming is correct. to_lower_camel_case("is_jumpstart_model_id")isJumpstartModelId; to_lower_camel_case("jumpstart_model_id")jumpstartModelId.
  • _jumpstart_model_id is safe. The isinstance(self.model, str) guard means it never passes a non-string to the retrieve path, and it reuses the memoized _cached_is_jumpstart, so it's cheap on the second call during telemetry extraction.
  • Not customer data. The emitted ID only comes from a value that _is_jumpstart_model_id() validated via model_uris.retrieve, i.e. a public JumpStart catalog ID — not an arbitrary customer string.
  • Ordering holds. _extract_telemetry_params is appended (telemetry_logging.py:384) before &x-latency= (line 392), so the flag reliably precedes latency, as the test asserts.

Tests ✅ — Good coverage: True/False for both build() and deploy(), model-ID present/absent for both, the before-x-latency ordering, and the new test_attr_call_skips_none unit test for the decorator change. Mocking _is_jumpstart_model_id is the right scope here (the helper's own detection is exercised elsewhere).

Minor (non-blocking) observations

  • &x-jumpstartModelId=<value> is inserted into the URL unencoded (unlike created_by, which uses quote(...)). JumpStart catalog IDs are constrained to URL-safe characters, so this is fine in practice — just noting the asymmetry.
  • isJumpstartModelId and jumpstartModelId are slightly redundant (the ID implies the flag), but that's a reasonable analytics choice and harmless.
  • The PR body describes 5 tests / only the flag; the diff now has 10 tests plus the model-ID emission and the ATTR_CALL None-skip. Worth updating the description for reviewers, but no code concern.

No security or performance concerns. LGTM. ✅

The model ID identifies JumpStart traffic and the exact model. The boolean
param duplicates this information and adds a second field for consumers.
Remove the boolean param from ModelBuilder telemetry and keep only
jumpstartModelId.

---
X-AI-Prompt: Can you remove the boolean flag and keep only the JumpStart model ID telemetry param?
X-AI-Tool: claude-code
@evakravi evakravi changed the title change: emit JumpStart identity in ModelBuilder telemetry change: emit the JumpStart model ID in ModelBuilder telemetry Sep 1, 2026
@evakravi
evakravi deployed to auto-approve September 1, 2026 22:03 — with GitHub Actions Active
@evakravi
evakravi deployed to auto-approve September 1, 2026 22:03 — with GitHub Actions Active
@evakravi
evakravi deployed to auto-approve September 1, 2026 22:03 — with GitHub Actions Active
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 1m 45s —— View job


PR Review — Emit JumpStart identity in ModelBuilder telemetry

  • Read PR diff (/tmp/pr.diff)
  • Verify _jumpstart_model_id / _is_jumpstart_model_id helpers against source
  • Verify ATTR_CALL None-skip in telemetry_logging.py
  • Check model_builder.py telemetry_params wiring
  • Review tests + lint/format config

Summary

The change is correct and well-tested. It adds a _jumpstart_model_id helper that returns the model ID for a JumpStart source (and None otherwise), wires it as an ATTR_CALL param into both build() and deploy(), and updates the ATTR_CALL branch to skip emission when a helper returns None. Net effect: &x-jumpstartModelId=<id> is emitted only for JumpStart models. Verified against the checked-out source:

  • ATTR_CALL None-skip is backward-safe. All pre-existing ATTR_CALL helpers (_is_fine_tuned, _is_model_customization, _is_nova_model_for_telemetry) return bool, never None, so their behavior is unchanged (telemetry_logging.py:246-254). The try/except around method() keeps a helper failure from breaking build()/deploy(), and the docstring at :195 documents the new None behavior.
  • _jumpstart_model_id is safe. The isinstance(self.model, str) guard prevents passing a non-string to the retrieve path, and it reuses the memoized _cached_is_jumpstart from _is_jumpstart_model_id, so the second call during telemetry extraction is cheap (model_builder_utils.py:2907-2931).
  • Key naming is correct. to_lower_camel_case("jumpstart_model_id")jumpstartModelId&x-jumpstartModelId=, matching the tests.
  • Not customer data. The emitted ID is only returned after _is_jumpstart_model_id() validated it via model_uris.retrieve, i.e. a public JumpStart catalog ID.

Tests ✅ — Good coverage: model-ID present/absent for both build() and deploy(), plus a test_attr_call_skips_none unit test for the decorator change. Mocking _is_jumpstart_model_id is the right scope (the helper's own detection is exercised elsewhere).

Findings

1. test_jumpstart_telemetry_flag.py:132-134 — missing blank line will fail CI (build-blocker). There is only one blank line between the end of the test class and the if __name__ == "__main__": block. flake8 (E305, not in the ignore list of sagemaker-serve/tox.ini) and black-check both require two blank lines here, and both run in the PR check tox envs. Add a second blank line before line 134 so lint/format checks pass.

2. Stale PR description (non-blocking). The body describes emitting a boolean &x-isJumpstartModelId= flag alongside the model ID, but the current diff only emits &x-jumpstartModelId= (the boolean param was added then removed per commit fd5329c). The tests even assert "&x-isJumpstartModelId=" not in extra. Worth updating the description so reviewers aren't misled — no code change needed.

Minor / non-blocking: &x-jumpstartModelId=<value> is inserted unencoded (unlike created_by, which uses quote(...)). JumpStart catalog IDs are URL-safe in practice, so this is fine — just noting the asymmetry.

No security or performance concerns. Aside from the blank-line fix (which will otherwise turn CI red), this LGTM. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant