Feat/finetuningoptions validation telemetry - #6286

Claude finished @rsareddy0329's task in 1m 24s —— View job

Code Review: FineTuningOptions validation telemetry

• Read PR diff and changed files
• Verify telemetry imports & signatures against sagemaker-core
• Analyze correctness, security/privacy, performance, tests
• Post review

Note: the inline-comment tool isn't available in this run, so findings are consolidated below with file/line references.

The change is well-scoped and the mechanics are correct: the new imports (_send_telemetry_request, _classify_error, _get_default_sagemaker_session, STATUS_TO_CODE, FEATURE_TO_CODE, SDK_VERSION, Status) all resolve, and the positional args passed to _send_telemetry_request(status, feature_list, session, failure_reason, failure_type, extra_info) line up correctly. The best-effort try/except wrapper and the "re-raise after emit" flow correctly guarantee telemetry never masks the user-facing validation error, and the tests cover that. Two substantive points:

🔴 Telemetry opt-out is not honored on this new path

common.py:109-125 — _emit_validation_failure calls _send_telemetry_request directly, without checking the TelemetryOptOut config flag. Every other emit site in this codebase gates on it: the _telemetry_emitter decorator resolves telemetry_opt_out_flag via resolve_value_from_config(..., config_path=TELEMETRY_OPT_OUT_PATH, ...) and only sends if not telemetry_opt_out_flag (see telemetry_logging.py:316-324, 398, 412).

As written, a user who has explicitly opted out of telemetry will still have these validation-failure events emitted. That's a privacy/compliance regression and is inconsistent with the rest of the module. Please resolve the opt-out flag (using the resolved session) and early-return when it is set, e.g.:

session = _get_default_sagemaker_session()
if not session:
    return
if resolve_value_from_config(
    direct_input=None,
    config_path=TELEMETRY_OPT_OUT_PATH,
    default_value=False,
    sagemaker_session=session,
):
    return

A unit test asserting mock_send.assert_not_called() when opt-out is set would lock this in.

🟡 extra string omits the standard env/platform fields

common.py:113-117 — the decorator's extra includes &x-env=<py>, &x-sys=<os>, &x-platform=<studio> (telemetry_logging.py:365-371). This new payload includes only func_name, x-sdkVersion, and x-errorCategory. Not a bug, but if these events are meant to be analyzed alongside the decorator-emitted ones, the missing platform dimensions will make them harder to slice consistently. Consider reusing the same fields for parity.

Minor

• common.py:126 — the # noqa: E722 comment is slightly off: E722 flags bare except:, but this is except Exception: (which isn't E722). The rest of the module uses # pylint: disable=W0703 for the same intent; matching that would be more accurate.
• Every validation failure creates a fresh boto3.Session() + Session(...) via _get_default_sagemaker_session() and spawns a daemon thread that makes an STS get_caller_identity() call. This is off the happy path and infrequent, so it's acceptable — just flagging that a caller looping over many invalid sets would incur repeated STS calls.

Overall: the approach and tests are solid; the opt-out gap is the one item I'd consider blocking before merge.
· feat/finetuningoptions-validation-telemetry