Skip to content

feat(python-sdk): telemetry#71

Open
czi-fsisenda wants to merge 3 commits into
mainfrom
fsisenda/sdk_python_telemetry
Open

feat(python-sdk): telemetry#71
czi-fsisenda wants to merge 3 commits into
mainfrom
fsisenda/sdk_python_telemetry

Conversation

@czi-fsisenda
Copy link
Copy Markdown
Contributor

Summary

  • implements telemetry in the same form as TypeScript SDK
  • adapts Python SDKs proposed telemetry shape to TypesScript shape
  • different implementation for generating telemetry client ids.
    • doesn't create persistent ids saved on disk.
    • should use same anonymous client id for same application run.
    • should use same client id for application run when user provides partner key

Test plan

  • Unit tests
  • Pending dry-run

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Python SDK telemetry implementation aligned with the existing TypeScript SDK telemetry event shape, wires telemetry sending into the evaluator lifecycle, and introduces supporting schemas/utilities plus unit tests.

Changes:

  • Introduces TS-shaped telemetry event models and an adapter from Python evaluation metadata to that event payload.
  • Adds fire-and-forget HTTP telemetry sending (httpx) and wires BaseEvaluator.evaluate() to schedule telemetry after each run.
  • Extends Python config to support telemetry endpoint defaults and client-id derivation; adds unit tests covering telemetry behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
sdks/python/src/learning_commons_evaluators/telemetry/init.py Telemetry send + scheduling implementation (async httpx + daemon thread).
sdks/python/src/learning_commons_evaluators/telemetry/adapter.py Maps Python evaluation metadata/input into a TS-shaped telemetry payload.
sdks/python/src/learning_commons_evaluators/telemetry/utils.py Deterministic client-id derivation helper (UUIDv5).
sdks/python/src/learning_commons_evaluators/schemas/ts_telemetry.py Pydantic models mirroring TS telemetry wire types.
sdks/python/src/learning_commons_evaluators/schemas/config.py Adds telemetry endpoint default, anonymous telemetry factories, and client_id_seed.
sdks/python/src/learning_commons_evaluators/schemas/init.py Re-exports new telemetry schema types.
sdks/python/src/learning_commons_evaluators/evaluators/base.py Wires schedule_send_telemetry into evaluation completion (success/failure).
sdks/python/pyproject.toml Adds httpx dependency for telemetry transport.
sdks/python/tests/telemetry/test_telemetry.py Tests send/schedule guards, headers, and payload inclusion rules.
sdks/python/tests/telemetry/test_telemetry_utils.py Tests deterministic client-id derivation behavior.
sdks/python/tests/telemetry/test_telemetry_adapter.py Tests adapter mapping and token aggregation behavior.
sdks/python/tests/schemas/test_config.py Tests new telemetry config defaults and new factory helpers.
sdks/python/tests/evaluators/test_base.py Tests that telemetry scheduling is invoked and receives correct args/status.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sdks/python/src/learning_commons_evaluators/schemas/config.py Outdated
Comment thread sdks/python/src/learning_commons_evaluators/telemetry/__init__.py Outdated
Comment thread sdks/python/src/learning_commons_evaluators/telemetry/__init__.py Outdated
Comment thread sdks/python/src/learning_commons_evaluators/telemetry/adapter.py Outdated
@czi-fsisenda czi-fsisenda changed the title feat: python sdk telemetry feat(python-sdk): telemetry May 20, 2026
@czi-fsisenda czi-fsisenda marked this pull request as ready for review May 20, 2026 03:59
@adnanrhussain adnanrhussain force-pushed the fsisenda/sdk_python_telemetry branch from a9b4b49 to 8689a8b Compare May 21, 2026 13:41
Copy link
Copy Markdown
Contributor

@adnanrhussain adnanrhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Pls just review the P0s. Thank you!



def _grade_from_input_metadata(input_metadata: dict[str, Any]) -> str | None:
gl = input_metadata.get("grade_level")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 - Could you please double check this? Looking at the input objects, it looks like the field is grade field and this will be None for both Conventionality and Vocabulary

Comment on lines +106 to +107
parts = [_provider_label(u) for u in total.values()]
return " + ".join(sorted(parts))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove spaces and maintain calling order

Suggested change
parts = [_provider_label(u) for u in total.values()]
return " + ".join(sorted(parts))
return "+".join(_provider_label(u) for u in total.values())


# Shared per process so multiple :class:`EvaluatorConfig` instances derive the same client id.
_PROCESS_CLIENT_ID_SEED = uuid.uuid4()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 - I would strongly recommend making the telemetry anonymous in the config by default, ie. not even needing to specify the config_config_anonymous_telemetry but just using create_config for the happy path default + primary documented case.

# Anonymous (default)
config = create_config(google_llm_provider_config=...)

# Tracked
config = create_config(google_llm_provider_config=..., telemetry_partner_id=LC_KEY)

# Off
config = create_config_no_telemetry(google_llm_provider_config=...)

And then we can module-cache the _ANONYMOUS_CLIENT_ID across create_configs / Evals

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.

3 participants